Skip to content

src, buffer: do not segfault on out-of-range index#11927

Closed
TimothyGu wants to merge 2 commits intonodejs:masterfrom
TimothyGu:buffer-oor
Closed

src, buffer: do not segfault on out-of-range index#11927
TimothyGu wants to merge 2 commits intonodejs:masterfrom
TimothyGu:buffer-oor

Conversation

@TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Mar 19, 2017

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

@TimothyGu
src, buffer: do not segfault on out-of-range index
c1bf555 
Also add test cases for partial writes and invalid indices.

Fixes: nodejs#8724
@TimothyGu TimothyGu 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-bot nodejs-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
Member Author

TimothyGu commented Mar 19, 2017

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
Member Author

TimothyGu commented Mar 19, 2017

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

addaleax
addaleax reviewed Mar 19, 2017
View reviewed changes
src/node_buffer.cc
size_t def,
size_t* ret) {
size_t* ret,
size_t needed = 0) {
Copy link
Member

@addaleax addaleax Mar 19, 2017

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
Member Author

@TimothyGu TimothyGu Mar 19, 2017

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

@addaleax addaleax Mar 19, 2017

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
Member Author

@TimothyGu TimothyGu Mar 19, 2017

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
Member Author

TimothyGu commented Mar 22, 2017

@addaleax, @bnoordhuis, PTAL.

addaleax
addaleax approved these changes Mar 22, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax 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
@TimothyGu @jasnell
src, buffer: do not segfault on out-of-range index
4fb9b12 
Also add test cases for partial writes and invalid indices.

PR-URL: #11927
Fixes: #8724
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

Landed in 4fb9b12

@jasnell jasnell closed this Mar 22, 2017
@TimothyGu TimothyGu deleted the buffer-oor branch March 22, 2017 05:50
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
@TimothyGu @MylesBorins
src, buffer: do not segfault on out-of-range index
59f71f5 
Also add test cases for partial writes and invalid indices.

PR-URL: #11927
Fixes: #8724
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
Merged
MylesBorins added a commit that referenced this pull request Mar 28, 2017
@MylesBorins
2017-03-28, Version 7.8.0 (Current)
f04524e 
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
@MylesBorins
2017-03-28, Version 7.8.0 (Current)
9b2dd47 
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
@TimothyGu TimothyGu mentioned this pull request Apr 3, 2017
Closed
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 18, 2017

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
@TimothyGu @MylesBorins
src, buffer: do not segfault on out-of-range index
c9ebc4d 
Also add test cases for partial writes and invalid indices.

PR-URL: #11927
Fixes: #8724
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@TimothyGu @MylesBorins
src, buffer: do not segfault on out-of-range index
c33933e 
Also add test cases for partial writes and invalid indices.

PR-URL: #11927
Fixes: #8724
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-03-28, Version 7.8.0 (Current)
e0661dd 
    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 <ilkka.myller@nodefield.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@TimothyGu @andrew749
src, buffer: do not segfault on out-of-range index
086f1f4 
Also add test cases for partial writes and invalid indices.

PR-URL: nodejs/node#11927
Fixes: nodejs/node#8724
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

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