module: fix wrong condition in early return logic for node_module path #6670

hefangshi · 2016-05-10T08:30:29Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

module

Description of change

the p < nmLen condition will fail when a module's name is end with
node_modules like foo_node_modules. The old logic will miss the
foo_node_modules/node_modules in node_modules paths.

TL;TR, a module named like foo_node_modules can't require any module
in his node_modules folder.

Here is a demo which compared the new nodeModulePaths with the old one.

const path = require('path');
var nmChars = [115, 101, 108, 117, 100, 111, 109, 95, 101, 100, 111, 110];
var nmLen = nmChars.length;

function nodeModulePaths(from) {
    // guarantee that 'from' is absolute.
    from = path.resolve(from);
    // Return early not only to avoid unnecessary work, but to *avoid* returning
    // an array of two items for a root: [ '//node_modules', '/node_modules' ]
    if (from === '/')
        return ['/node_modules'];

    // note: this approach *only* works when the path is guaranteed
    // to be absolute.  Doing a fully-edge-case-correct path.split
    // that works on both Windows and Posix is non-trivial.
    const paths = [];
    var p = 0;
    var last = from.length;
    for (var i = from.length - 1; i >= 0; --i) {
        const code = from.charCodeAt(i);
        if (code === 47 /*/*/ ) {
            if (p !== nmLen)
                paths.push(from.slice(0, last) + '/node_modules');
            last = i;
            p = 0;
        }
        else if (p !== -1 && p < nmLen) {
            if (nmChars[p] === code) {
                ++p;
            }
            else {
                p = -1;
            }
        }
    }

    return paths;
};

const splitRe = process.platform === 'win32' ? /[\/\\]/ : /\//;
nodeModulePathsOld = function (from) {
    // guarantee that 'from' is absolute.
    from = path.resolve(from);
    // note: this approach *only* works when the path is guaranteed
    // to be absolute.  Doing a fully-edge-case-correct path.split
    // that works on both Windows and Posix is non-trivial.
    var paths = [];
    var parts = from.split(splitRe);
    for (var tip = parts.length - 1; tip >= 0; tip--) {
        // don't search in .../node_modules/node_modules
        if (parts[tip] === 'node_modules') continue;
        var dir = parts.slice(0, tip + 1).concat('node_modules').join(path.sep);
        paths.push(dir);
    }
    return paths;
}

console.log('new:', nodeModulePaths('/Users/hefangshi/test/foo_node_modules'));
console.log('old:', nodeModulePathsOld('/Users/hefangshi/test/foo_node_modules'));

Output:

new: [ '/Users/hefangshi/test/node_modules',
  '/Users/hefangshi/node_modules',
  '/Users/node_modules' ]
old: [ '/Users/hefangshi/test/foo_node_modules/node_modules',
  '/Users/hefangshi/test/node_modules',
  '/Users/hefangshi/node_modules',
  '/Users/node_modules',
  '/node_modules' ]

The main reason is the p < nmLen condition will ignore foo_node_modules and simply
think it should be /node_modules, and won't search /Users/hefangshi/test/foo_node_modules/node_modules for dependency.

I already tested in mac and windows, but the testcases were not very full edge tested.

@mscdex please have a look on it.

mscdex · 2016-05-10T14:32:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/2561/

Fishrock123 · 2016-05-10T15:02:31Z

Aside: Why do we need/have a land-on-v6.x label since it's not in LTS?

Fishrock123 · 2016-05-10T15:03:35Z

Test seems to fail on windows:

not ok 144 test-module-nodemodulepaths.js
# 
# assert.js:90
# throw new assert.AssertionError({
# ^
# AssertionError: [ 'C:\\Users\\Rocko Artischocko\\node_stuff\\foo\\node_modules',
# 'C:\\Users\\Rocko Artischocko\\node_stuff\\node_modules',
# ' deepStrictEqual [ 'C:\\Users\\Rocko Artischocko\\node_stuff\\foo\\node_modules',
# 'C:\\Users\\Rocko Artischocko\\node_stuff\\node_modules',
# '
# at platformCases.forEach (C:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\parallel\test-module-nodemodulepaths.js:52:10)
# at Array.forEach (native)
# at Object.<anonymous> (C:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\parallel\test-module-nodemodulepaths.js:50:15)
# at Module._compile (module.js:539:32)
# at Object.Module._extensions..js (module.js:548:10)
# at Module.load (module.js:456:32)
# at tryModuleLoad (module.js:415:12)
# at Function.Module._load (module.js:407:3)
# at Function.Module.runMain (module.js:573:10)
# at startup (node.js:160:18)

hefangshi · 2016-05-10T17:39:31Z

