Fix windows segfault #6938

Kelmar · 2016-05-23T21:17:24Z

Fix for Windows Segfault with missing arguments.

Platform: Windows
Arch: x64
Compiler: VC2015

When specifying a parameter that requires an additional argument on the
command line, node would segfault. This appears to be specific to
Windows, adjusted command line argument parsing to hold a nullptr
terminal.

Take two of the fix, based on feedback from @addaleax

Arch: x64 Compiler: VC2015 When specifing a parameter that requries an additional argument on the command line, node would segfault. This appears to be specific to Windows, adjusted command line argument parsing to hold a nullptr terminal.

addaleax · 2016-05-23T21:25:13Z

The code changes looks good. I don’t know if you’ve read your way through the contributing guidlines, but a couple of things things that would be awesome here:

Ideally, there would be a regression test added in this PR. You can take a look at e.g. test/parallel/test-cli-eval.js and see if you can add a similar test there, where you invoke only node -e.
The commit message would need to look a bit more like specified in the guidelines, i.e. there would be a subject line that says in a very short form what was changed here.

@thefourtheye This approach looks good to you?

cjihrig · 2016-05-23T21:35:42Z

@addaleax did you mean to close this?

cjihrig · 2016-05-23T21:36:07Z

Sorry, confused this with the other PR with the same title.

bnoordhuis · 2016-05-24T12:43:36Z

LGTM but the commit log should conform to the style guide and a regression test would be nice.

This bug is over four years old. It's amusing and a little odd it managed to go undiscovered for so long.

Kelmar · 2016-05-24T14:26:45Z

@addaleax I'll get those changes in as soon as I am able; really I just ran across it and was like, "that seems like an easy fix." But yea, I'm glad to see there are regression tests. :)

addaleax · 2016-05-24T14:31:39Z

@Kelmar yeah, don’t worry. Adding a regression test can also be done later and/or by someone else if you don’t find the time – again, if you need anything finding your way around here, just ask. :)

Kelmar · 2016-05-24T15:07:13Z

@addaleax Done! I'll read over the contributing documentation now. I hope you find this fix helpful. :)

addaleax · 2016-05-24T15:15:18Z

test/parallel/test-cli-eval.js

+// Missing argument should not crash
+child.exec(nodejs + ' -e', function (status, stdout, stderr) {
+  assert.notStrictEqual(status, null);
+  assert.equal(status.code, 9);


nit: It’s generally preferred to use assert.strictEqual over assert.equal

Kelmar · 2016-05-24T15:21:17Z

Done!

addaleax · 2016-05-24T15:25:27Z

LGTM up to assert.equal -> assert.strictEqual.

Thank you for the report + bugfix! Really funny this went unnoticed so long.

thefourtheye · 2016-05-26T13:52:55Z

test/parallel/test-cli-eval.js

@@ -66,6 +66,12 @@ child.exec(nodejs + ' --eval "require(\'./test/parallel/test-cli-eval.js\')"',
      assert.equal(status.code, 42);
    });

+// Missing argument should not crash
+child.exec(nodejs + ' -e', function(status, stdout, stderr) {


As the assertion happens within the callback, its better to use common.mustCall

thefourtheye · 2016-05-26T13:55:09Z

LGTM + nits.

addaleax · 2016-05-29T20:56:03Z

ping @Kelmar … There are only two things left to address here (commit log should conform to the style guide + Use strictEqual), that should be all and then this can be landed.

Kelmar · 2016-06-06T15:54:37Z

@addaleax Okay, I believe this should fix everything up to your submission guide lines. Please let me know if I've missed anything or didn't interpret the instructions correctly. :)

addaleax · 2016-06-06T21:05:51Z

@Kelmar There’s actually one point by @thefourtheye still open, namely that the callback for child.exec should be wrapped in common.mustCall(), like child.exec(…, common.mustCall(function(status, stdout, stderr) { … })) to make sure that the callback actually gets called.

I realize that the other tests in the file don’t set a good example here, but it would still be great if you could update your added test with that.

addaleax · 2016-06-18T19:38:17Z

CI: https://ci.nodejs.org/job/node-test-commit/3780/

addaleax · 2016-06-20T00:04:28Z

Landed in 5bddc9a, fixed commit log and nit on landing.

@Kelmar Thanks for your contribution!

When specifing a parameter that requries an additional argument on the command line, node would segfault. This appears to be specific to Windows, adjusted command line argument parsing to hold a nullptr terminal. Adding unit test for crash on missing arguments. PR-URL: #6938 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Platform: Windows

fb6f59e

Arch: x64 Compiler: VC2015 When specifing a parameter that requries an additional argument on the command line, node would segfault. This appears to be specific to Windows, adjusted command line argument parsing to hold a nullptr terminal.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 23, 2016

addaleax mentioned this pull request May 23, 2016

Platform: Windows #6935

Closed

addaleax added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 23, 2016

mscdex added windows Issues and PRs related to the Windows platform. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels May 24, 2016

Adding unit test for crash on missing arguments.

1eaeb81

Minor syntax change to fix styling.

ad915d0

addaleax reviewed May 24, 2016
View reviewed changes

Reverting whitespace change

f1f9a29

Fishrock123 changed the title ~~Platform: Windows~~ Fix windows segfault May 24, 2016

addaleax added the lts-watch-v4.x label May 24, 2016

thefourtheye reviewed May 26, 2016
View reviewed changes

Correcting unit test to standards

18a58af

Another quick change for unit tests.

290d19e

addaleax closed this Jun 20, 2016

Fishrock123 mentioned this pull request Jun 27, 2016

Propose v6.3.0 #7441

Merged

Fishrock123 mentioned this pull request Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jul 11, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

Uh oh!

Fix windows segfault #6938

Fix windows segfault #6938

Uh oh!

Conversation

Kelmar commented May 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 23, 2016

Uh oh!

cjihrig commented May 23, 2016

Uh oh!

cjihrig commented May 23, 2016

Uh oh!

bnoordhuis commented May 24, 2016

Uh oh!

Kelmar commented May 24, 2016

Uh oh!

addaleax commented May 24, 2016

Uh oh!

Kelmar commented May 24, 2016

Uh oh!

addaleax May 24, 2016

Choose a reason for hiding this comment

Uh oh!

Kelmar commented May 24, 2016

Uh oh!

addaleax commented May 24, 2016

Uh oh!

thefourtheye May 26, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented May 26, 2016

Uh oh!

addaleax commented May 29, 2016

Uh oh!

Kelmar commented Jun 6, 2016

Uh oh!

addaleax commented Jun 6, 2016

Uh oh!

addaleax commented Jun 18, 2016

Uh oh!

addaleax commented Jun 20, 2016

Uh oh!

Uh oh!

Kelmar commented May 23, 2016 •

edited

Loading