Fixed copying of closures for declared functions.

After 0a2a0b5a74f4 (0.6.0),  the referencing of a closure value inside of
a nested function may result in heap-use-after-free.  For this to happen
the closure value have to be referenced in a function invoked asynchronously.
For example if a closure value is referenced in r.subrequest() or setTimeout()
handler.

The problem was that closure values of nested function were assigned during
the function call and the memory shared between all the cloned VMs was used
to store temporary assignments until the moment the declared function was
referenced.  When two VMs executed concurrently, the first VM might see
the changed made by the second VM if the first one was suspended.

The fix is to copy all declared functions at the time of the call.

This closes #421 issue on GitHub.

diff --git a/src/njs_function.c b/src/njs_function.c
index e61f4e62c7990acef51ec5b3bead8e187e210a41..b166ef202d97f7c0205c7c14a68b4a1aea972ddf 100644 (file)
--- a/src/njs_function.c
+++ b/src/njs_function.c
@@ -615,6 +615,7 @@ njs_function_lambda_call(njs_vm_t *vm)
     njs_value_t            *args, **local, *value;
     njs_value_t            **cur_local, **cur_closures, **cur_temp;
     njs_function_t         *function;
+    njs_declaration_t      *declr;
     njs_function_lambda_t  *lambda;
 
     frame = (njs_frame_t *) vm->top_frame;
@@ -680,7 +681,15 @@ njs_function_lambda_call(njs_vm_t *vm)
     while (n != 0) {
         n--;
 
-        function = njs_function(lambda->declarations[n]);
+        declr = &lambda->declarations[n];
+        value = njs_scope_value(vm, declr->index);
+
+        *value = *declr->value;
+
+        function = njs_function_value_copy(vm, value);
+        if (njs_slow_path(function == NULL)) {
+            return NJS_ERROR;
+        }
 
         ret = njs_function_capture_closure(vm, function, function->u.lambda);
         if (njs_slow_path(ret != NJS_OK)) {

diff --git a/src/njs_function.h b/src/njs_function.h
index 07e058bdd5ca437e951de567421de72de0a6b8e1..f936547c4244a57f457166b8665fdfec66f36b1b 100644 (file)
--- a/src/njs_function.h
+++ b/src/njs_function.h
@@ -14,7 +14,7 @@ struct njs_function_lambda_s {
     uint32_t                       nlocal;
     uint32_t                       temp;
 
-    njs_value_t                    **declarations;
+    njs_declaration_t              *declarations;
     uint32_t                       ndeclarations;
 
     njs_index_t                    self;

diff --git a/src/njs_parser.h b/src/njs_parser.h
index 49524c277916e6fc03e9534288c7cb5fc35a011d..2e1f23bcdfe8a701abf434af581ea8a40c1886b1 100644 (file)
--- a/src/njs_parser.h
+++ b/src/njs_parser.h
@@ -104,6 +104,12 @@ typedef struct {
 } njs_parser_rbtree_node_t;
 
 
+typedef struct {
+    njs_value_t                     *value;
+    njs_index_t                     index;
+} njs_declaration_t;
+
+
 typedef njs_int_t (*njs_parser_traverse_cb_t)(njs_vm_t *vm,
     njs_parser_node_t *node, void *ctx);
 

diff --git a/src/njs_variable.c b/src/njs_variable.c
index 811886c72296680e0ee7b03d8a8d77ff5fea92f8..d47836c7d846d7184894ac560efd138a0e9ee23f 100644 (file)
--- a/src/njs_variable.c
+++ b/src/njs_variable.c
@@ -9,7 +9,7 @@
 #include <njs_main.h>
 
 
-static njs_value_t **njs_variable_scope_function_add(njs_parser_t *parser,
+static njs_declaration_t *njs_variable_scope_function_add(njs_parser_t *parser,
     njs_parser_scope_t *scope);
 static njs_parser_scope_t *njs_variable_scope_find(njs_parser_t *parser,
      njs_parser_scope_t *scope, uintptr_t unique_id, njs_variable_type_t type);
@@ -39,8 +39,8 @@ njs_variable_function_add(njs_parser_t *parser, njs_parser_scope_t *scope,
     uintptr_t unique_id, njs_variable_type_t type)
 {
     njs_bool_t             ctor;
-    njs_value_t            **declr;
     njs_variable_t         *var;
+    njs_declaration_t      *declr;
     njs_parser_scope_t     *root;
     njs_function_lambda_t  *lambda;
 
@@ -76,10 +76,12 @@ njs_variable_function_add(njs_parser_t *parser, njs_parser_scope_t *scope,
             return NULL;
         }
 
-        *declr = &var->value;
-
         var->index = njs_scope_index(root->type, root->items, NJS_LEVEL_LOCAL,
                                      type);
+
+        declr->value = &var->value;
+        declr->index = var->index;
+
         root->items++;
     }
 
@@ -90,12 +92,12 @@ njs_variable_function_add(njs_parser_t *parser, njs_parser_scope_t *scope,
 }
 
 
-static njs_value_t **
+static njs_declaration_t *
 njs_variable_scope_function_add(njs_parser_t *parser, njs_parser_scope_t *scope)
 {
     if (scope->declarations == NULL) {
         scope->declarations = njs_arr_create(parser->vm->mem_pool, 1,
-                                             sizeof(njs_value_t *));
+                                             sizeof(njs_declaration_t));
         if (njs_slow_path(scope->declarations == NULL)) {
             return NULL;
         }

diff --git a/src/test/njs_unit_test.c b/src/test/njs_unit_test.c
index e81f628c975449e81dd77e7fa857ef6e6eba709d..ae97e76e7bf4af3189782c1a7d846680ea227d77 100644 (file)
--- a/src/test/njs_unit_test.c
+++ b/src/test/njs_unit_test.c
@@ -20964,7 +20964,6 @@ static njs_unit_test_t  njs_async_handler_test[] =
               ),
       njs_str("1") },
 
-#if 0 /* FIXME */
     { njs_str("globalThis.main = (function() {"
               "     let obj = { a: 1, b: 2};"
               "     function cb(r) { r.retval(obj.a); }"
@@ -20975,7 +20974,6 @@ static njs_unit_test_t  njs_async_handler_test[] =
               "})();"
               ),
       njs_str("1") },
-#endif
 };