path: fix win32 relative() when "to" is a prefix #5456

omsmith · 2016-02-26T19:51:05Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
- make -j8 test
- vcbuild test nosign
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

path

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

Description of change

Please provide a description of the change here.

when the basename of "to" was a prefix of the basename of "from" win32
relative() would miss including it in the result

Fixes #5447

R=@mscdex

omsmith · 2016-02-26T19:51:55Z

@mscdex I'm not sure about UNC paths, which the tests are currently lacking

Edit: yeah, my change appears to break ['\\\\foo\\bar', '\\\\foo\\bar\\baz', 'baz'], will have to investigate

mscdex · 2016-02-26T20:06:39Z

UNC paths should work just as well, it would probably be good to add at least a test for that too.

Also it might be good to add a test with reversed inputs: ['C:\\foo\\bar\\baz', 'C:\\foo\\bar\\baz-quux', '..\\baz-quux'] and to add those two tests to the posix tests (using posix directory structure) as well.

mscdex · 2016-02-26T20:07:14Z

lib/path.js

+        if (fromLen > length) {
+          if (from.charCodeAt(fromStart + i) === 92/*\*/) {
+            // We get here if `to` is the exact base path for `from`.
+            // For example: from:='C:\\foo\\bar'; to='C:\\foo'


nit: should be from=

omsmith · 2016-02-26T20:24:54Z

Discrepancy in path.win32.normalize which may be effecting some things?

EDIT: Also path.win32.resolve, which is what relative actually calls

omsmith bit-buster | Fri Feb 26 at 15:24:10
/home/omsmith ~ node -v
v5.7.0
omsmith bit-buster | Fri Feb 26 at 15:24:16
/home/omsmith ~ node
> path.win32.normalize('\\\\foo\\bar')
'\\\\foo\\bar\\'
> path.win32.normalize('\\\\foo\\bar\\baz')
'\\\\foo\\bar\\baz'

mscdex · 2016-02-26T20:38:33Z

Yeah, I think for relative() we can safely trim any slashes at the end after normalization. This should fix that:

--- /tmp/H9mE2i_path.js
+++ /home/mscdex/git/node/lib/path.js
@@ -586,6 +586,10 @@
         break;
     }
     var fromEnd = from.length;
+    for (; fromEnd - 1 > fromStart; --fromEnd) {
+      if (from.charCodeAt(fromEnd - 1) !== 92/*\*/)
+        break;
+    }
     var fromLen = (fromEnd - fromStart);

     // Trim any leading backslashes
@@ -595,6 +599,10 @@
         break;
     }
     var toEnd = to.length;
+    for (; toEnd - 1 > toStart; --toEnd) {
+      if (to.charCodeAt(toEnd - 1) !== 92/*\*/)
+        break;
+    }
     var toLen = (toEnd - toStart);

     // Compare paths to find the longest common path from root
@@ -648,12 +656,12 @@
     // Lastly, append the rest of the destination (`to`) path that comes after
     // the common path parts
     if (out.length > 0)
-      return out + toOrig.slice(toStart + lastCommonSep);
+      return out + toOrig.slice(toStart + lastCommonSep, toEnd);
     else {
       toStart += lastCommonSep;
       if (toOrig.charCodeAt(toStart) === 92/*\*/)
         ++toStart;
-      return toOrig.slice(toStart);
+      return toOrig.slice(toStart, toEnd);
     }
   },

omsmith · 2016-02-26T20:47:18Z

That fixes it, will get the PR updated

omsmith · 2016-02-26T21:05:29Z

Ping @mscdex

mscdex · 2016-02-26T21:06:36Z

lib/path.js

@@ -586,6 +586,10 @@ const win32 = {
        break;
    }
    var fromEnd = from.length;
