PS-10113 [8.4] Server crashes if JS routine tries to allocate more than internal heap memory limit

https://perconadev.atlassian.net/browse/PS-10113

Attempt to allocate more than js_lang.max_mem_size * 4 bytes of memory
on JS heap in one go caused server crash.

The problem occurred because such allocation exceeded V8 memory limit,
which was set by our JS language component to be 4x from the memory
limit specified by used through js_lang.max_mem_size variable.

The idea was that we should be able to detect exceeding max_mem_size
limit well before reaching V8 memory limit. It worked OK in case of
small allocations, but failed in case when the problem was caused
by single huge allocation.

V8 is not good at handling OOM situations gracefully and simply aborts
process if allocation is about to exceed its memory limit and extra GC
doesn't help.

This patch tries to alleviate the problem by not setting V8 memory limit
explicitly and relying on its generous default instead (typically > 1Gb).
As result only really huge allocation attempts should be affected.

The old behavior is still can be achieved by setting newly introduced
js_lang.max_mem_size_hard_limit_factor system variable to non-0 value.
In this case JS language component will set V8 memory limit to
max_mem_size * max_mem_size_hard_limit_factor bytes, and one should be
ready to tolerate process aborts if this limit is exceeded. To reduce
chance of misuse changing of this new variable requires server restart.

Author: dlenev

components/js_lang/js_lang_common.h

@@ -113,10 +113,11 @@ static constexpr std::string_view CREATE_PRIVILEGE_NAME = "CREATE_JS_ROUTINE";
 // Defined as a macro so we can easier concatenate it with other literals.
 #define MAX_CONSOLE_LOG_SIZE_VAR_NAME "max_console_log_size"
 
-// Name of system variable which limits the size of per isolate memory.
+// Names of system variables which limit the size of per isolate memory.
 //
-// Defined as a macro so we can easier concatenate it with other literals.
+// Defined as macros so we can easier concatenate them with other literals.
 #define MAX_MEM_SIZE_VAR_NAME "max_mem_size"
+#define MAX_MEM_SIZE_HARD_LIMIT_FACTOR_VAR_NAME "max_mem_size_hard_limit_factor"
 
 // We use RapidJSON to produce console log and information about memory
 // usage in JSON format.

components/js_lang/js_lang_core.cc

