Skip to content

Conversation

@TimothyGu
Copy link
Member

Also add test cases for partial writes and invalid indices.

Fixes: #8724

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)

src, buffer

Also add test cases for partial writes and invalid indices. Fixes: nodejs#8724
@TimothyGuTimothyGu added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Mar 19, 2017
@nodejs-github-botnodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Mar 19, 2017
@TimothyGu
Copy link
MemberAuthor

No significant performance changes:

 improvement confidence p.value buffers/buffer-write.js millions=3 type="FloatLE" buffer="fast" noAssert="false" 2.77 % 0.20424472 buffers/buffer-write.js millions=3 type="FloatLE" buffer="fast" noAssert="true" 4.72 % * 0.04616853 buffers/buffer-write.js millions=3 type="FloatLE" buffer="slow" noAssert="false" 8.50 % * 0.01310677 buffers/buffer-write.js millions=3 type="FloatLE" buffer="slow" noAssert="true" 3.21 % 0.27286382 

@TimothyGu
Copy link
MemberAuthor

size_t def,
size_t* ret){
size_t* ret,
size_t needed = 0){
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yes, below.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I mean, inside of ParseArrayIndex? I feel like I am missing the point of passing this argument…

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Wait... oops, git commit -p fail. Updated patch incoming.

@TimothyGu
Copy link
MemberAuthor

@addaleax, @bnoordhuis, PTAL.

Copy link
Member

@addaleaxaddaleax left a comment

Choose a reason for hiding this comment

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

src/ changes LGTM.

The test file is a bit hard to grok but the changes are additive, so … ¯\_(ツ)_/¯

jasnell pushed a commit that referenced this pull request Mar 22, 2017
Also add test cases for partial writes and invalid indices. PR-URL: #11927Fixes: #8724 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member

Landed in 4fb9b12

@jasnelljasnell closed this Mar 22, 2017
@TimothyGuTimothyGu deleted the buffer-oor branch March 22, 2017 05:50
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
Also add test cases for partial writes and invalid indices. PR-URL: #11927Fixes: #8724 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Mar 28, 2017
MylesBorins added a commit that referenced this pull request Mar 28, 2017
Notable changes: * buffer: - do not segfault on out-of-range index (Timothy Gu) #11927 * crypto: - Fix memory leak if certificate is revoked (Tom Atkinson) #12089 * deps: * upgrade npm to 4.2.0 (Kat Marchán) #11389 * fix async await desugaring in V8 (Michaël Zasso) #12004 * readline: - add option to stop duplicates in history (Danny Nemer) #2982 * src: - add native URL class (James M Snell) #11801 PR-URL: #12104
MylesBorins added a commit that referenced this pull request Mar 29, 2017
Notable changes: * buffer: - do not segfault on out-of-range index (Timothy Gu) #11927 * crypto: - Fix memory leak if certificate is revoked (Tom Atkinson) #12089 * deps: * upgrade npm to 4.2.0 (Kat Marchán) #11389 * fix async await desugaring in V8 (Michaël Zasso) #12004 * readline: - add option to stop duplicates in history (Danny Nemer) #2982 * src: - add native URL class (James M Snell) #11801 PR-URL: #12104
@italoacasasitaloacasas mentioned this pull request Apr 10, 2017
2 tasks
@MylesBorins
Copy link
Contributor

Landed on v6.x-staging. Not landing cleanly on v4.x due to differences in releases. please feel free to change label and backport

MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
Also add test cases for partial writes and invalid indices. PR-URL: #11927Fixes: #8724 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Also add test cases for partial writes and invalid indices. PR-URL: #11927Fixes: #8724 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Apr 19, 2017
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
 Notable changes: * buffer: - do not segfault on out-of-range index (Timothy Gu) nodejs/node#11927 * crypto: - Fix memory leak if certificate is revoked (Tom Atkinson) nodejs/node#12089 * deps: * upgrade npm to 4.2.0 (Kat Marchán) nodejs/node#11389 * fix async await desugaring in V8 (Michaël Zasso) nodejs/node#12004 * readline: - add option to stop duplicates in history (Danny Nemer) nodejs/node#2982 * src: - add native URL class (James M Snell) nodejs/node#11801 PR-URL: nodejs/node#12104 Signed-off-by: Ilkka Myller <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Also add test cases for partial writes and invalid indices. PR-URL: nodejs/node#11927Fixes: nodejs/node#8724 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bufferIssues and PRs related to the buffer subsystem.c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

buffer: segfault writing values with noAssert=true

5 participants

@TimothyGu@jasnell@MylesBorins@addaleax@nodejs-github-bot