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 input type checking regression #5244

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 15, 2016

Before b212be0, input types were not checked in some path functions and the inputs were passed directly to regexp.exec() which implicitly converts its argument to a string.

This commit both removes the type checking added in b212be0 and adds string coercion for those functions.

/cc @thealphanerd

Before b212be0, input types were not checked in some path functions
and the inputs were passed directly to `regexp.exec()` which
implicitly converts its argument to a string.

This commit both removes the type checking added in b212be0 and
adds string coercion for those functions.
@mscdex mscdex added the path Issues and PRs related to the path subsystem. label Feb 15, 2016
@MylesBorins
Copy link
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/1671/
citgm: https://ci.nodejs.org/job/thealphanerd-smoker/76/

citgm + CI are green (expected failures on ppc)

LGTM

@MylesBorins
Copy link
Contributor

@mscdex what's the semver on this? Would it bring #5123 back down from Major?

@mscdex
Copy link
Contributor Author

mscdex commented Feb 15, 2016

@thealphanerd Yes, it should.

@rvagg
Copy link
Member

rvagg commented Feb 15, 2016

yay for smoke testing! yay for more tests! this is a great outcome of more thorough processes.

@rvagg
Copy link
Member

rvagg commented Feb 15, 2016

oh, and fix lgtm, 🚢

@MylesBorins
Copy link
Contributor

@mscdex so it looks like this patch is not applying cleanly to v5.x (although it is for master). Would you be able to take a peak?

@jasnell
Copy link
Member

jasnell commented Feb 15, 2016

LGTM

@@ -698,7 +698,8 @@ const win32 = {


dirname: function dirname(path) {
assertPath(path);
if (typeof path !== 'string')
Copy link
Contributor

Choose a reason for hiding this comment

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

We can skip this check and simply do path = '' + path, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably doesn't matter either way. I just did it this way in case v8 doesn't/can't optimize it by not creating a new string if path is already a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking out loud here. Will path = path.toString() be better then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's effectively the same as path = '' + path but less safe (e.g. won't work on null and other values).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah. Totally forgot about them.

@thefourtheye
Copy link
Contributor

LGTM

mscdex added a commit that referenced this pull request Feb 17, 2016
Before b212be0, input types were not checked in some path functions
and the inputs were passed directly to `regexp.exec()` which
implicitly converts its argument to a string.

This commit both removes the type checking added in b212be0 and
adds string coercion for those functions.

PR-URL: #5244
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@mscdex
Copy link
Contributor Author

mscdex commented Feb 17, 2016

Landed in 9209bf6.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 17, 2016

@rvagg Do you want me to submit a separate PR for inclusion into v5.x or will it get cherry-picked at some point (and should be tagged with land-on-v5.x despite being closed now)?

@mscdex mscdex deleted the path-fix-regression branch February 17, 2016 19:26
@rvagg
Copy link
Member

rvagg commented Feb 18, 2016

@mscdex the cherry-picking process for v5.x is opt-out rather than opt-in like v4.x so this'll come across in my next update (within the next couple of ours), no need to take action on your part except for PRs that shouldn't go in to v5.x.

Thanks for fixing!

rvagg pushed a commit that referenced this pull request Feb 18, 2016
Before b212be0, input types were not checked in some path functions
and the inputs were passed directly to `regexp.exec()` which
implicitly converts its argument to a string.

This commit both removes the type checking added in b212be0 and
adds string coercion for those functions.

PR-URL: #5244
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
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.

5 participants