My fault, it's another different between regex version and manual parsers.

The node_modules in root path like C:\\node_modules or /node_modules was missing.

hefangshi · 2016-05-11T10:36:21Z

Need another CI, @Fishrock123 @mscdex

mscdex · 2016-05-11T13:06:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/2579/

mhart · 2016-05-11T14:15:31Z

This also fixes #6679 FWIW

Fishrock123 · 2016-05-11T14:27:25Z

@mscdex Looks like the linter is failing again, does that just mean this need rebasing?

Fishrock123 · 2016-05-11T14:29:11Z

Looks like the windows tests are still failing: https://ci.nodejs.org/job/node-test-binary-windows/2075/

not ok 145 test-module-nodemodulepaths.js
# 
# assert.js:90
# throw new assert.AssertionError({
# ^
# AssertionError: [ 'C:\\node_modules' ] deepStrictEqual [ 'C:\\\\node_modules', 'C:\\node_modules' ]
# at platformCases.forEach (C:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2013\label\win2008r2\test\parallel\test-module-nodemodulepaths.js:74:10)
# at Array.forEach (native)
# at Object.<anonymous> (C:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2013\label\win2008r2\test\parallel\test-module-nodemodulepaths.js:72:15)
# at Module._compile (module.js:543:32)
# at Object.Module._extensions..js (module.js:552:10)
# at Module.load (module.js:460:32)
# at tryModuleLoad (module.js:419:12)
# at Function.Module._load (module.js:411:3)
# at Function.Module.runMain (module.js:577:10)
# at startup (node.js:160:18)

mscdex · 2016-05-11T14:38:41Z

@Fishrock123 If the linter is failing it means there is a lint problem that needs to be fixed. The output doesn't display because it's written to a tap file. The linter job doesn't have a tap display page yet.

Fishrock123 · 2016-05-11T14:48:18Z

Oh.. @hefangshi please run make jslint. :)

hefangshi · 2016-05-11T17:27:28Z

@Fishrock123 Done, and I used --amend, so the commit is 453d066767f328dc73d6987cc8d30adcad828b79 . I'm sorry that I didn't have test environment for both linux and windows yesterday, so the code was not properly tested. Now the testcases are all passed in my centos6.3 and win10.

mscdex · 2016-05-11T22:06:25Z

~~CI: https://ci.nodejs.org/job/node-test-pull-request/2595/~~

EDIT: it looks like there's something wrong with CI at the moment

mscdex · 2016-05-11T22:10:18Z

lib/module.js

+
+ // return root node_modules when path is 'D:\\'.
+ // path.resolve will make sure from.length >=3 in Windows.
+ if (from[from.length - 1] === '\\' && from[from.length - 2] === ':')


What about prepending a from.length >= 2 check here and then changing the string equality checks to .charCodeAt()-based checks? The length check will help prevent a deopt when accessing a non-existent index and the character comparison changes should improve performance a little bit.

How about

if (from.length <= 2 || (from.charCodeAt(from.length - 1) === 92 /*\*/ && from.charCodeAt(from.length - 2) === 58 /*:*/)) return [from + 'node_modules'];

I'm not sure that if from.length is always >=3 (handled by path.resolve on windows), so the from.length - 1 and from.length - 2 is always exist, will v8 still do deopt on it?

Actually on second thought I think it is probably safe to assume from.length >= 2 on Windows. So just the charCodeAt() changes should be sufficient.

Done, should I squash the commits?

It wouldn't hurt

hefangshi · 2016-05-13T05:38:28Z

CI: https://ci.nodejs.org/job/node-test-commit/3315/

Looks like have an irrelevant failure in windows.

hefangshi · 2016-05-17T06:14:04Z

@Fishrock123 will this patch land in 6.1.1? I think this bug should be fixed ASAP.

Fishrock123 · 2016-05-17T15:34:21Z

Sorry, I don't really understand this well enough to review. :(

v6.2.0 is probably coming out today but this will likely not be in a release until next week as it has not landed yet.

jasnell · 2016-05-17T15:36:36Z

@bnoordhuis ... can you take a look at this one.

MylesBorins · 2016-05-20T17:39:28Z

@mscdex did you have any more thoughts?

mscdex · 2016-05-20T18:12:42Z

@thealphanerd I'm not familiar enough with the module resolution logic algorithms themselves (much less the Windows side of things) to really comment in detail, so all I can say is to check that CI is ok with it.

hefangshi · 2016-05-21T11:33:58Z

@mscdex But this regression was introduced by you in this commit hefangshi@ae18bbe

That's the reason I ping you at the beginning.

evanlucas · 2016-05-24T11:42:34Z

