unix: fail with ENAMETOOLONG in `uv_pipe_*` by addaleax · Pull Request #1329 · libuv/libuv

addaleax · 2017-04-27T23:42:14Z

Fail with ENAMETOOLONG when the name of a Unix socket exceeds
sizeof(saddr.sun_path). Previously the path was just truncated,
which could result in nasty bugs, and even though that behaviour
has been always been around, it’s hard to imagine a situation in
which ending up with an incorrect path is better than not creating
a socket at all.

Ref: nodejs/node#12601
Ref: nodejs/node#12708

This is targeting master because I wasn’t sure whether you’d consider this a breaking change (you probably do because it’s documented that way).

Fail with ENAMETOOLONG when the name of a Unix socket exceeds `sizeof(saddr.sun_path)`. Previously the path was just truncated, which could result in nasty bugs, and even though that behaviour has been always been around, it’s hard to imagine a situation in which ending up with an incorrect path is better than not creating a socket at all. Ref: nodejs/node#12601 Ref: nodejs/node#12708

santigimeno · 2017-04-28T07:57:49Z

  memset(&saddr, 0, sizeof saddr);
-  strncpy(saddr.sun_path, pipe_fname, sizeof(saddr.sun_path) - 1);
-  saddr.sun_path[sizeof(saddr.sun_path) - 1] = '\0';
+  memcpy(saddr.sun_path, pipe_fname, name_len);


woudn't memcpy(saddr.sun_path, pipe_fname, name_len - 1); work too?

santigimeno · 2017-04-28T08:00:04Z

  memset(&saddr, 0, sizeof saddr);
-  strncpy(saddr.sun_path, name, sizeof(saddr.sun_path) - 1);
-  saddr.sun_path[sizeof(saddr.sun_path) - 1] = '\0';
+  memcpy(saddr.sun_path, name, name_len);


santigimeno · 2017-04-28T08:02:54Z

+  name_len = strlen(name) + 1;
+
+  if (name_len > sizeof(saddr.sun_path))
+    return -ENAMETOOLONG;


we could use UV_ENAMETOOLONG directly here

we tend to use -errno in src/unix

👍 Got it.

santigimeno · 2017-04-28T08:05:38Z

