[njs] Fixed module importing using require().

[Dmitry Volyntsev]

details:   https://hg.nginx.org/njs/rev/b79a22d6d7f3
branches:  
changeset: 1132:b79a22d6d7f3
user:      Dmitry Volyntsev <xeioex at nginx.com>
date:      Thu Aug 15 19:22:01 2019 +0300
description:
Fixed module importing using require().

Previously, require() did not make a mutable copy of imported module.
As a result, cloned VMs could change a shared module object making
module.object->hash inconsistent.

Before 04d7a5d93ae6, the problem manifested itself only in "PROP GET"
operations, for example, using "require('fs').renameSync", because
njs_value_property() makes a mutable copy of shared methods in
module.object->hash.

After 04d7a5d93ae6, "METHOD CALL" operations, such as
"require('fs').renameSync()", now have the same problem as in "PROP
GET".

Importing modules using "import" statement is not affected, because
for it "OBJECT COPY" instruction is generated, which makes a mutable
copy of imported module object.

The fix is to make a mutable copy of a shared module in require().

This closes #206 issue on Github.

diffstat:

 src/njs_module.c         |  18 ++++++++++++---
 src/test/njs_unit_test.c |  52 ++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 58 insertions(+), 12 deletions(-)

diffs (134 lines):

diff -r 8b2f303ab48a -r b79a22d6d7f3 src/njs_module.c
--- a/src/njs_module.c	Wed Aug 14 20:22:32 2019 +0300
+++ b/src/njs_module.c	Thu Aug 15 19:22:01 2019 +0300
@@ -503,9 +503,10 @@ njs_module_insert(njs_vm_t *vm, njs_modu
 
 
 njs_int_t
-njs_module_require(njs_vm_t *vm, njs_value_t *args,
-    njs_uint_t nargs, njs_index_t unused)
+njs_module_require(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
+    njs_index_t unused)
 {
+    njs_object_t        *object;
     njs_module_t        *module;
     njs_lvlhsh_query_t  lhq;
 
@@ -520,9 +521,18 @@ njs_module_require(njs_vm_t *vm, njs_val
 
     if (njs_lvlhsh_find(&vm->modules_hash, &lhq) == NJS_OK) {
         module = lhq.value;
-        module->object.__proto__ = &vm->prototypes[NJS_PROTOTYPE_OBJECT].object;
 
-        njs_set_object(&vm->retval, &module->object);
+        object = njs_mp_alloc(vm->mem_pool, sizeof(njs_object_t));
+        if (njs_slow_path(object == NULL)) {
+            njs_memory_error(vm);
+            return NJS_ERROR;
+        }
+
+        *object = module->object;
+        object->__proto__ = &vm->prototypes[NJS_PROTOTYPE_OBJECT].object;
+        object->shared = 0;
+
+        njs_set_object(&vm->retval, object);
 
         return NJS_OK;
     }
diff -r 8b2f303ab48a -r b79a22d6d7f3 src/test/njs_unit_test.c
--- a/src/test/njs_unit_test.c	Wed Aug 14 20:22:32 2019 +0300
+++ b/src/test/njs_unit_test.c	Thu Aug 15 19:22:01 2019 +0300
@@ -13489,6 +13489,22 @@ static njs_unit_test_t  njs_module_test[
 };
 
 
+static njs_unit_test_t  njs_shared_test[] =
+{
+    { njs_str("var cr = require('crypto'); cr.createHash"),
+      njs_str("[object Function]") },
+
+    { njs_str("var cr = require('crypto'); cr.createHash('md5')"),
+      njs_str("[object Hash]") },
+
+    { njs_str("import cr from 'crypto'; cr.createHash"),
+      njs_str("[object Function]") },
+
+    { njs_str("import cr from 'crypto'; cr.createHash('md5')"),
+      njs_str("[object Hash]") },
+};
+
+
 static njs_unit_test_t  njs_tz_test[] =
 {
      { njs_str("var d = new Date(1); d = d + ''; d.slice(0, 33)"),
@@ -14278,6 +14294,7 @@ typedef struct {
     njs_bool_t  disassemble;
     njs_bool_t  verbose;
     njs_bool_t  module;
+    njs_uint_t  repeat;
 } njs_opts_t;
 
 
@@ -14308,7 +14325,7 @@ njs_unit_test(njs_unit_test_t tests[], s
     njs_vm_t      *vm, *nvm;
     njs_int_t     ret;
     njs_str_t     s;
-    njs_uint_t    i;
+    njs_uint_t    i, repeat;
     njs_stat_t    prev;
     njs_bool_t    success;
     njs_vm_opt_t  options;
@@ -14350,13 +14367,21 @@ njs_unit_test(njs_unit_test_t tests[], s
                 njs_disassembler(vm);
             }
 
-            nvm = njs_vm_clone(vm, NULL);
-            if (nvm == NULL) {
-                njs_printf("njs_vm_clone() failed\n");
-                goto done;
-            }
-
-            ret = njs_vm_start(nvm);
+            repeat = opts->repeat;
+
+            do {
+                if (nvm != NULL) {
+                    njs_vm_destroy(nvm);
+                }
+
+                nvm = njs_vm_clone(vm, NULL);
+                if (nvm == NULL) {
+                    njs_printf("njs_vm_clone() failed\n");
+                    goto done;
+                }
+
+                ret = njs_vm_start(nvm);
+            } while (--repeat != 0);
 
             if (njs_vm_retval_string(nvm, &s) != NJS_OK) {
                 njs_printf("njs_vm_retval_string() failed\n");
@@ -14844,6 +14869,8 @@ main(int argc, char **argv)
 
     njs_memzero(&stat, sizeof(njs_stat_t));
 
+    opts.repeat = 1;
+
     ret = njs_unit_test(njs_test, njs_nitems(njs_test), "script tests",
                         &opts, &stat);
     if (ret != NJS_OK) {
@@ -14878,6 +14905,15 @@ main(int argc, char **argv)
         return ret;
     }
 
+    opts.module = 0;
+    opts.repeat = 128;
+
+    ret = njs_unit_test(njs_shared_test, njs_nitems(njs_shared_test),
+                        "shared tests", &opts, &stat);
+    if (ret != NJS_OK) {
+        return ret;
+    }
+
     njs_printf("TOTAL: %s [%ui/%ui]\n", stat.failed ? "FAILED" : "PASSED",
                stat.passed, stat.passed + stat.failed);