Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Platform: Windows #6935

Closed
wants to merge 1 commit into from
Closed

Platform: Windows #6935

wants to merge 1 commit into from

Conversation

Kelmar
Copy link

@Kelmar Kelmar commented May 23, 2016

Platform: Windows
Arch: x64
Compiler: VS2015

When running node with --eval or --require without the required argument,
node would segfault. This should correct that and display a reasonable
error message again.

Arch: x64
Compiler: VS2015

When running node with --evail or --require without the required argument,
node would segfault.  This should correct that and display a reasonable
error message again.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 23, 2016
@addaleax addaleax added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 23, 2016
@thefourtheye
Copy link
Contributor

thefourtheye commented May 23, 2016

@Kelmar I couldn't reproduce the problem

➜  node git:(master) node -e 'console.log(process.version)'
v6.2.0
➜  node git:(master) node --eval
node: --eval requires an argument
➜  node git:(master) node --require
node: --require requires an argument
➜  node git:(master) node --evail
node: bad option: --evail

Edit: Do you mean, this is specific to Windows?

@addaleax
Copy link
Member

It looks to me like this is a bug, but it might be better to fix it in node_main.cc: In this line, there is no additional slot for the trailing nullptr allocated, although it probably should be. The fact that argv[argc] == NULL is pretty much something one should be able to take for granted.

@Kelmar
Copy link
Author

Kelmar commented May 23, 2016

@addalex That is probably a more correct fix yes. The assumption seems to be that argv would be terminated with a nullptr, but apparently wasn't the case when I mistakenly did a "node -e" command and got a segfault.

@addaleax
Copy link
Member

@Kelmar Would you be interested in updating this PR with that fix or creating a new one?

@thefourtheye
Copy link
Contributor

argv is not a single string of characters, but an array of character pointers. argc specifies the length of the array. Why would additional slot for nullptr be needed?

@addaleax
Copy link
Member

addaleax commented May 23, 2016

@thefourtheye Yeah, I know. It’s standard-mandated that argv terminates with nullptr (see e.g. http://stackoverflow.com/a/16419021/688904 )… it’s definitely not needed, but it’s just something that coders may expect to hold true.

@Kelmar
Copy link
Author

Kelmar commented May 23, 2016

@addaleax I would be happy to make the change. :)

@addaleax
Copy link
Member

@Kelmar Great! Let us know if you need anything. :)

@thefourtheye
Copy link
Contributor

Ah, okay. If the standard mandates then lets do it. cc @bnoordhuis

@Kelmar
Copy link
Author

Kelmar commented May 23, 2016

I have created a new pull request arg_segfault_fix_2 :)

@addaleax
Copy link
Member

Closing this in favour of #6938 then :)

@addaleax addaleax closed this May 23, 2016
@Kelmar Kelmar deleted the fix_segfault_arg branch May 23, 2016 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants