Update PipeWrap to use uv_pipe_bind2 and uv_pipe_connect2 to restore ability to connect to abstract domain sockets #49667

ggoodman · 2023-09-15T13:44:28Z

This PR moves PipeWrap from the classic uv_pipe_bind and uv_pipe_connect methods to their newer uv_pipe_bind2 and uv_pipe_connect2 counterparts. This restores support for connecting to abstract unix domain sockets. Support was lost due to the refactor in libuv/libuv#4030 where uv_pipe_bind and uv_pipe_connect now infer the length of the pathname using strlen(). For an abstract unix socket, this was producing 0. Moving to uv_pipe_bind2 and uv_pipe_connect2 allows us to pass an explicit size_t for the socket path based on the length of the v8 string.

Fixes: #49656

ggoodman · 2023-09-15T13:50:51Z

Alright, here we go @santigimeno. I think I got the commit message guidelines down. Looking for feedback on the test. It should do the thing but might not be at the peak of elegance or codebase best practices.

Also, since I'm expecting to make changes; is rebasing ok or do we prefer new fix commits for review purposes?

santigimeno

Mostly looking good. Left some comments. Also a good idea is for you to test if the linter passes: make lint.

test/parallel/test-pipe-abstract-socket-http.js

ggoodman · 2023-09-15T18:47:09Z

@santigimeno turns out the guide you linked has almost exactly the test I wanted. I added an extra common.mustCall to ensure that the server successfully binds and calls the "listening" callback. Looks nice and tight now.

lpinca

LGMT with lint issues fixed

santigimeno

LGTM. Great work!

ggoodman · 2023-09-15T19:37:46Z

Ugh, copy paste mistake. Fixing.

The introduction of the uv_pipe_bind2 and uv_pipe_connect2 methods in libuv v1.46.0 changed the behaviour of uv_pipe_bind and uv_pipe_connect. This broke the ability to connect to abstract domain sockets on linux. This change ports PipeWrap to use the new uv_pipe_bind2 and uv_pipe_connect2 methods to restore abstract domain socket support. Fixes: nodejs#49656 Refs: libuv/libuv#4030

Introduce a new linux-only test for binding to an abstract unix socket and then making an http request against that socket. Refs: nodejs#49656

nodejs-github-bot · 2023-09-15T19:54:01Z

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

ggoodman · 2023-09-15T21:20:53Z

Seems like the osx11-x64 configuration in CI is having a bad day. Is the CI failure related or unrelated?

debadree25 · 2023-09-16T14:59:51Z

Looks unrelated will restart ci

nodejs-github-bot · 2023-09-16T15:00:14Z

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

nodejs-github-bot · 2023-09-18T07:11:24Z

Landed in 553169f

debadree25 · 2023-09-18T07:12:18Z

Thank you for your contribution @ggoodman 🚀

The introduction of the uv_pipe_bind2 and uv_pipe_connect2 methods in libuv v1.46.0 changed the behaviour of uv_pipe_bind and uv_pipe_connect. This broke the ability to connect to abstract domain sockets on linux. This change ports PipeWrap to use the new uv_pipe_bind2 and uv_pipe_connect2 methods to restore abstract domain socket support. Fixes: #49656 Refs: libuv/libuv#4030 PR-URL: #49667 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The introduction of the uv_pipe_bind2 and uv_pipe_connect2 methods in libuv v1.46.0 changed the behaviour of uv_pipe_bind and uv_pipe_connect. This broke the ability to connect to abstract domain sockets on linux. This change ports PipeWrap to use the new uv_pipe_bind2 and uv_pipe_connect2 methods to restore abstract domain socket support. Fixes: nodejs#49656 Refs: libuv/libuv#4030 PR-URL: nodejs#49667 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

We need to handle errors from uv_pipe_connect2() because return type is `int`. Fixes: nodejs#50652 Refs: nodejs#49667 Refs: libuv/libuv#4030

We need to handle errors from uv_pipe_connect2() because return type is `int`. Fixes: #50652 Refs: #49667 Refs: libuv/libuv#4030 PR-URL: #50657 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: theanarkh <theratliter@gmail.com>

PR-URL: #50904 Refs: #49667 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

We need to handle errors from uv_pipe_connect2() because return type is `int`. Fixes: #50652 Refs: #49667 Refs: libuv/libuv#4030 PR-URL: #50657 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: theanarkh <theratliter@gmail.com>

PR-URL: #50904 Refs: #49667 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Sep 15, 2023

santigimeno requested changes Sep 15, 2023

View reviewed changes

test/parallel/test-pipe-abstract-socket-http.js Outdated Show resolved Hide resolved

test/parallel/test-pipe-abstract-socket-http.js Outdated Show resolved Hide resolved

santigimeno reviewed Sep 15, 2023

View reviewed changes

test/parallel/test-pipe-abstract-socket-http.js Outdated Show resolved Hide resolved

