Upgrade to libuv version 1.46.0 broke connecting to abstract unix sockets

[ggoodman]

Version

>= 20.4.0

Platform

Linux b9efe00af536 5.15.49-linuxkit #1 SMP PREEMPT Tue Sep 13 07:51:32 UTC 2022 aarch64 GNU/Linux

Subsystem

net

What steps will reproduce the bug?

const http = require('http');

const SOCKET = '\0abstract';

// Create a local server to receive data from
const server = http.createServer((req, res) => {
  res.writeHead(200, { 'Content-Type': 'application/json' });
  res.end(
    JSON.stringify({
      data: 'Hello Worlds!',
    })
  );
});

server.listen(SOCKET, () => {
  const postData = JSON.stringify({
    msg: 'Hello World!',
  });

  const options = {
    socketPath: SOCKET,
    path: '/upload',
    method: 'POST',
    headers: {
      'Content-Type': 'application/json',
      'Content-Length': Buffer.byteLength(postData),
    },
  };

  const req = http.request(options, (res) => {
    console.log(`STATUS: ${res.statusCode}`);
    res.setEncoding('utf8');
    res.on('data', (chunk) => {});
    res.on('end', () => {
      process.exit(0);
    });
    res.on('error', (err) => {
      console.trace(err);
      process.exit(1);
    });
  });

  req.on('error', (err) => {
    console.trace(err);
    process.exit(1);
  });

  req.end(postData);
});

server.on('error', (err) => {
  console.trace(err);
  process.exit(1);
});

If you have access to docker to facilitate cross-version testing:

for version in "12" "16" "18" "20.3" "20"; do 
  docker run --rm --volume $(pwd)/abstract_test.js:/srv/abstract_test.js "node:${version}" node /srv/abstract_test.js
done

Output:

v12.22.12 STATUS: 200
v16.20.2 STATUS: 200
v18.17.1 STATUS: 200
v20.3.1 STATUS: 200
Trace: Error: listen EINVAL: invalid argument abstract

How often does it reproduce? Is there a required condition?

It requires 100% of the time.

What is the expected behavior? Why is that the expected behavior?

Abstract unix socket connections are possible.

What do you see instead?

See repro steps.

Additional information

In libuv/libuv#4030, we introduced uv_pipe_bind2 as a successor to uv_pipe_bind that now accepts a size_t representing the socket name length (since it starts with a NULL byte). In PipeWrap::Bind, we only ever call uv_pipe_bind:

node/src/pipe_wrap.cc

Lines 167 to 173 in 44084b8

	void PipeWrap::Bind(const FunctionCallbackInfo<Value>& args) {
	PipeWrap* wrap;
	ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
	node::Utf8Value name(args.GetIsolate(), args[0]);
	int err = uv_pipe_bind(&wrap->handle_, *name);
	args.GetReturnValue().Set(err);
	}

.

That means that the namelen argument to uv_pipe_bind2 will always default to the result of strlen(name). I suspect that strlen("\0anything") will always yield 0, resulting in EINVAL here: https://github.com/libuv/libuv/blob/0d78f3c758fdc41019093353a7ff4fed83005ac9/src/unix/pipe.c#L65-L66.

I think that the fix might be to detect strings with a leading "\0" and pass an explicit namelen to uv_pipe_bind2 in PipeWrap::Bind.

[santigimeno]

I think that the fix might be to detect strings with a leading "\0" and pass an explicit namelen to uv_pipe_bind2 in PipeWrap::Bind.

Sounds reasonable. Do you want to send a PR?

[ggoodman]

@santigimeno thanks for getting back so quickly. I'm not familiar with any of the C / C++ types and could use a hint on how to determine the correct length from name here:

node/src/pipe_wrap.cc

Lines 170 to 171 in 44084b8

	node::Utf8Value name(args.GetIsolate(), args[0]);
	int err = uv_pipe_bind(&wrap->handle_, *name);

I feel like there may be some subtleties here when going from utf8 to char* that I'd need to think about.

[santigimeno]

For the length, I guess name.length() should work. Also take into account that you should use uv_pipe_connect2() instead of uv_pipe_connect(). You can take a look at the tests in libuv/libuv#4030 for examples on how to use them.

[ggoodman]

I have the naive set of changes here. Currently running make after ./configure and 🤞🏼. I have no experience working w/ c / c++ in the last 20 years.

Any tips on validating changes before kicking off a PR?

[santigimeno]

Those changes are perfect imo. Use make -j [N] where you could set N to the number of cores, so it builds faster. The only thing missing would be adding a test in test/parallel folder. You can look for similar tests there (probably the ones starting with test-pipe-). Then make sure that the test passes by running: ./node test/parallel/test-pipe-your-new-test.js.