Aside: Why do we need/have a land-on-v6.x label since it's not in LTS?

Sorry @Fishrock123, I believe I added that during the v6.2.0 release to make it easier to cherry-pick. Some commits were dependent on semver-major changes.

Fishrock123 · 2016-05-24T17:22:16Z

I removed that label, the proper one for what you are describing is dont-land-on-v6.x,

Fishrock123 · 2016-05-25T16:00:29Z

citgm run: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/282/

hefangshi · 2016-06-04T15:36:08Z

So, any idea? Are we just ignore such a basic bug here? I didn't mean to push this pr land, but I think at least this bug should be fixed.

MylesBorins · 2016-06-06T22:32:58Z

citgm failures are unrelated

/cc @nodejs/ctc thoughts?

Fishrock123 · 2016-06-17T20:40:08Z

I think we should land this. I'm having a hard time understanding it enough to make a clear judgement but the test cases are much more thorough so I don't think that should be an issue.

@nodejs/collaborators Gona land this next week unless someone objects.

hefangshi · 2016-07-08T08:32:06Z

@Fishrock123 any progress?

rmg · 2016-07-08T18:09:50Z

This looks pretty reasonable to me, especially if the added tests fail without the corresponding fix.

@hefangshi since this hasn't landed yet, would you be able to add in one more test case? I think the WIN and POSIX cases should both include something like /usr/lib/node_modules/npm/node_modules/node-gyp/node_modules/glob/node_modules/minimatch (and whatever the Windows equivalent is of that globally installed npm path). The module path code is a little hairy and should have something "real world" in it that should never break.

hefangshi · 2016-07-11T06:18:27Z

@rmg Done, added a global npm node_modules paths testcases.

evanlucas · 2016-08-01T01:17:47Z

Running CI again: https://ci.nodejs.org/job/node-test-pull-request/3474/

I just ran into this myself. LGTM

Trott · 2016-08-01T03:48:10Z

Only failure in CI is a known flaky.

cjihrig · 2016-08-01T13:31:00Z

