Skip to content

src: fix vector subscript out of range - #18460

Closed
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:fix-18459
Closed

src: fix vector subscript out of range#18460
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:fix-18459

Conversation

@apapirovski

@apapirovskiapapirovski commented Jan 30, 2018

Copy link
Copy Markdown
Contributor

It appears that #18291 broke debug builds on Windows. This should resolve the issue.

@tniessen is currently running a test. If anyone else can try a Windows debug build with this patch applied, that would be appreciated.

Fixes: #18459

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@apapirovskiapapirovski added c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land. labels Jan 30, 2018
@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 30, 2018
@apapirovski

apapirovski commented Jan 30, 2018

Copy link
Copy Markdown
ContributorAuthor

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

tniessen
tniessen approved these changes Jan 30, 2018
View reviewed changes

@tniessentniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tests pass: https://ci.nodejs.org/job/node-test-commit-light/196/
This seems to solve the problem. Thanks for the quick fix @apapirovski!

@tniessen

tniessen commented Jan 30, 2018

Copy link
Copy Markdown
Member

It would be great if someone else from @nodejs/platform-windows could confirm that this indeed fixes the issue before fast-tracking this.

@apapirovski

apapirovski commented Jan 30, 2018

Copy link
Copy Markdown
ContributorAuthor

/cc @addaleax@jasnell

bnoordhuis
bnoordhuis reviewed Jan 30, 2018
View reviewed changes
Comment threadsrc/node.cc Outdated

@bnoordhuisbnoordhuisJan 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could replace &args[1] with &args.data()[1] to get rid of the conditional.

(It's legal to create a pointer that points one element beyond the array as long as you don't dereference it - which won't happen if argc == 0 because there won't be elements to copy.)

@seishunseishunJan 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Even better: replace it with args.begin() + 1, since std::copy works with iterators.

@apapirovskiapapirovskiJan 30, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ok, updated now. Thanks for the feedback.

seishun
seishun suggested changes Jan 30, 2018
View reviewed changes

@seishunseishun left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use either mine or @bnoordhuis's suggestion to avoid the conditional.

@apapirovski

apapirovski commented Jan 30, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@apapirovski

apapirovski commented Jan 31, 2018

Copy link
Copy Markdown
ContributorAuthor

Landed in 332b56c

@apapirovski
apapirovski deleted the fix-18459 branch January 31, 2018 14:36
apapirovski added a commit that referenced this pull request Jan 31, 2018
@apapirovski
src: fix vector subscript out of range
332b56c
PR-URL: #18460Fixes: #18459
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
@apapirovski@MylesBorins
src: fix vector subscript out of range
10b3345
PR-URL: #18460Fixes: #18459
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@apapirovski@MylesBorins
src: fix vector subscript out of range
8c39ea3
PR-URL: #18460Fixes: #18459
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@apapirovski@MylesBorins
src: fix vector subscript out of range
272fd2e
PR-URL: #18460Fixes: #18459
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
@MylesBorinsMylesBorins mentioned this pull request Feb 21, 2018
Merged
@MylesBorinsMylesBorins mentioned this pull request Mar 20, 2018
Closed
3 tasks
@MylesBorins

MylesBorins commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

Needs to land if we backport #18291

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@apapirovski@MayaLekova
src: fix vector subscript out of range
f15594c
PR-URL: nodejs#18460Fixes: nodejs#18459
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis left review comments
@cjihrigcjihrigcjihrig approved these changes
@tniessentniessentniessen approved these changes
+2 more reviewers
@seishunseishunseishun approved these changes
@XadillaXXadillaXXadillaX approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.fast-trackPRs that do not need to wait for 48 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Debug assertion fails on Windows

8 participants

@apapirovski@tniessen@MylesBorins@bnoordhuis@seishun@cjihrig@XadillaX@nodejs-github-bot