Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

path: fix path.normalize not correctly normalizing relative paths #17974

Closed
wants to merge 2 commits into from
Closed

path: fix path.normalize not correctly normalizing relative paths #17974

wants to merge 2 commits into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Jan 4, 2018 •
edited
Loading

After slicing, the lastSegmentLength should be calculated again.
It should be (res.length - 1 - lastSlashIndex), instead of value j.

Tests for it is also added.

Fixes: #17928

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

path

@nodejs-github-bot nodejs-github-bot added the path Issues and PRs related to the path subsystem. label Jan 4, 2018
@starkwang
Copy link
Contributor Author

cc @targos @gibfahn

@starkwang starkwang force-pushed the fix-path-normalize branch 2 times, most recently from 89d7fa1 to 4acf24e Compare January 4, 2018 05:28
@starkwang
Copy link
Contributor Author

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

@starkwang
path: fix path.normalize not correctly normalizing relative paths
b1aba4a 
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

Fixes: nodejs#17928
@starkwang starkwang added the confirmed-bug Issues with confirmed bugs. label Jan 6, 2018
gibfahn
gibfahn approved these changes Jan 7, 2018
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me as @targos is okay with it. Added tests look good.

@gibfahn gibfahn added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 7, 2018
@joyeecheung joyeecheung removed the confirmed-bug Issues with confirmed bugs. label Jan 9, 2018
@joyeecheung
Copy link
Member

Removed the confirmed-bug label since that one is usually applied to the issues, not the PRs fixing them

joyeecheung
joyeecheung reviewed Jan 9, 2018
View reviewed changes
lib/path.js Outdated
@@ -29,6 +29,17 @@ function assertPath(path) {
}
}

function findLastSlashIndex(path, isWin32) {
var i = path.length - 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic here can be simplified to Math.max(path.lastIndexOf(isWin32 ? '\\' : '/'), 0)? Or does iterating over the string with charCodeAt yield any performance benefit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is just inherited from the logic before. I've just removed it and used lastIndexOf.

@starkwang
Copy link
Contributor Author

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

@starkwang
Copy link
Contributor Author

Landed in 315d1f5

@starkwang starkwang closed this Jan 11, 2018
starkwang added a commit that referenced this pull request Jan 11, 2018
@starkwang
path: fix path.normalize for relative paths
315d1f5 
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

PR-URL: #17974
Fixes: #17928
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
evanlucas pushed a commit that referenced this pull request Jan 22, 2018
@starkwang @evanlucas
path: fix path.normalize for relative paths
a4a16ff 
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

PR-URL: #17974
Fixes: #17928
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
@starkwang @evanlucas
path: fix path.normalize for relative paths
470832f 
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

PR-URL: #17974
Fixes: #17928
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@evanlucas evanlucas mentioned this pull request Jan 30, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@starkwang @MylesBorins
path: fix path.normalize for relative paths
37071b8 
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

PR-URL: #17974
Fixes: #17928
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@starkwang @MylesBorins
path: fix path.normalize for relative paths
fea710e 
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

PR-URL: #17974
Fixes: #17928
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 27, 2018
Closed
@MylesBorins MylesBorins mentioned this pull request Feb 27, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@targos targos targos approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
path Issues and PRs related to the path subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

win32 path.normalize() not correctly normalizing relative paths containing ../ that advance above root
7 participants
@starkwang @joyeecheung @targos @gibfahn @MylesBorins @TimothyGu @nodejs-github-bot