Skip to content

events: don't inherit from Object.prototype#6092

Merged
mscdex merged 1 commit intonodejs:masterfrom
mscdex:events-empty-prototype
Apr 19, 2016
Merged

events: don't inherit from Object.prototype#6092
mscdex merged 1 commit intonodejs:masterfrom
mscdex:events-empty-prototype

Conversation

@mscdex
Copy link
Contributor

@mscdex mscdex commented Apr 7, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)
  • events
Description of change

This commit safely allows event names that are named the same as properties that are ordinarily inherited from Object.prototype such as __proto__.

Fixes: #728

@mscdex mscdex added events Issues and PRs related to the events subsystem / EventEmitter. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 7, 2016
@mscdex mscdex force-pushed the events-empty-prototype branch from 8680b06 to 2cec8a1 Compare April 7, 2016 00:39
@mscdex
Copy link
Contributor Author

mscdex commented Apr 7, 2016

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

@mscdex
Copy link
Contributor Author

mscdex commented Apr 7, 2016

Also, there is no performance regression with this change 💃

@mscdex mscdex added this to the 6.0.0 milestone Apr 7, 2016
@thefourtheye
Copy link
Contributor

thefourtheye commented Apr 7, 2016

Can we make use of the test changes in #2350?

@mscdex
Copy link
Contributor Author

mscdex commented Apr 7, 2016

@thefourtheye The included tests should cover it I think

@evanlucas
Copy link
Contributor

evanlucas commented Apr 7, 2016

@mscdex will this have any affect on process since it sets _events to a regular object?

@vkurchatkin
Copy link
Contributor

vkurchatkin commented Apr 7, 2016

@evanlucas it makes sense to simply call EventEmitter on process

@mscdex mscdex force-pushed the events-empty-prototype branch from 2cec8a1 to 3711c52 Compare April 7, 2016 13:06
@mscdex
Copy link
Contributor Author

mscdex commented Apr 7, 2016

@evanlucas Fixed.

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

@evanlucas
Copy link
Contributor

evanlucas commented Apr 7, 2016

LGTM

Fishrock123
Fishrock123 reviewed Apr 7, 2016
View reviewed changes
src/node.cc Outdated
Copy link
Contributor

@Fishrock123 Fishrock123 Apr 7, 2016

Choose a reason for hiding this comment

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

Is this process._events?

Copy link
Contributor Author

@mscdex mscdex Apr 7, 2016

Choose a reason for hiding this comment

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

Yes.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Apr 7, 2016

Hmmm, can this be back-ported? Theoretically, this could be considered a security issue for unvalidated input.

@Fishrock123 Fishrock123 mentioned this pull request Apr 7, 2016
Merged
3 tasks
@mscdex
Copy link
Contributor Author

mscdex commented Apr 7, 2016

@Fishrock123 It's definitely a semver-major change, but I don't recall how that plays out with backporting/LTS/etc.

@mscdex mscdex mentioned this pull request Apr 7, 2016
Closed
3 tasks
@mscdex
Copy link
Contributor Author

mscdex commented Apr 7, 2016

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/187/

@jasnell
Copy link
Member

jasnell commented Apr 7, 2016

I don't believe back porting would be possible to v4. It might be safe to get into v5 tho.

@jasnell
Copy link
Member

jasnell commented Apr 7, 2016

LGTM

@mscdex
Copy link
Contributor Author

mscdex commented Apr 7, 2016

CI is green except for a flaky test and citgm is green.

@mscdex
Copy link
Contributor Author

mscdex commented Apr 18, 2016

CI again one last time: https://ci.nodejs.org/job/node-test-pull-request/2305/

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

Still LGTM

@mscdex
events: don't inherit from Object.prototype
e38bade 
This commit safely allows event names that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__.

Fixes: nodejs#728
PR-URL: nodejs#6092
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mscdex mscdex force-pushed the events-empty-prototype branch from 3711c52 to e38bade Compare April 19, 2016 01:47
@mscdex mscdex merged commit e38bade into nodejs:master Apr 19, 2016
@mscdex mscdex deleted the events-empty-prototype branch April 19, 2016 01:49
@Fishrock123
Copy link
Contributor

Fishrock123 commented Apr 19, 2016

iirc usually we don't care about warnings

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

Typically, but it's nice to avoid them when possible to do so, no?

@Fishrock123
Copy link
Contributor

Fishrock123 commented Apr 19, 2016 •
edited
Loading

I guess, just consider though, that there are tons of warnings of the same and there isn't necessarily easy ways to avoid it.

@thefourtheye thefourtheye mentioned this pull request Apr 19, 2016
Closed
2 tasks
thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Apr 19, 2016
@thefourtheye
src: fix -Wunused-result warning in e38bade
52a40be 
This patch fixes the warning introduced by the changes in e38bade.

Ref: nodejs#6092
thefourtheye added a commit that referenced this pull request Apr 19, 2016
@thefourtheye
src: fix -Wunused-result warning in e38bade
697790c 
This patch fixes the warning introduced by the changes in e38bade.

Ref: #6092
PR-URL: #6276
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@mscdex @joelostrowski
events: don't inherit from Object.prototype
0324c24 
This commit safely allows event names that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__.

Fixes: nodejs#728
PR-URL: nodejs#6092
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@thefourtheye @joelostrowski
src: fix -Wunused-result warning in e38bade
2a45a0b 
This patch fixes the warning introduced by the changes in e38bade.

Ref: nodejs#6092
PR-URL: nodejs#6276
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@mscdex @jasnell
events: don't inherit from Object.prototype
c3ad97d 
This commit safely allows event names that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__.

Fixes: #728
PR-URL: #6092
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@thefourtheye @jasnell
src: fix -Wunused-result warning in e38bade
f286e1e 
This patch fixes the warning introduced by the changes in e38bade.

Ref: #6092
PR-URL: #6276
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@emorikawa emorikawa mentioned this pull request Oct 11, 2016
Closed
@BridgeAR BridgeAR mentioned this pull request Apr 1, 2019
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

events Issues and PRs related to the events subsystem / EventEmitter. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@mscdex @thefourtheye @evanlucas @vkurchatkin @Fishrock123 @jasnell @addaleax