+  name_len = strlen(name) + 1;
+
+  if (name_len > sizeof(saddr.sun_path)) {
+    err = -ENAMETOOLONG;


s/-ENAMETOOLONG/UV_ENAMETOOLONG

saghul · 2017-04-28T08:27:56Z

-        Paths on Unix get truncated to ``sizeof(sockaddr_un.sun_path)`` bytes, typically between
-        92 and 108 bytes.
+        If a path on Unix exceeds ``sizeof(sockaddr_un.sun_path)`` bytes, typically between
+        92 and 108 bytes, ``uv_pipe_bind`` will fail with ``UV_ENAMETOOLONG``.


Can you add a .. versionchanged:: section?

saghul · 2017-04-28T08:28:05Z

-        Paths on Unix get truncated to ``sizeof(sockaddr_un.sun_path)`` bytes, typically between
-        92 and 108 bytes.
+        If a path on Unix exceeds ``sizeof(sockaddr_un.sun_path)`` bytes, typically between
+        92 and 108 bytes, ``uv_pipe_bind`` will fail with ``UV_ENAMETOOLONG``.


addaleax · 2017-04-28T13:13:06Z

@santigimeno @saghul addressed comments, PTAL

santigimeno · 2017-04-28T13:21:38Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/254/

santigimeno · 2017-04-28T13:26:56Z

Some relevant failures in the windows bots:

not ok 25 - pipe_connect_to_long_path
# exit code 3
# Output from process `pipe_connect_to_long_path`:
# Assertion failed in test\test-pipe-connect-error.c on line 61: status == UV_ENAMETOOLONG

not ok 107 - pipe_bind_error_long_path
# exit code 3
# Output from process `pipe_bind_error_long_path`:
# Assertion failed in test\test-pipe-bind-error.c on line 151: r == UV_ENAMETOOLONG

addaleax · 2017-04-28T13:30:07Z

Right, my bad, the tests should not have been added for Windows. Updated with #ifndef _WIN32 guards…

santigimeno · 2017-04-28T13:39:41Z

new CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/255/

santigimeno · 2017-04-28T14:01:09Z

Some aix failures:

not ok 25 - pipe_connect_to_long_path
# exit code 393222
# Output from process `pipe_connect_to_long_path`:
# Assertion failed in test/test-pipe-connect-error.c on line 61: status == UV_ENAMETOOLONG

not ok 25 - pipe_connect_to_long_path
# exit code 393222
# Output from process `pipe_connect_to_long_path`:
# Assertion failed in test/test-pipe-connect-error.c on line 61: status == UV_ENAMETOOLONG

addaleax · 2017-04-28T19:03:36Z

Okay, fixed up tests for AIX which seems to have an impressive sizeof(sockaddr_un.sun_path) value of 1023… could somebody start CI again?

santigimeno · 2017-04-28T19:12:07Z

Okay, fixed up tests for AIX which seems to have an impressive sizeof(sockaddr_un.sun_path) value of 1023… could somebody start CI again?

😮
CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/256/

addaleax · 2017-04-28T19:31:41Z

Ci is green \o/

santigimeno · 2017-04-28T19:57:57Z

AIX which seems to have an impressive sizeof(sockaddr_un.sun_path) value of 1023

I'm not sure but I'll ask, should we add this info to the docs? Right now we're saying that's typically between 92 and 108 bytes.

Regardless of that, still LGTM.

addaleax · 2017-04-28T20:12:07Z

Tbh, I can’t really say just how niche AIX is, so I am not sure it’s worth pointing that out or not (also, libuv is about cross-platform abstraction, so I guess it should be okay if users know they can only rely on 90 bytes or so being available…?).

santigimeno · 2017-04-28T20:17:58Z

libuv is about cross-platform abstraction, so I guess it should be okay if users know they can only rely on 90 bytes or so being available…?

Yes, it makes sense. Let's leave it as it is.

saghul

LGTM. Good work Anna!

Fail with ENAMETOOLONG when the name of a Unix socket exceeds `sizeof(saddr.sun_path)`. Previously the path was just truncated, which could result in nasty bugs, and even though that behaviour has been always been around, it’s hard to imagine a situation in which ending up with an incorrect path is better than not creating a socket at all. Refs: nodejs/node#12601 Refs: nodejs/node#12708 PR-URL: #1329 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

santigimeno · 2017-05-31T13:02:04Z

Landed in 66319cf. Thanks!

addaleax force-pushed the pipe-bind-enametoolong branch from 050f0f7 to 3da120b Compare April 27, 2017 23:50

santigimeno requested changes Apr 28, 2017

View reviewed changes

saghul requested changes Apr 28, 2017

View reviewed changes

[squash] address review comments

373a4d4

santigimeno approved these changes Apr 28, 2017

View reviewed changes

[squash] disable test on windows

a2aa352

cjihrig approved these changes Apr 28, 2017

View reviewed changes

[squash] account for large AIX unix socket filename limit

2852d87

saghul approved these changes Apr 29, 2017

View reviewed changes

cjihrig added the v2 label May 1, 2017

santigimeno closed this May 31, 2017

addaleax deleted the pipe-bind-enametoolong branch May 31, 2017 13:02

addaleax mentioned this pull request Jun 14, 2017

net: socket file names are being truncated, and may become unremovable nodejs/node#13670

Closed

Conversation

addaleax commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 28, 2017

Uh oh!

santigimeno commented Apr 28, 2017

Uh oh!

santigimeno commented Apr 28, 2017

Uh oh!

addaleax commented Apr 28, 2017

Uh oh!

santigimeno commented Apr 28, 2017

Uh oh!

santigimeno commented Apr 28, 2017

Uh oh!

addaleax commented Apr 28, 2017

Uh oh!

santigimeno commented Apr 28, 2017

Uh oh!

addaleax commented Apr 28, 2017

Uh oh!

santigimeno commented Apr 28, 2017

Uh oh!

addaleax commented Apr 28, 2017

Uh oh!

santigimeno commented Apr 28, 2017

Uh oh!

saghul left a comment

Choose a reason for hiding this comment

Uh oh!

santigimeno commented May 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

addaleax commented Apr 27, 2017 •

edited

Loading