From 2dfe28c639eb9cde451b6a13e2825e31a8f4acd9 Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Wed, 1 Oct 2014 13:25:09 +0300 Subject: [PATCH 1/4] Bug testcase for GH-48 --- .../test-bug-commonjs-relative-id.js | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 ecmascript-testcases/test-bug-commonjs-relative-id.js diff --git a/ecmascript-testcases/test-bug-commonjs-relative-id.js b/ecmascript-testcases/test-bug-commonjs-relative-id.js new file mode 100644 index 00000000..220f3c1c --- /dev/null +++ b/ecmascript-testcases/test-bug-commonjs-relative-id.js @@ -0,0 +1,56 @@ +/* + * Relative resolution bug in Duktape 0.12.0 + * + * https://github.com/svaarala/duktape/issues/48 + */ + +/*--- +{ + "custom": true +} +---*/ + +/*=== +Duktape.modSearch foo +Duktape.modSearch quux +Duktape.modSearch foo/bar +Duktape.modSearch foo/quux +===*/ + +function test() { + /* + * Relative resolution from 'foo' + */ + + Duktape.modSearch = function (id) { + print('Duktape.modSearch', id); + if (id === 'foo') { + return "var text = 'Hello world!'; // not visible outside the module\n" + + "var quux = require('./quux'); // loads quux\n" + + "exports.hello = function () { print(text); };"; + } + return ''; // return a fake empty module + }; + void require('foo'); + + /* + * Relative resolution from 'foo/bar' + */ + + Duktape.modSearch = function (id) { + print('Duktape.modSearch', id); + if (id === 'foo/bar') { + return "var text = 'Hello world!'; // not visible outside the module\n" + + "var quux = require('./quux'); // loads foo/quux\n" + + "exports.hello = function () { print(text); };"; + } + return ''; // return a fake empty module + }; + void require('foo/bar'); +} + +try { + test(); +} catch (e) { + print(e.stack || e); +} From 96c602a272b465c20591f1739987896159a062b7 Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Wed, 1 Oct 2014 13:27:29 +0300 Subject: [PATCH 2/4] Relative module ID fix (GH-48) --- src/duk_bi_global.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/duk_bi_global.c b/src/duk_bi_global.c index ba2eb260..b0abae5b 100644 --- a/src/duk_bi_global.c +++ b/src/duk_bi_global.c @@ -810,16 +810,20 @@ static void duk__bi_global_resolve_module_id(duk_context *ctx, const char *req_i * Set up the resolution input which is the requested ID directly * (if absolute or no current module path) or with current module * ID prepended (if relative and current module path exists). + * + * Suppose current module is 'foo/bar' and relative path is './quux'. + * The 'bar' component must be replaced so the initial input here is + * 'foo/bar/.././quux'. */ req_id_len = DUK_STRLEN(req_id); if (mod_id != NULL && req_id[0] == '.') { mod_id_len = DUK_STRLEN(mod_id); - if (mod_id_len + 1 + req_id_len + 1 >= sizeof(buf_in)) { + if (mod_id_len + 4 + req_id_len + 1 >= sizeof(buf_in)) { DUK_DD(DUK_DDPRINT("resolve error: current and requested module ID don't fit into resolve input buffer")); goto resolve_error; } - (void) DUK_SNPRINTF((char *) buf_in, sizeof(buf_in), "%s/%s", (const char *) mod_id, (const char *) req_id); + (void) DUK_SNPRINTF((char *) buf_in, sizeof(buf_in), "%s/../%s", (const char *) mod_id, (const char *) req_id); } else { if (req_id_len + 1 >= sizeof(buf_in)) { DUK_DD(DUK_DDPRINT("resolve error: requested module ID doesn't fit into resolve input buffer")); From 2cbeea5840243918bfe99554ee5210a8fcf5dd4a Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Wed, 1 Oct 2014 13:42:50 +0300 Subject: [PATCH 3/4] require() relative ID test case fixes These testcases were testing for incorrect behavior. --- .../test-commonjs-require-environment.js | 13 +++-- .../test-commonjs-require-resolution.js | 51 +++++++++++++------ .../test-commonjs-require-tweaked-id.js | 20 ++++---- 3 files changed, 54 insertions(+), 30 deletions(-) diff --git a/ecmascript-testcases/test-commonjs-require-environment.js b/ecmascript-testcases/test-commonjs-require-environment.js index 6e74a29b..efe3532e 100644 --- a/ecmascript-testcases/test-commonjs-require-environment.js +++ b/ecmascript-testcases/test-commonjs-require-environment.js @@ -9,8 +9,10 @@ /*=== basic bindings +modSearch: foo/bar +modSearch: foo/quux require: function false -module: object foo/bar false false false +module: object foo/quux false false false exports: object true ===*/ @@ -19,10 +21,11 @@ exports: object true var global_require = require; Duktape.modSearch = function (id) { - if (id === 'foo') { - return 'var mod = require("./bar");\n' - } + print('modSearch:', id); if (id === 'foo/bar') { + return 'var mod = require("./quux");\n' + } + if (id === 'foo/quux') { return 'var pd;\n' + 'print("require:", typeof require, require === global_require);\n' + 'pd = Object.getOwnPropertyDescriptor(module, "id");\n' + @@ -36,7 +39,7 @@ Duktape.modSearch = function (id) { print('basic bindings'); function bindingTest() { - var mod = require('foo'); + var mod = require('foo/bar'); // module.id must be a resolved absolute path so that it can be used // to require the correct module from any other module diff --git a/ecmascript-testcases/test-commonjs-require-resolution.js b/ecmascript-testcases/test-commonjs-require-resolution.js index ab9a68a5..82f0b5fd 100644 --- a/ecmascript-testcases/test-commonjs-require-resolution.js +++ b/ecmascript-testcases/test-commonjs-require-resolution.js @@ -61,8 +61,12 @@ global require: foo/mod1/. -> TypeError global require: foo/mod1/.. -> TypeError Duktape.modSearch baz Duktape.modSearch xxx -Duktape.modSearch baz/xxx -Duktape.modSearch baz/xxx/yyy +Duktape.modSearch xxy +Duktape.modSearch xxx/yyy +Duktape.modSearch quux/foo +Duktape.modSearch xxz +Duktape.modSearch quux/xxw +Duktape.modSearch quux/xxw/yyy Duktape.modSearch zzz Duktape.modSearch www ===*/ @@ -144,8 +148,25 @@ function basicResolutionTest() { print('Duktape.modSearch', id); if (id === 'baz') { return 'require("xxx");\n' + // absolute - 'require("./xxx");\n' + // relative - 'require("./xxx/yyy");\n' + // relative + 'require("./xxy");\n' + // relative + 'require("./xxx/yyy");\n' // relative + ; + } + return ''; // return a fake empty module + }; + + void require('baz'); + + /* + * Require from inside a module with a few more path components. + */ + + Duktape.modSearch = function (id) { + print('Duktape.modSearch', id); + if (id === 'quux/foo') { + return 'require("xxz");\n' + // absolute + 'require("./xxw");\n' + // relative + 'require("./xxw/yyy");\n' + // relative 'require("../zzz");\n' + // relative 'require("././../www");\n' ; @@ -153,7 +174,7 @@ function basicResolutionTest() { return ''; // return a fake empty module }; - void require('baz'); + void require('quux/foo'); } print('basic resolution'); @@ -260,14 +281,14 @@ Duktape.modSearch foo/bar 268: TypeError 269: TypeError 270: TypeError -230: foo/bar -231: foo/bar -232: foo/bar -233: foo/bar -234: foo/bar -235: foo/bar -236: foo/bar -237: foo/bar +230: bar +231: bar +232: bar +233: bar +234: bar +235: TypeError +236: TypeError +237: TypeError 238: TypeError 239: TypeError 240: TypeError @@ -304,8 +325,8 @@ Duktape.modSearch foo/bar ===*/ /* Test the current implementation limit for ID lengths. This also -* does some boundary value testing for ID length. -*/ + * does some boundary value testing for ID length. + */ function lengthTest() { var i; diff --git a/ecmascript-testcases/test-commonjs-require-tweaked-id.js b/ecmascript-testcases/test-commonjs-require-tweaked-id.js index fa94a0c6..c1ac357d 100644 --- a/ecmascript-testcases/test-commonjs-require-tweaked-id.js +++ b/ecmascript-testcases/test-commonjs-require-tweaked-id.js @@ -12,24 +12,24 @@ /*=== Duktape.modSearch quux quux: Error -Duktape.modSearch foo/bar/quux -./quux: Error Duktape.modSearch foo/quux +./quux: Error +Duktape.modSearch quux ../quux: Error -Duktape.modSearch testModule +Duktape.modSearch testModule/subModule Duktape.modSearch test/innerRequire testModule: Error ===*/ Duktape.modSearch = function (id) { print('Duktape.modSearch', id); - if (id == 'testModule') { + if (id == 'testModule/subModule') { // require.id is non-writable but is configurable, so its value must // be changed with Object.defineProperty(). return 'var mod;\n' + - 'exports.name = "testModule";\n' + - 'Object.defineProperty(require, "id", { value: "./././testModule/foo/../../test" });\n' + // same as 'test' but non-canonical - 'mod = require("./innerRequire");\n'; + 'exports.name = "testModule/subModule";\n' + + 'Object.defineProperty(require, "id", { value: "./././testModule/subModule/foo/../../../test/foo" });\n' + // same as 'test/foo' but non-canonical + 'mod = require("./innerRequire");\n'; // test/foo + ./innerRequire -> test/innerRequire } throw new Error('cannot find module'); } @@ -51,13 +51,13 @@ function tweakedIdentifierTest() { require.id = './foo//./bar'; // same as 'foo/bar' globalTest('quux'); - globalTest('./quux'); - globalTest('../quux'); + globalTest('./quux'); // foo/bar + ./quux -> foo/quux + globalTest('../quux'); // foo/bar + ../quux -> quux delete require.id; // Module 'id' not relative try { - mod = require('testModule'); + mod = require('testModule/subModule'); print('never here'); } catch (e) { print('testModule:', e.name); From f1b9cbf5db588499c061ec2f61481cb239c4c844 Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Wed, 1 Oct 2014 13:43:18 +0300 Subject: [PATCH 4/4] Release note: require() relative resolution --- RELEASES.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/RELEASES.rst b/RELEASES.rst index 49987303..2cfc99c0 100644 --- a/RELEASES.rst +++ b/RELEASES.rst @@ -550,6 +550,9 @@ Planned * Rename RELEASES.txt, AUTHORS.txt, and README.txt files to .rst suffix for better automatic formatting +* Fix require() resolution of relative module identifiers, which was off by + one component (see GH-48) + * Fix DUK_INVALID_INDEX define value, it used INT_MIN directly * Fix return value of Duktape.gc() to return true (instead of false) for