lib/module.js

 const paths = [];
 var p = 0;
 var last = from.length;
 for (var i = from.length - 1; i >= 0; --i) {
 const code = from.charCodeAt(i);
- if (code === 92/*\*/ || code === 47/*/*/) {
+ // use colon as a split to add driver root node_modules


These comments seem to only apply to the colon, added in this PR. Anyone stumbling on this in the future might be confused by it. Could you expand the comment please. Also, please start comments with a capital letter, and terminate with a period.

Done, feel free to give me some advise about the comments since I don't know English too well.

evanlucas · 2016-08-02T11:06:16Z

@hefangshi can you format the commit message to follow https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit?

Also, we try to avoid using gendered pronouns (like "his"). The last line in the first commit could be updated to something like:

TL;TR, a module with a name that ends with node_modules cannot require 
any other modules in the node_modules folder

The `p < nmLen` condition will fail when a module's name is end with `node_modules` like `foo_node_modules`. The old logic will miss the `foo_node_modules/node_modules` in node_modules paths. TL;TR, a module named like `foo_node_modules` can't require any module in the node_modules folder.

Manual parsers didn't handle the root path on both platform, so push driver root node_modules when colon is matched in windows to avoid parse dirver name and direct push `/node_modules` into paths in posix.

hefangshi · 2016-08-02T13:53:07Z

@evanlucas Thanks for your advice, the commit message issue was fixed.

Add a real world global node_modules path test case come from npm's dependency to make test more effective.

evanlucas · 2016-08-08T23:24:41Z

alright running CI one more time and landing provided it is green https://ci.nodejs.org/job/node-test-pull-request/3581/

The `p < nmLen` condition will fail when a module's name is end with `node_modules` like `foo_node_modules`. The old logic will miss the `foo_node_modules/node_modules` in node_modules paths. TL;TR, a module named like `foo_node_modules` can't require any module in the node_modules folder. Fixes: #6679 PR-URL: #6670 Reviewed-By: Evan Lucas <evanlucas@me.com>

evanlucas · 2016-08-09T10:58:36Z

Landed in 55852e1. Thanks!

The `p < nmLen` condition will fail when a module's name is end with `node_modules` like `foo_node_modules`. The old logic will miss the `foo_node_modules/node_modules` in node_modules paths. TL;TR, a module named like `foo_node_modules` can't require any module in the node_modules folder. Fixes: #6679 PR-URL: #6670 Reviewed-By: Evan Lucas <evanlucas@me.com>

MylesBorins · 2016-09-30T18:14:14Z

I've marked this as don't land. Please let me know if I was mistaken and it should be backported /cc @cjihrig

cjihrig · 2016-09-30T18:17:59Z

I'm fine with leaving this out of v4, but ping @evanlucas who is more familiar with this commit.

mscdex added the module Issues and PRs related to the module subsystem. label May 10, 2016

mscdex added the land-on-v6.x label May 10, 2016

hefangshi force-pushed the fix-node_modules-paths branch 2 times, most recently from d8e826e to a880856 Compare May 10, 2016 18:54

targos mentioned this pull request May 11, 2016

require has issues when run from / #6679

Closed

hefangshi force-pushed the fix-node_modules-paths branch from a880856 to 453d066 Compare May 11, 2016 17:21

mscdex reviewed May 11, 2016
View reviewed changes

hefangshi force-pushed the fix-node_modules-paths branch from 453d066 to 8e57a4e Compare May 12, 2016 05:55

Fishrock123 removed the land-on-v6.x label May 14, 2016

cjihrig reviewed Aug 1, 2016
View reviewed changes

hefangshi force-pushed the fix-node_modules-paths branch from bef38b8 to 88bbb00 Compare August 2, 2016 04:44

hefangshi and others added 2 commits August 2, 2016 21:46

module: fix root path node_modules missing issue

145f276

Manual parsers didn't handle the root path on both platform, so push driver root node_modules when colon is matched in windows to avoid parse dirver name and direct push `/node_modules` into paths in posix.

hefangshi force-pushed the fix-node_modules-paths branch from 88bbb00 to b589023 Compare August 2, 2016 13:51

test: add real world case for node_modules paths

ff893e4

Add a real world global node_modules path test case come from npm's dependency to make test more effective.

hefangshi force-pushed the fix-node_modules-paths branch from b589023 to ff893e4 Compare August 2, 2016 13:57

evanlucas closed this Aug 9, 2016

cjihrig mentioned this pull request Aug 10, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

MylesBorins added the dont-land-on-v4.x label Sep 30, 2016

module: fix wrong condition in early return logic for node_module path #6670

module: fix wrong condition in early return logic for node_module path #6670

Conversation

hefangshi commented May 10, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

mscdex commented May 10, 2016

Fishrock123 commented May 10, 2016 • edited Loading

Fishrock123 commented May 10, 2016

hefangshi commented May 10, 2016 • edited Loading

hefangshi commented May 11, 2016

mscdex commented May 11, 2016

mhart commented May 11, 2016

Fishrock123 commented May 11, 2016

Fishrock123 commented May 11, 2016

mscdex commented May 11, 2016 • edited Loading

Fishrock123 commented May 11, 2016

hefangshi commented May 11, 2016 • edited Loading

mscdex commented May 11, 2016 • edited Loading

mscdex May 11, 2016

Choose a reason for hiding this comment

hefangshi May 12, 2016 • edited Loading

Choose a reason for hiding this comment

mscdex May 12, 2016

Choose a reason for hiding this comment

hefangshi May 12, 2016 • edited Loading

Choose a reason for hiding this comment

mscdex May 12, 2016

Choose a reason for hiding this comment

hefangshi commented May 13, 2016

hefangshi commented May 17, 2016

Fishrock123 commented May 17, 2016

jasnell commented May 17, 2016

MylesBorins commented May 20, 2016

mscdex commented May 20, 2016

hefangshi commented May 21, 2016

evanlucas commented May 24, 2016

Fishrock123 commented May 24, 2016

Fishrock123 commented May 25, 2016

hefangshi commented Jun 4, 2016

MylesBorins commented Jun 6, 2016

Fishrock123 commented Jun 17, 2016

hefangshi commented Jul 8, 2016

rmg commented Jul 8, 2016

hefangshi commented Jul 11, 2016

evanlucas commented Aug 1, 2016

Trott commented Aug 1, 2016

cjihrig Aug 1, 2016

Choose a reason for hiding this comment

hefangshi Aug 2, 2016

Choose a reason for hiding this comment

evanlucas commented Aug 2, 2016

hefangshi commented Aug 2, 2016

evanlucas commented Aug 8, 2016

evanlucas commented Aug 9, 2016

MylesBorins commented Sep 30, 2016

cjihrig commented Sep 30, 2016

hefangshi commented May 10, 2016 •

edited

Loading

Fishrock123 commented May 10, 2016 •

edited

Loading

hefangshi commented May 10, 2016 •

edited

Loading

mscdex commented May 11, 2016 •

edited

Loading

hefangshi commented May 11, 2016 •

edited

Loading

mscdex commented May 11, 2016 •

edited

Loading

hefangshi May 12, 2016 •

edited

Loading

hefangshi May 12, 2016 •

edited

Loading