windows: fix opening of read-only stdin pipes #1223
Conversation
|
Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion. The following commiters were not found in the CLA:
You can fix all these things without opening another issue. Please see CONTRIBUTING.md for more information |
| /* that is not a pipe. */ | ||
| if (GetLastError() == ERROR_INVALID_PARAMETER) { | ||
| SetLastError(WSAENOTSOCK); | ||
| DWORD err = GetLastError(); |
There was a problem hiding this comment.
Please define it at the top of the function.
|
Updated, thanks. |
|
Would Code looks ok to me, my Windows kung-fu is pretty limited though :-/ |
|
Updated to reset writable flag. Added a test to node.js here: nodejs/node-v0.x-archive#7433 |
| return UV_EINVAL; | ||
| } | ||
|
|
||
| uv_pipe_connection_init(pipe); | ||
| pipe->handle = os_handle; | ||
| pipe->flags |= UV_HANDLE_READABLE | UV_HANDLE_WRITABLE; |
There was a problem hiding this comment.
Where are the flags and handle set in the pipe now? I can't see it in the diff.
There was a problem hiding this comment.
See line 1771:
- DWORD duplex_flags = UV_HANDLE_READABLE | UV_HANDLE_WRITABLE;
uv_set_pipe_handle is setting the flags, clearing UV_HANDLE_WRITABLE should the pipe be read-only.
There was a problem hiding this comment.
Yes, but where do we save those flags into pipe->flags now?
There was a problem hiding this comment.
|
LGTM, though I'd like @piscisaureus's blessing :-) |
| SetLastError(WSAENOTSOCK); | ||
| err = GetLastError(); | ||
| if (err == ERROR_ACCESS_DENIED) { | ||
| /* SetNamedPipeHandleState can fail if the handle doesn't have either */ |
There was a problem hiding this comment.
I know it is lame, but could you please make it a single block comment?
|
LGTM |
Fix nodejs/node-v0.x-archive#7345 Google Chrome is launching native messaging hosts by invoking cmd.exe with input/output redirected from/to named pipes. The host ends up with a read-only handle to the stdin pipe. This is causing SetNamedPipeHandleState to fail.
|
addressed style issues |
|
Fixed a couple of style nits since I was there and landed it as ba47e68 Thanks @orangemocha! |
Fix nodejs/node-v0.x-archive#7345
Google Chrome is launching native messaging hosts by invoking cmd.exe
with input/output redirected from/to named pipes. The host ends up
with a read-only handle to the stdin pipe. This is causing
SetNamedPipeHandleState to fail.