@@ -301,27 +301,24 @@ Js_isolate *Js_isolate::create() {
     isolated processes, for which "let us crash on reaching the limit"
     behavior doesn't cause problems.
 
-    We do best-effort attempt to respect our memory limits without crashes
-    by employing the following approach:
-    - Set V8 limit MAX_MEM_SIZE_SAFETY_MARGIN_FACTOR times (4x at the moment)
-      higher than our intended limit.
-    - Check if our intended limit is exceeded periodically (at GC time).
+    By default, we try to avoid such aborts and do not set V8 memory limit
+    explicitly, relying on its generous default (typically > 1Gb).
+    We only do best-effort attempt to respect max_mem_size memory limit by
+    checking if it is exceeded periodically (at GC time).
 
-    TODO: Consider changing safety multiplier after getting more feedback
-          from users (PLv8 uses x2, but some of our tests easily crashed with
-          such multiplier, some other V8 embedders seem to use x8).
-
-          Note that if we won't set heap limit explicitly it will be set
-          automatically by V8 to some large value anyway (1Gb).
+    Users can set smaller V8 limit by setting max_mem_size_hard_limit_factor
+    to non-0 value if they are willing to risk server aborts or if such aborts
+    are preferrable to exceeding memory limits.
 
     TODO: Consider having a hardened mode for JS execution in which it will
           be run in separate worker process (like it is done in Chrome).
   */
 
-  constexpr size_t MAX_MEM_SIZE_SAFETY_MARGIN_FACTOR = 4;
-  create_params.constraints.ConfigureDefaultsFromHeapSize(
-      0, result->m_memory_manager.m_max_mem_size *
-             MAX_MEM_SIZE_SAFETY_MARGIN_FACTOR);
+  if (Js_isolate::Memory_manager::s_max_mem_size_hard_limit_factor > 0) {
+    create_params.constraints.ConfigureDefaultsFromHeapSize(
+        0, result->m_memory_manager.m_max_mem_size *
+               Js_isolate::Memory_manager::s_max_mem_size_hard_limit_factor);
+  }
 
   v8::Isolate *isolate = v8::Isolate::New(create_params);
 
@@ -573,18 +570,20 @@ constexpr unsigned int MAX_MEM_SIZE_DEFAULT = 8 * 1024 * 1024;
 constexpr unsigned int MAX_MEM_SIZE_MIN = 3 * 1024 * 1024;
 
 unsigned int Js_isolate::Memory_manager::s_max_mem_size = MAX_MEM_SIZE_DEFAULT;
+unsigned int Js_isolate::Memory_manager::s_max_mem_size_hard_limit_factor = 0;
 
 bool Js_isolate::register_vars() {
   /*
-    Register variable for limiting per Isolate memory consumption.
+    Register variables for limiting of per Isolate memory consumption.
 
-    Note that while minimum value for this setting might seem to be fairly
-    big it was determined by expiriments. V8 tends to overrides/increases
-    smaller values anyway.
+    Note that while minimum value for max_mem_size setting might seem to be
+    fairly big it was determined by expiriments. V8 tends to overrides/
+    increases smaller values anyway.
 
     TODO: Possibly change the default after gathering more feedback from users.
   */
-  INTEGRAL_CHECK_ARG(uint) max_mem_size_check;
+  INTEGRAL_CHECK_ARG(uint) max_mem_size_check, max_mem_size_hl_factor_check;
+
   max_mem_size_check.def_val = MAX_MEM_SIZE_DEFAULT;
   max_mem_size_check.min_val = MAX_MEM_SIZE_MIN;
   max_mem_size_check.max_val = 1024 * 1024 * 1024;
@@ -593,7 +592,7 @@ bool Js_isolate::register_vars() {
   if (mysql_service_component_sys_variable_register->register_variable(
           CURRENT_COMPONENT_NAME_STR, MAX_MEM_SIZE_VAR_NAME,
           PLUGIN_VAR_INT | PLUGIN_VAR_UNSIGNED,
-          "Maximum JS memory size for each user and connection", nullptr,
+          "Soft limit for JS memory size for each user and connection", nullptr,
           nullptr, (void *)&max_mem_size_check,
           (void *)&Memory_manager::s_max_mem_size)) {
     // Registration of system variable is non-trivial, and can fail for
@@ -605,12 +604,42 @@ bool Js_isolate::register_vars() {
     return true;
   }
 
+  /*
+    We do not enable hard JS memory limit by default, as hitting it causes
+    process abort and, in general case, it is fairly easy to do so without
+    triggering the best-effort limit first. We also do not allow turning it on/
+    changing this setting at runtime to reduce misuse.
+  */
+  max_mem_size_hl_factor_check.def_val = 0;
+  max_mem_size_hl_factor_check.min_val = 0;
+  max_mem_size_hl_factor_check.max_val = 1024;
+  max_mem_size_hl_factor_check.blk_sz = 0;
+
+  if (mysql_service_component_sys_variable_register->register_variable(
+          CURRENT_COMPONENT_NAME_STR, MAX_MEM_SIZE_HARD_LIMIT_FACTOR_VAR_NAME,
+          PLUGIN_VAR_INT | PLUGIN_VAR_UNSIGNED | PLUGIN_VAR_READONLY,
+          "Multiplier defining hard limit for JS memory size for each"
+          "user and connection (0 means that V8 default is used)",
+          nullptr, nullptr, (void *)&max_mem_size_hl_factor_check,
+          (void *)&Memory_manager::s_max_mem_size_hard_limit_factor)) {
+    // See above.
+    my_error(ER_LANGUAGE_COMPONENT, MYF(0),
+             "Can't register " MAX_MEM_SIZE_HARD_LIMIT_FACTOR_VAR_NAME
+             " system variable for " CURRENT_COMPONENT_NAME_STR " component.");
+    // We can't do much if the below call fails.
+    (void)mysql_service_component_sys_variable_unregister->unregister_variable(
+        CURRENT_COMPONENT_NAME_STR, MAX_MEM_SIZE_VAR_NAME);
+    return true;
+  }
+
   if (mysql_service_status_variable_registration->register_variable(
           get_status_vars_defs())) {
     // Registration of status variables can't fail unless OOM.
     // In this case error should have been reported already.
     //
-    // We can't do much if we the below call fails.
+    // We can't do much if the below calls fail.
+    (void)mysql_service_component_sys_variable_unregister->unregister_variable(
+        CURRENT_COMPONENT_NAME_STR, MAX_MEM_SIZE_HARD_LIMIT_FACTOR_VAR_NAME);
     (void)mysql_service_component_sys_variable_unregister->unregister_variable(
         CURRENT_COMPONENT_NAME_STR, MAX_MEM_SIZE_VAR_NAME);
     return true;

components/js_lang/js_lang_core.h

@@ -393,11 +393,21 @@ class Js_isolate {
     bool m_is_max_mem_size_exceeded{false};
 
     /**
-      Global dynamic variable for maximum memory size limit to set for
+      Global dynamic variable for maximum memory size soft limit to set for
       new isolates.
     */
     static unsigned int s_max_mem_size;
 
+    /**
+      Global read-only (i.e. start-up settable) variable for hard memory limit
+      in V8 engine.
+      0 - indicates that no V8 limit is set explicitly and the default V8 limit
+      is used (typically > 1Gb),
+      non-0  - indicates that V8 limit is set explicitly. The value used as a
+      multiplier which we use to produce limit value from m_max_mem_size.
+    */
+    static unsigned int s_max_mem_size_hard_limit_factor;
+
     /**
       Memory usage counter values for this isolate which already have been
       agreggated into global memory usage counters.

mysql-test/suite/component_js_lang/r/js_lang_mem_limit.result

@@ -7,22 +7,23 @@ GRANT CREATE_JS_ROUTINE ON *.* TO root@localhost;
 SELECT @@js_lang.max_mem_size;
 @@js_lang.max_mem_size
 8388608
-#
-# 1) Create routine which will hog prescribed amount of memory on the
-#    V8 heap in the process of its execution.
+# Create routine which will hog prescribed amount of memory on the
+# V8 heap in the process of its execution.
 #
 # JSON.parse(JSON.stringify()) trick is necessary to suppress V8 string
 # optimizations which will cause much smaller memory usage if applied.
+CREATE PROCEDURE p_heap_hog(size INT) LANGUAGE JS AS
+$$ let s = JSON.parse(JSON.stringify(("A").repeat(size/2 /* Two copies of string! */))) $$;
+#
+# 1) Test how we enforce memory limits for allocations on V8 heap.
 #
-# Note that since we can only do "best effort" memory limit detection
-# with V8, spike allocation of memory still might cause V8/process
-# aborts, or go unnoticed if memory is quickly released.
+# Note that since, by default, we can only do "best effort" memory limit
+# detection with V8, spike allocation of memory might go unnoticed if
+# memory is quickly released.
 #
 # So the below tests might have to be adjusted to slow down memory
 # allocation (to let detection kick in) or disabled if it starts to
 # misbehave on some machine.
-CREATE PROCEDURE p_heap_hog(size INT) LANGUAGE JS AS
-$$ let s = JSON.parse(JSON.stringify(("A").repeat(size/2 /* Two copies of string! */))) $$;
 #
 # Check that we detect exceeding the limit and report it.
 CALL p_heap_hog(10*1000*1000);
@@ -35,7 +36,13 @@ ERROR HY000: Memory limit exceeded.
 CALL p_heap_hog(5*1000*1000);
 CALL p_heap_hog(5*1000*1000);
 CALL p_heap_hog(5*1000*1000);
-DROP PROCEDURE p_heap_hog;
+#
+# Also check what happens if we try to hog way more than the limit.
+#
+# This was the problem in PS-10113 "Server crashes if JS routine tries
+# to allocate more than internal heap memory limit".
+CALL p_heap_hog(40*1000*1000);
+ERROR HY000: Memory limit exceeded.
 #
 # 2) Now create routine which will hog prescribed amount of memory on
 #    ArrayBuffer allocator.
@@ -290,7 +297,51 @@ glob_ems	peak_ems
 # Local clean-up.
 DROP PROCEDURE p1;
 DROP FUNCTION f1;
+#
+# 7) Test coverage for max_mem_size and max_mem_size_hard_limit_factor
+#    system variables.
+#
+#
+# 7.1) Increasing max_mem_size variable should allow to allocate
+#      more in new connections/isolates.
+#
+SET GLOBAL js_lang.max_mem_size = 16*1024*1024;
+connect  con_add2, localhost, root,,;
+CALL p_heap_hog(10*1000*1000);
+disconnect con_add2;
+connection default;
+SET GLOBAL js_lang.max_mem_size = DEFAULT;
+#
+# 7.2) But max_mem_size_hard_limit_factor variable should not be
+#      settable at runtime.
+#
+SET GLOBAL js_lang.max_mem_size_hard_limit_factor = 2;
+ERROR HY000: Variable 'js_lang.max_mem_size_hard_limit_factor' is a read only variable
+#
+# 7.3) OTOH it should be possible to change this variable
+#      using PERSIST_ONLY and restart.
+#
+SET PERSIST_ONLY js_lang.max_mem_size_hard_limit_factor = 2;
+# restart
+SELECT @@js_lang.max_mem_size_hard_limit_factor;
+@@js_lang.max_mem_size_hard_limit_factor
+2
+#
+# 7.4) Exceeding hard limit set by variable should lead to
+#      server abort.
+#
+CALL p_heap_hog(20*1000*1000);
+ERROR HY000: Lost connection to MySQL server during query
+# restart
+# Let us restore factor variable value now.
+# Another server restart is needed for this.
+RESET PERSIST js_lang.max_mem_size_hard_limit_factor;
+# restart
+SELECT @@js_lang.max_mem_size_hard_limit_factor;
+@@js_lang.max_mem_size_hard_limit_factor
+0
 # Global clean-up.
+DROP PROCEDURE p_heap_hog;
 REVOKE CREATE_JS_ROUTINE ON *.* FROM root@localhost;
 # Disconnect of default connection to free the only remaining
 # connection context and isolate, so we can uninstall component.

mysql-test/suite/component_js_lang/t/js_lang_mem_limit.test

@@ -3,6 +3,10 @@
 #
 --source include/have_js_lang_component.inc
 
+# Valgrind and CrashReporter are problematic for testing crashes.
+--source include/not_valgrind.inc
+--source include/not_crashrep.inc
+
 --echo # Global prepare of playground.
 --enable_connect_log
 
@@ -15,22 +19,24 @@ GRANT CREATE_JS_ROUTINE ON *.* TO root@localhost;
 --echo # parameters of calls below need to be ajusted.
 SELECT @@js_lang.max_mem_size;
 
---echo #
---echo # 1) Create routine which will hog prescribed amount of memory on the
---echo #    V8 heap in the process of its execution.
+--echo # Create routine which will hog prescribed amount of memory on the
+--echo # V8 heap in the process of its execution.
 --echo #
 --echo # JSON.parse(JSON.stringify()) trick is necessary to suppress V8 string
 --echo # optimizations which will cause much smaller memory usage if applied.
+CREATE PROCEDURE p_heap_hog(size INT) LANGUAGE JS AS
+    $$ let s = JSON.parse(JSON.stringify(("A").repeat(size/2 /* Two copies of string! */))) $$;
+
+--echo #
+--echo # 1) Test how we enforce memory limits for allocations on V8 heap.
 --echo #
---echo # Note that since we can only do "best effort" memory limit detection
---echo # with V8, spike allocation of memory still might cause V8/process
---echo # aborts, or go unnoticed if memory is quickly released.
+--echo # Note that since, by default, we can only do "best effort" memory limit
+--echo # detection with V8, spike allocation of memory might go unnoticed if
+--echo # memory is quickly released.
 --echo #
 --echo # So the below tests might have to be adjusted to slow down memory
 --echo # allocation (to let detection kick in) or disabled if it starts to
 --echo # misbehave on some machine.
-CREATE PROCEDURE p_heap_hog(size INT) LANGUAGE JS AS
-    $$ let s = JSON.parse(JSON.stringify(("A").repeat(size/2 /* Two copies of string! */))) $$;
 
 --echo #
 --echo # Check that we detect exceeding the limit and report it.
@@ -46,7 +52,13 @@ CALL p_heap_hog(5*1000*1000);
 CALL p_heap_hog(5*1000*1000);
 CALL p_heap_hog(5*1000*1000);
 
-DROP PROCEDURE p_heap_hog;
+--echo #
+--echo # Also check what happens if we try to hog way more than the limit.
+--echo #
+--echo # This was the problem in PS-10113 "Server crashes if JS routine tries
+--echo # to allocate more than internal heap memory limit".
+--error ER_LANGUAGE_COMPONENT
+CALL p_heap_hog(40*1000*1000);
 
 --echo #
 --echo # 2) Now create routine which will hog prescribed amount of memory on
@@ -253,7 +265,70 @@ SELECT JSON_EXTRACT(@mem_usage, "$.global.externalMemorySize") < 1024  AS glob_e
 DROP PROCEDURE p1;
 DROP FUNCTION f1;
 
+--echo #
+--echo # 7) Test coverage for max_mem_size and max_mem_size_hard_limit_factor
+--echo #    system variables.
+--echo #
+
+--echo #
+--echo # 7.1) Increasing max_mem_size variable should allow to allocate
+--echo #      more in new connections/isolates.
+--echo #
+
+SET GLOBAL js_lang.max_mem_size = 16*1024*1024;
+
+--connect (con_add2, localhost, root,,)
+
+CALL p_heap_hog(10*1000*1000);
+
+--disconnect con_add2
+--source include/wait_until_disconnected.inc
+--connection default
+
+SET GLOBAL js_lang.max_mem_size = DEFAULT;
+
+--echo #
+--echo # 7.2) But max_mem_size_hard_limit_factor variable should not be
+--echo #      settable at runtime.
+--echo #
+--error ER_INCORRECT_GLOBAL_LOCAL_VAR
+SET GLOBAL js_lang.max_mem_size_hard_limit_factor = 2;
+
+--echo #
+--echo # 7.3) OTOH it should be possible to change this variable
+--echo #      using PERSIST_ONLY and restart.
+--echo #
+
+SET PERSIST_ONLY js_lang.max_mem_size_hard_limit_factor = 2;
+
+--source include/restart_mysqld.inc
+
+SELECT @@js_lang.max_mem_size_hard_limit_factor;
+
+--echo #
+--echo # 7.4) Exceeding hard limit set by variable should lead to
+--echo #      server abort.
+--echo #
+
+--source include/expect_crash.inc
+
+--error CR_SERVER_LOST
+CALL p_heap_hog(20*1000*1000);
+
+--source include/start_mysqld.inc
+
+--echo # Let us restore factor variable value now.
+--echo # Another server restart is needed for this.
+
+RESET PERSIST js_lang.max_mem_size_hard_limit_factor;
+
+--source include/restart_mysqld.inc
+
+SELECT @@js_lang.max_mem_size_hard_limit_factor;
+
 --echo # Global clean-up.
+DROP PROCEDURE p_heap_hog;
+
 REVOKE CREATE_JS_ROUTINE ON *.* FROM root@localhost;
 
 --echo # Disconnect of default connection to free the only remaining