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.

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

path: fix path.relative() for prefixes at device root #5490

Closed
wants to merge 1 commit into from

Conversation

omsmith
Copy link
Contributor

@omsmith omsmith commented Feb 29, 2016

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)?
  • 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)

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

  • path

Description of change

Please provide a description of the change here.

Fixes #5485

@omsmith
Copy link
Contributor Author

omsmith commented Feb 29, 2016

CC @mscdex @silverwind

return to.slice(toStart + i + 1);
} else if (i === 0) {
// We get here if `from` is the root
// For example: from='/'; to='/foo'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case for could be an if check before the loop, probably?

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly, yeah. Maybe run the benchmarks to see if it helps performance!

@mscdex mscdex added windows Issues and PRs related to the Windows platform. path Issues and PRs related to the path subsystem. and removed windows Issues and PRs related to the Windows platform. labels Feb 29, 2016
@silverwind silverwind added this to the 5.7.1 milestone Feb 29, 2016
@silverwind
Copy link
Contributor

@silverwind silverwind mentioned this pull request Feb 29, 2016
5 tasks
@silverwind
Copy link
Contributor

CI is green, LGTM. We can possibly optimize further in the future, but otherwise, I'd land this as is.

@silverwind
Copy link
Contributor

@mscdex wanna sign this off too?

@mscdex
Copy link
Contributor

mscdex commented Feb 29, 2016

LGTM

@omsmith
Copy link
Contributor Author

omsmith commented Feb 29, 2016

Tweaking later sounds good to me 👍

@silverwind
Copy link
Contributor

Thanks again! Landed in f296a7f.

@silverwind silverwind closed this Feb 29, 2016
silverwind pushed a commit that referenced this pull request Feb 29, 2016
Fixes #5485

PR-URL: #5490
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Brian White <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

@mscdex ... is this relevant for v4?

Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
Fixes #5485

PR-URL: #5490
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Brian White <[email protected]>
@mscdex
Copy link
Contributor

mscdex commented Mar 2, 2016

@jasnell It's only relevant if my original path perf PR is landed.

Fishrock123 added a commit that referenced this pull request Mar 2, 2016
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
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 2, 2016
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
@omsmith omsmith deleted the path-fix-relative-at-root branch March 4, 2016 14:20
@MylesBorins
Copy link
Contributor

adding don't land. If we decide to revisit the original Path changes then we can change this

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

hmm... @thealphanerd ... why did you pull the don't land back off and add the lts-watch back on?

@MylesBorins
Copy link
Contributor

@rvagg wanted to keep the path changes under LTS watch. There were regressions but now that those have primarily been found and fixed the idea was that we could eventually land them

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

Added the lts-agenda label. I'd like @nodejs/lts to weight in on the path changes.

@rvagg
Copy link
Member

rvagg commented Mar 14, 2016

Actually, I just wanted to make sure that if they got backported that they all stayed together since they are sprawling now. I'm -1 on backporting these at all though, just being prepared in case they end up going through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

path.relative parse wrong in windows
6 participants