FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: don't abort on prototype getters by trevnorris · Pull Request #3302 · nodejs/node · GitHub

/ node Public

buffer: don't abort on prototype getters - #3302

Closed
trevnorris wants to merge 1 commit into
nodejs:masterfrom
trevnorris:invalid-buf-prop-access
ConversationCommits (1)ChecksFiles changed
Closed

buffer: don't abort on prototype getters#3302
trevnorris wants to merge 1 commit into
nodejs:masterfrom
trevnorris:invalid-buf-prop-access

Conversation

trevnorris commented Oct 9, 2015

Copy link
Copy Markdown
Contributor

Accessing prototype properties directly on a typed array will throw. So
do an extra check in Buffer's own getters to verify it is being called
on an instance.

Fixes: #3297

R=@Fishrock123 ?

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

buffer: don't abort on prototype getters
63aaab1 
Accessing prototype properties directly on a typed array will throw. So
do an extra check in Buffer's own getters to verify it is being called
on an instance.

Fixes: nodejs#3297
trevnorris added the buffer Issues and PRs related to the buffer subsystem. label Oct 9, 2015

bnoordhuis commented Oct 9, 2015

Copy link
Copy Markdown
Member

LGTM

1 similar comment

Fishrock123 commented Oct 9, 2015

Copy link
Copy Markdown
Contributor

LGTM

trevnorris added a commit that referenced this pull request Oct 9, 2015
buffer: don't abort on prototype getters
e97dae5 
Accessing prototype properties directly on a typed array will throw. So
do an extra check in Buffer's own getters to verify it is being called
on an instance.

Fixes: #3297
PR-URL: #3302
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

trevnorris commented Oct 9, 2015

Copy link
Copy Markdown
Contributor Author

Thanks. Landed on e97dae5

trevnorris closed this Oct 9, 2015

thefourtheye commented Oct 10, 2015

Copy link
Copy Markdown
Contributor

Isn't this a major change? Behaviour is not backward compatible, right? Earlier we used to throw and we don't now.

trevnorris commented Oct 10, 2015

Copy link
Copy Markdown
Contributor Author

@thefourtheye This was a bug from the change to using typed arrays.

trevnorris commented Oct 10, 2015

Copy link
Copy Markdown
Contributor Author

If you take notice of the change, we didn't remove an exception. It was throwing because of bad access to a typed array property.

jasnell commented Oct 10, 2015

Copy link
Copy Markdown
Member

@trevnorris ... should this land in v4.x? ... nm... looking at it, there's no reason why not :-)

trevnorris added a commit that referenced this pull request Oct 10, 2015
buffer: don't abort on prototype getters
5479562 
Accessing prototype properties directly on a typed array will throw. So
do an extra check in Buffer's own getters to verify it is being called
on an instance.

Fixes: #3297
PR-URL: #3302
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell commented Oct 10, 2015

Copy link
Copy Markdown
Member

landed in v4.x in 5479562

jasnell mentioned this pull request Oct 10, 2015
Closed

thefourtheye commented Oct 11, 2015

Copy link
Copy Markdown
Contributor

If you take notice of the change, we didn't remove an exception. It was throwing because of bad access to a typed array property.

Ah, fine then. Thanks for clarifying :-)

trevnorris deleted the invalid-buf-prop-access branch March 28, 2016 22:25
snyk-bot mentioned this pull request Apr 24, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

buffer Issues and PRs related to the buffer subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL