-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
net: allow IPC servers be accessible by all #19472
Conversation
doc/api/net.md
Outdated
@@ -250,6 +250,10 @@ added: v0.11.14 | |||
* `backlog` {number} Common parameter of [`server.listen()`][] | |||
functions. | |||
* `exclusive` {boolean} **Default:** `false` | |||
* `readable_all` {boolean} For IPC servers makes the pipe readable | |||
for all users. ***Default:** `false` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: ***
-> **
doc/api/net.md
Outdated
* `readable_all` {boolean} For IPC servers makes the pipe readable | ||
for all users. ***Default:** `false` | ||
* `writable_all` {boolean} For IPC servers makes the pipe writable | ||
for all users. ***Default:** `false` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: ***
-> **
doc/api/net.md
Outdated
@@ -250,6 +250,10 @@ added: v0.11.14 | |||
* `backlog` {number} Common parameter of [`server.listen()`][] | |||
functions. | |||
* `exclusive` {boolean} **Default:** `false` | |||
* `readable_all` {boolean} For IPC servers makes the pipe readable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These options should be camelCase.
lib/net.js
Outdated
@@ -1524,6 +1524,13 @@ Server.prototype.listen = function(...args) { | |||
backlog = options.backlog || backlogFromArgs; | |||
listenInCluster(this, pipeName, -1, -1, | |||
backlog, undefined, options.exclusive); | |||
let mode = 0; | |||
if (!!options.readable_all) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are expected to be booleans, you could do options.readableAll === true
.
Updated, PTAL. |
ping CI failures are unrelated. |
As per "Who to cc...", cc @bnoordhuis, @indutny, @nodejs/streams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pinging @santigimeno since he also reviewed the libuv pull request.
* `readableAll` {boolean} For IPC servers makes the pipe readable | ||
for all users. **Default:** `false` | ||
* `writableAll` {boolean} For IPC servers makes the pipe writable | ||
for all users. **Default:** `false` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should explain why you would want to do that, not just how.
lib/net.js
Outdated
let mode = 0; | ||
if (options.readableAll === true) | ||
mode |= PipeConstants.READABLE_ALL; | ||
if (options.writableAll === true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, what is the reason you went for strict true-ness instead of truthiness?
lib/net.js
Outdated
if (options.writableAll === true) | ||
mode |= PipeConstants.WRITABLE_ALL; | ||
if (mode !== 0) | ||
this._handle.fchmod(mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the return value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. A couple of comments:
- I guess it's not easy testing this functionality on every platform but we could add tests that at least make sure that the changes don't break anything. We could even use
fs.stat
to check the mode of the pipe path on unices? - We should merge pipe: fixed uv_pipe_chmod for OSX Darwin libuv/libuv#1635 soon to fix
uv_pipe_chmod
onOS X
.
@bzoz seems like the tests are failing. |
Linux failures seem unrelated, re-running the CI just to make sure: https://ci.nodejs.org/job/node-test-pull-request/14333/ |
Failures look unrelated, PTAL |
Ping? |
It would be good to get some further LGs for this. Ping @nodejs/http @nodejs/http2 @nodejs/streams |
src/pipe_wrap.cc
Outdated
void PipeWrap::Fchmod(const v8::FunctionCallbackInfo<v8::Value>& args) { | ||
PipeWrap* wrap; | ||
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); | ||
int mode = args[0]->Int32Value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use the overload that takes a context
argument, or (even better imo) write this as
CHECK(args[0]->IsInt32());
int mode = args[0].As<Int32>()->Value();
?
src/pipe_wrap.h
Outdated
enum PipeChmod { | ||
READABLE_ALL = UV_READABLE, | ||
WRITABLE_ALL = UV_WRITABLE | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason not to expose the libuv names directly? Makes things a bit more grep
able :)
Gentle ping in the direction of @bzoz :) |
Adds mappings to uv_pipe_chmod call by adding two new options to listen call. This allows the IPC server pipe to be made readable or writable by all users. Fixes: nodejs#19154
76f7e9e
to
6d8b6dc
Compare
Updated (and rebased), PTAL |
Failures are unrelated, If no one objects I'll land this tomorrow. |
Landed in 531b4be |
Marking semver-minor because of the two new options. |
Notable Changes: * **deps**: - upgrade npm to 6.1.0 (Rebecca Turner) #20190 * **fs**: - fix reads with pos \> 4GB (Mathias Buus) #21003 * **net**: - new option to allow IPC servers to be readable and writable by all users (Bartosz Sosnowski) #19472 * **stream**: - fix removeAllListeners() for Stream.Readable to work as expected when no arguments are passed (Kael Zhang) #20924 * **Added new collaborators** - John-David Dalton (https://github.com/jdalton) PR-URL: #21011
Notable Changes: * **deps**: - upgrade npm to 6.1.0 (Rebecca Turner) #20190 * **fs**: - fix reads with pos \> 4GB (Mathias Buus) #21003 * **net**: - new option to allow IPC servers to be readable and writable by all users (Bartosz Sosnowski) #19472 * **stream**: - fix removeAllListeners() for Stream.Readable to work as expected when no arguments are passed (Kael Zhang) #20924 * **Added new collaborators** - John-David Dalton (https://github.com/jdalton) PR-URL: #21011
Notable Changes: * **deps**: - upgrade npm to 6.1.0 (Rebecca Turner) #20190 * **fs**: - fix reads with pos \> 4GB (Mathias Buus) #21003 * **net**: - new option to allow IPC servers to be readable and writable by all users (Bartosz Sosnowski) #19472 * **stream**: - fix removeAllListeners() for Stream.Readable to work as expected when no arguments are passed (Kael Zhang) #20924 * **Added new collaborators** - John-David Dalton (https://github.com/jdalton) PR-URL: #21011
Just out of curiosity, is there any chance of this receiving a backport to Node 8 even though it already entered maintenance state? It seems like a very minor task as the proper libuv version is already present, and since there is no easy workaround for the lack of this functionality on node 8, especially on windows, I'd be happy to give it a shot. |
@jeroenvollenbrock I do not believe this will be able to land on 8.x. It is in maintenance mode and as such we don't do regular semver-minors. We did recently do one, but the only minor changes that landed were updates to napi /cc @nodejs/lts |
I understand and very well respect the policy of not backporting semver-minor changes. I do think however it might be a good idea to make it a bit more clear in the documentation that readableAll and writableAll are node 10+ options in case a backport is definitely not possible. The docs currently make it seem like these two options have been around since Node v0.11.14. I actually only found out they weren't after debugging EACCESS errors, and manually compared v8 docs with the v10 docs. |
Adds mappings to
uv_pipe_chmod
call by adding two new options to listen call. This allows the IPC server pipe to be made readable or writable by all users.Fixes: #19154
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes