path: fix win32 relative() when "to" is a prefix by omsmith · Pull Request #5456 · nodejs/node · GitHub

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

@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

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.

nit: should be from=

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'

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);
     }
   },

That fixes it, will get the PR updated

Ping @mscdex

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

Good call

Added

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

CI is all green. LGTM

LGTM

@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

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

@silverwind done. only conflict was in path.posix.resolverelative test cases. make test passes locally

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 <mscdex@mscdex.net> Reviewed-By: Roman Reiss <me@silverwind.io>

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 <mscdex@mscdex.net> Reviewed-By: Roman Reiss <me@silverwind.io>

adds posix test cases for paths similar to those that caused #5447 PR-URL: #5456 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Roman Reiss <me@silverwind.io>

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 <mscdex@mscdex.net> Reviewed-By: Roman Reiss <me@silverwind.io>

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 <mscdex@mscdex.net> Reviewed-By: Roman Reiss <me@silverwind.io>

adds posix test cases for paths similar to those that caused #5447 PR-URL: #5456 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Roman Reiss <me@silverwind.io>

@mscdex ... is this relevant for v4?

@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

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

@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?

@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

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.

@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

rvagg pushed a commit that referenced this pull request Feb 28, 2016

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

c83725c

adds posix test cases for paths similar to those that caused #5447 PR-URL: #5456 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Roman Reiss <me@silverwind.io>

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

path: fix win32 relative() when "to" is a prefix by omsmith · Pull Request #5456 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

path: fix win32 relative() when "to" is a prefix#5456omsmith wants to merge 3 commits intonodejs:masternodejs/node:masterfrom omsmith:path-win32-fix-relativeomsmith/node:path-win32-fix-relativeCopy head branch name to clipboard

Conversation

omsmith commented Feb 26, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

omsmith commented Feb 26, 2016

Uh oh!

mscdex commented Feb 26, 2016

Uh oh!

mscdex Feb 26, 2016

Choose a reason for hiding this comment

Uh oh!

omsmith commented Feb 26, 2016

Uh oh!

mscdex commented Feb 26, 2016

Uh oh!

omsmith commented Feb 26, 2016

Uh oh!

omsmith commented Feb 26, 2016

Uh oh!

mscdex Feb 26, 2016

Choose a reason for hiding this comment

Uh oh!

omsmith Feb 26, 2016

Choose a reason for hiding this comment

Uh oh!

omsmith Feb 26, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex commented Feb 26, 2016

Uh oh!

mscdex commented Feb 26, 2016

Uh oh!

silverwind commented Feb 27, 2016

Uh oh!

silverwind commented Feb 27, 2016

Uh oh!

omsmith commented Feb 27, 2016

Uh oh!

silverwind commented Feb 27, 2016

Uh oh!

silverwind commented Feb 27, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

mscdex commented Mar 2, 2016

Uh oh!

MylesBorins commented Mar 10, 2016

Uh oh!

rvagg commented Mar 10, 2016

Uh oh!

MylesBorins commented Mar 10, 2016

Uh oh!

jasnell commented Mar 11, 2016

Uh oh!

jasnell commented Mar 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

path: fix win32 relative() when "to" is a prefix#5456
omsmith wants to merge 3 commits into
nodejs:masterfrom
omsmith:path-win32-fix-relative