unix: support long unix paths on macos + bsds #2917
Conversation
| union uv__sockaddr_un { | ||
| struct sockaddr sa; | ||
| struct uv__sockaddr_un_path up; | ||
| }; |
There was a problem hiding this comment.
Is offsetof a legal constexpr for C89? I'm not sure the type-punning (via the union) is desirable either, as it's not legal C code, though we are intending to turn off TBAA.
I wonder if we should instead use the normal struct definition, then add a storage type with the extra padding:
| union uv__sockaddr_un { | |
| struct sockaddr sa; | |
| struct uv__sockaddr_un_path up; | |
| }; | |
| union uv__sockaddr_un_storage { | |
| struct sockaddr_un sa; | |
| #ifdef SOCK_MAXADDRLEN | |
| /* macOS and the BSDs support paths > sizeof(su.sun_path). | |
| * SOCK_MAXADDRLEN is, to the best of my knowledge, always 255. | |
| * Minus the two byte header, that leaves 253 bytes for the path. | |
| */ | |
| char up[SOCK_MAXADDRLEN]; | |
| #else | |
| char up[sizeof(struct sockaddr_un)]; | |
| #endif | |
| }; |
elsewhere, then, instead of sizeof(s->up.path) then, it'd be either sizeof(s->sa.path) + sizeof(s-> up) - sizeof(s->sa) or equivalently sizeof(s->up) - offsetof(struct sockaddr_un, sun_path)
There was a problem hiding this comment.
Sorry @vtjnash, missed your comment.
Is offsetof a legal constexpr for C89?
Yes, IMO (compiler builtin), and easy enough to fix if someone somewhere is using an archaic compiler.
I'm not sure the type-punning (via the union) is desirable either
I suspect that's an academic concern because the only aliasing is between struct sockaddr and char pad[]. Even if it's not theoretic, libuv is built with -fno-strict-aliasing now.
Your suggestion would work but it feels like it has a larger margin for error so I'd rather stick with my approach.
The BSDs let you pass path names longer than sizeof(s.sun_path), they read past the end when socklen > sizeof(struct sockaddr_un) up to a limit of at least 255 characters. This commit refactors src/unix/pipe.c to make it easy to support long paths. The actual support will be done in a follow-up commit. Refs: libuv#2826
See the previous commit. This commit allows paths up to 253 bytes instead of the current restriction of 104 bytes. Fixes: libuv#2826
|
Not stale, waiting on feedback on #2917 (comment). |
|
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
|
I don't use macos or any of the bsds anymore so I don't expect I'll be fixing this up anytime soon. Closing. |
Fixes: #2826
CI: https://ci.nodejs.org/job/libuv-test-commit/1953/