+    for (; fromEnd - 1 > fromStart; --fromEnd) {


I forgot to add a comment here and for the other loop below for completeness. If you could, just add something like "Trim trailing slashes (applicable to UNC paths only)".

mscdex · 2016-02-26T21:23:29Z

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

mscdex · 2016-02-26T21:50:41Z

CI is all green. LGTM

silverwind · 2016-02-27T09:30:26Z

LGTM

silverwind · 2016-02-27T13:44:48Z

@omsmith need a rebase!

when the basename of "to" was a prefix of the basename of "from" win32 relative() would miss including it in the result Fixes nodejs#5447

@mscdex

win32 normalize() will output a trailing '\' for some UNC paths. trim them before processing Change by @mscdex Add basic UNC path tests to win32 relative()

adds posix test cases for paths similar to those that caused nodejs#5447

omsmith · 2016-02-27T14:21:55Z

@silverwind done. only conflict was in path.posix.~~resolve~~relative test cases. make test passes locally

silverwind · 2016-02-27T15:34:49Z

One more CI: https://ci.nodejs.org/job/node-test-pull-request/1767/

when the basename of "to" was a prefix of the basename of "from" win32 relative() would miss including it in the result Fixes: #5447 PR-URL: #5456 Reviewed-By: Brian White <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

@mscdex

win32 normalize() will output a trailing '\' for some UNC paths. trim them before processing Change by @mscdex Add basic UNC path tests to win32 relative() PR-URL: #5456 Reviewed-By: Brian White <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

adds posix test cases for paths similar to those that caused #5447 PR-URL: #5456 Reviewed-By: Brian White <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

silverwind · 2016-02-27T16:32:17Z

Thanks! Landed in b33879d .. 7fc6645.

when the basename of "to" was a prefix of the basename of "from" win32 relative() would miss including it in the result Fixes: #5447 PR-URL: #5456 Reviewed-By: Brian White <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

@mscdex

win32 normalize() will output a trailing '\' for some UNC paths. trim them before processing Change by @mscdex Add basic UNC path tests to win32 relative() PR-URL: #5456 Reviewed-By: Brian White <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

adds posix test cases for paths similar to those that caused #5447 PR-URL: #5456 Reviewed-By: Brian White <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

jasnell · 2016-03-02T16:25:00Z

@mscdex ... is this relevant for v4?

mscdex · 2016-03-02T19:24:05Z

@jasnell This is only relevant if my original path perf PR is landed.

Notable changes: * governance: The Core Technical Committee (CTC) added four new members to help guide Node.js core development: Evan Lucas, Rich Trott, Ali Ijaz Sheikh and Сковорода Никита Андреевич (Nikita Skovoroda). * openssl: Upgrade from 1.0.2f to 1.0.2g (Ben Noordhuis) #5507 - Fix a double-free defect in parsing malformed DSA keys that may potentially be used for DoS or memory corruption attacks. It is likely to be very difficult to use this defect for a practical attack and is therefore considered low severity for Node.js users. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0705 - Fix a defect that can cause memory corruption in certain very rare cases relating to the internal `BN_hex2bn()` and `BN_dec2bn()` functions. It is believed that Node.js is not invoking the code paths that use these functions so practical attacks via Node.js using this defect are _unlikely_ to be possible. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0797 - Fix a defect that makes the CacheBleed Attack (https://ssrg.nicta.com.au/projects/TS/cachebleed/) possible. This defect enables attackers to execute side-channel attacks leading to the potential recovery of entire RSA private keys. It only affects the Intel Sandy Bridge (and possibly older) microarchitecture when using hyper-threading. Newer microarchitectures, including Haswell, are unaffected. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0702 * Fixed several regressions that appeared in v5.7.0: - path.relative(): - Output is no longer unnecessarily verbose (Brian White) #5389 - Resolving UNC paths on Windows now works correctly (Owen Smith) #5456 - Resolving paths with prefixes now works correctly from the root directory (Owen Smith) #5490 - url: Fixed an off-by-one error with `parse()` (Brian White) #5394 - dgram: Now correctly handles a default address case when offset and length are specified (Matteo Collina) #5407 PR-URL: #5464

Notable changes: * governance: The Core Technical Committee (CTC) added four new members to help guide Node.js core development: Evan Lucas, Rich Trott, Ali Ijaz Sheikh and Сковорода Никита Андреевич (Nikita Skovoroda). * openssl: Upgrade from 1.0.2f to 1.0.2g (Ben Noordhuis) nodejs#5507 - Fix a double-free defect in parsing malformed DSA keys that may potentially be used for DoS or memory corruption attacks. It is likely to be very difficult to use this defect for a practical attack and is therefore considered low severity for Node.js users. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0705 - Fix a defect that can cause memory corruption in certain very rare cases relating to the internal `BN_hex2bn()` and `BN_dec2bn()` functions. It is believed that Node.js is not invoking the code paths that use these functions so practical attacks via Node.js using this defect are _unlikely_ to be possible. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0797 - Fix a defect that makes the CacheBleed Attack (https://ssrg.nicta.com.au/projects/TS/cachebleed/) possible. This defect enables attackers to execute side-channel attacks leading to the potential recovery of entire RSA private keys. It only affects the Intel Sandy Bridge (and possibly older) microarchitecture when using hyper-threading. Newer microarchitectures, including Haswell, are unaffected. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0702 * Fixed several regressions that appeared in v5.7.0: - path.relative(): - Output is no longer unnecessarily verbose (Brian White) nodejs#5389 - Resolving UNC paths on Windows now works correctly (Owen Smith) nodejs#5456 - Resolving paths with prefixes now works correctly from the root directory (Owen Smith) nodejs#5490 - url: Fixed an off-by-one error with `parse()` (Brian White) nodejs#5394 - dgram: Now correctly handles a default address case when offset and length are specified (Matteo Collina) nodejs#5407 PR-URL: nodejs#5464

MylesBorins · 2016-03-10T21:36:45Z

Adding a dont-land-on-v4.x flag as I do not believe we will be landing the path fixes

rvagg · 2016-03-10T22:07:49Z

@thealphanerd if we end up landing the path perf fix on v4.x, how hard is it going to be to find all of these extras that are required on top of it? Should we be keeping a list somewhere so we don't land one without all of the others?

MylesBorins · 2016-03-10T22:18:38Z

@rvagg this was an exact problem that we dealt with in the past... I'm spacing on the regression, but we ended up missing one and that caused some nastiness to affect v4

This was my reasoning for setting the path changes to dont-land to begin with.

Perhaps we need to make a new tool + meta data (such as fixes-regresssion-from:) to make it easier to keep track of this stuff moving forward

jasnell · 2016-03-11T02:15:47Z

I'm thinking that we need a way of tracking Refs between PRs via additional metdata. Something like a Reqs: <PR-URL> and Reverts: <PR-URL> whenever someone knows that one PR depends on another one.

jasnell · 2016-03-11T02:16:28Z

@thealphanerd ... curious why the lts-watch was added and the dont-land was removed immediately after you added the dont land.

mscdex reviewed Feb 26, 2016
View reviewed changes

mscdex added the path Issues and PRs related to the path subsystem. label Feb 26, 2016

omsmith force-pushed the path-win32-fix-relative branch from 305c2cb to e52f14e Compare February 26, 2016 20:58

mscdex reviewed Feb 26, 2016
View reviewed changes

omsmith force-pushed the path-win32-fix-relative branch from e52f14e to f340451 Compare February 26, 2016 21:15

rvagg added this to the 5.7.1 milestone Feb 27, 2016

rvagg mentioned this pull request Feb 27, 2016

Release proposal: v5.7.1 #5464

Merged

5 tasks

omsmith added 3 commits February 27, 2016 09:19

path: fix win32 relative() when "to" is a prefix

c18d24e

when the basename of "to" was a prefix of the basename of "from" win32 relative() would miss including it in the result Fixes nodejs#5447

path: fix win32 relative() for UNC paths

80fd741

win32 normalize() will output a trailing '\' for some UNC paths. trim them before processing Change by @mscdex Add basic UNC path tests to win32 relative()

test: add test-cases for posix path.relative()

e3f3679

adds posix test cases for paths similar to those that caused nodejs#5447

omsmith force-pushed the path-win32-fix-relative branch from f340451 to e3f3679 Compare February 27, 2016 14:20

silverwind closed this Feb 27, 2016

omsmith deleted the path-win32-fix-relative branch March 4, 2016 14:20

MylesBorins mentioned this pull request Mar 10, 2016

Audit commits not found on v4.x #5647

Closed

MylesBorins added the dont-land-on-v4.x label Mar 10, 2016

MylesBorins added lts-watch-v4.x and removed dont-land-on-v4.x labels Mar 10, 2016

MylesBorins mentioned this pull request Mar 11, 2016

Track backporting Path optimizations nodejs/Release#87

Closed

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels May 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

path: fix win32 relative() when "to" is a prefix #5456

path: fix win32 relative() when "to" is a prefix #5456

omsmith commented Feb 26, 2016

omsmith commented Feb 26, 2016

mscdex commented Feb 26, 2016

mscdex Feb 26, 2016

omsmith commented Feb 26, 2016

mscdex commented Feb 26, 2016

omsmith commented Feb 26, 2016

omsmith commented Feb 26, 2016

mscdex Feb 26, 2016

omsmith Feb 26, 2016

omsmith Feb 26, 2016

mscdex commented Feb 26, 2016

mscdex commented Feb 26, 2016

silverwind commented Feb 27, 2016

silverwind commented Feb 27, 2016

omsmith commented Feb 27, 2016

silverwind commented Feb 27, 2016

silverwind commented Feb 27, 2016

jasnell commented Mar 2, 2016

mscdex commented Mar 2, 2016

MylesBorins commented Mar 10, 2016

rvagg commented Mar 10, 2016

MylesBorins commented Mar 10, 2016

jasnell commented Mar 11, 2016

jasnell commented Mar 11, 2016

path: fix win32 relative() when "to" is a prefix #5456

path: fix win32 relative() when "to" is a prefix #5456

Conversation

omsmith commented Feb 26, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

omsmith commented Feb 26, 2016

mscdex commented Feb 26, 2016

mscdex Feb 26, 2016

Choose a reason for hiding this comment

omsmith commented Feb 26, 2016

mscdex commented Feb 26, 2016

omsmith commented Feb 26, 2016

omsmith commented Feb 26, 2016

mscdex Feb 26, 2016

Choose a reason for hiding this comment

omsmith Feb 26, 2016

Choose a reason for hiding this comment

omsmith Feb 26, 2016

Choose a reason for hiding this comment

mscdex commented Feb 26, 2016

mscdex commented Feb 26, 2016

silverwind commented Feb 27, 2016

silverwind commented Feb 27, 2016

omsmith commented Feb 27, 2016

silverwind commented Feb 27, 2016

silverwind commented Feb 27, 2016

jasnell commented Mar 2, 2016

mscdex commented Mar 2, 2016

MylesBorins commented Mar 10, 2016

rvagg commented Mar 10, 2016

MylesBorins commented Mar 10, 2016

jasnell commented Mar 11, 2016

jasnell commented Mar 11, 2016