From: Dmitry Volyntsev <xeioex@nginx.com>
Date: Thu, 15 Aug 2019 16:22:01 +0000 (+0300)
Subject: Fixed module importing using require().
X-Git-Tag: 0.3.5~1
X-Git-Url: http://www.kaiwu.me/postgresql/commit/?a=commitdiff_plain;h=92b8a81bd32d229d047b8ed03ad6a93bfc3f4a50;p=njs.git

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.
---

diff --git a/src/njs_module.c b/src/njs_module.c
index 552770a3..750c3ba3 100644
--- a/src/njs_module.c
+++ b/src/njs_module.c
@@ -503,9 +503,10 @@ njs_module_insert(njs_vm_t *vm, njs_module_t *module)
 
 
 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_value_t *args,
 
     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 --git a/src/test/njs_unit_test.c b/src/test/njs_unit_test.c
index aadb8f33..c28a7d84 100644
--- a/src/test/njs_unit_test.c
+++ b/src/test/njs_unit_test.c
@@ -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[], size_t num, const char *name,
     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[], size_t num, const char *name,
                 njs_disassembler(vm);
             }
 
-            nvm = njs_vm_clone(vm, NULL);
-            if (nvm == NULL) {
-                njs_printf("njs_vm_clone() failed\n");
-                goto done;
-            }
+            repeat = opts->repeat;
 
-            ret = njs_vm_start(nvm);
+            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);