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

src: fix vector subscript out of range by apapirovski · Pull Request #18460 · nodejs/node · GitHub

/ node Public

src: fix vector subscript out of range - #18460

Closed
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:fix-18459
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

apapirovski 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

apapirovski 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
apapirovski requested a review from tniessen January 30, 2018 18:38
nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 30, 2018

apapirovski commented Jan 30, 2018

Copy link
Copy Markdown
Contributor Author

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

tniessen approved these changes Jan 30, 2018
View reviewed changes

tniessen 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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 commented Jan 30, 2018

Copy link
Copy Markdown
Contributor Author

/cc @addaleax @jasnell

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

bnoordhuis Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.)

seishun Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

apapirovski Jan 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok, updated now. Thanks for the feedback.

seishun suggested changes Jan 30, 2018
View reviewed changes

seishun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

apapirovski commented Jan 30, 2018

Copy link
Copy Markdown
Contributor Author

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

apapirovski commented Jan 31, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 332b56c

apapirovski deleted the fix-18459 branch January 31, 2018 14:36
apapirovski added a commit that referenced this pull request Jan 31, 2018
src: fix vector subscript out of range
332b56c 
PR-URL: #18460
Fixes: #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
src: fix vector subscript out of range
10b3345 
PR-URL: #18460
Fixes: #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
src: fix vector subscript out of range
8c39ea3 
PR-URL: #18460
Fixes: #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
src: fix vector subscript out of range
272fd2e 
PR-URL: #18460
Fixes: #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 mentioned this pull request Feb 21, 2018
Merged
MylesBorins mentioned this pull request Mar 20, 2018
Closed
3 tasks

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
src: fix vector subscript out of range
f15594c 
PR-URL: nodejs#18460
Fixes: 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>
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

bnoordhuis bnoordhuis left review comments
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
+2 more reviewers
seishun seishun approved these changes
XadillaX XadillaX 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-track PRs 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

Back | FazBrowse Home | New Git URL