lpinca approved these changes Sep 15, 2023

View reviewed changes

ggoodman force-pushed the fix-49656 branch from 2db02d2 to 420c22b Compare September 15, 2023 19:13

santigimeno approved these changes Sep 15, 2023

View reviewed changes

ggoodman added 2 commits September 15, 2023 15:38

test: adds linux-only test for abstract sockets

8afecf1

Introduce a new linux-only test for binding to an abstract unix socket and then making an http request against that socket. Refs: nodejs#49656

ggoodman force-pushed the fix-49656 branch from 420c22b to 8afecf1 Compare September 15, 2023 19:39

debadree25 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 15, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 15, 2023

github-actions bot mentioned this pull request Sep 16, 2023

CI Reliability 2023-09-16 nodejs/reliability#667

Open

25 tasks

jasnell approved these changes Sep 16, 2023

View reviewed changes

This was referenced Sep 17, 2023

CI Reliability 2023-09-17 nodejs/reliability#668

Open

CI Reliability 2023-09-18 nodejs/reliability#669

Open

debadree25 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 18, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 18, 2023

nodejs-github-bot merged commit 553169f into nodejs:main Sep 18, 2023
33 checks passed

This was referenced Sep 19, 2023

CI Reliability 2023-09-19 nodejs/reliability#670

Open

CI Reliability 2023-09-20 nodejs/reliability#671

Open

This was referenced Sep 28, 2023

v20.7.1 proposal #49917

Closed

v20.8.0 proposal #49932

Merged

deokjinkim added a commit to deokjinkim/node that referenced this pull request Nov 10, 2023

src: handle errors from uv_pipe_connect2()

76acb04

We need to handle errors from uv_pipe_connect2() because return type is `int`. Fixes: nodejs#50652 Refs: nodejs#49667 Refs: libuv/libuv#4030

deokjinkim added a commit to deokjinkim/node that referenced this pull request Nov 10, 2023

src: handle errors from uv_pipe_connect2()

68ce7f9

We need to handle errors from uv_pipe_connect2() because return type is `int`. Fixes: nodejs#50652 Refs: nodejs#49667 Refs: libuv/libuv#4030

deokjinkim mentioned this pull request Nov 10, 2023

src: handle errors from uv_pipe_connect2() #50657

Merged

yseymour mentioned this pull request Nov 23, 2023

When binding server to a UNIX socket in an abstract namespace, the address length is wrong #50889

Closed

theanarkh mentioned this pull request Nov 25, 2023

doc: add doc for Unix abstract socket #50904

Merged

4 tasks

nodejs-github-bot pushed a commit that referenced this pull request Nov 28, 2023

doc: add doc for Unix abstract socket

69866bf

PR-URL: #50904 Refs: #49667 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

targos pushed a commit that referenced this pull request Dec 4, 2023

doc: add doc for Unix abstract socket

eecab88

PR-URL: #50904 Refs: #49667 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

richardlau pushed a commit that referenced this pull request Mar 25, 2024

doc: add doc for Unix abstract socket

5f8e7a0

PR-URL: #50904 Refs: #49667 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PipeWrap to use uv_pipe_bind2 and uv_pipe_connect2 to restore ability to connect to abstract domain sockets #49667

Update PipeWrap to use uv_pipe_bind2 and uv_pipe_connect2 to restore ability to connect to abstract domain sockets #49667

ggoodman commented Sep 15, 2023

ggoodman commented Sep 15, 2023

santigimeno left a comment

ggoodman commented Sep 15, 2023

lpinca left a comment

santigimeno left a comment

ggoodman commented Sep 15, 2023

nodejs-github-bot commented Sep 15, 2023

ggoodman commented Sep 15, 2023

debadree25 commented Sep 16, 2023

nodejs-github-bot commented Sep 16, 2023

nodejs-github-bot commented Sep 18, 2023

debadree25 commented Sep 18, 2023

Update PipeWrap to use uv_pipe_bind2 and uv_pipe_connect2 to restore ability to connect to abstract domain sockets #49667

Update PipeWrap to use uv_pipe_bind2 and uv_pipe_connect2 to restore ability to connect to abstract domain sockets #49667

Conversation

ggoodman commented Sep 15, 2023

ggoodman commented Sep 15, 2023

santigimeno left a comment

Choose a reason for hiding this comment

ggoodman commented Sep 15, 2023

lpinca left a comment

Choose a reason for hiding this comment

santigimeno left a comment

Choose a reason for hiding this comment

ggoodman commented Sep 15, 2023

nodejs-github-bot commented Sep 15, 2023

ggoodman commented Sep 15, 2023

debadree25 commented Sep 16, 2023

nodejs-github-bot commented Sep 16, 2023

nodejs-github-bot commented Sep 18, 2023

debadree25 commented Sep 18, 2023