Skip to content
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

Fixed off-by-one error in checking length of abstract namespace Unix sockets #14483

Merged

Conversation

derickr
Copy link
Member

@derickr derickr commented Jun 5, 2024

I was trying to use PHP to talk to Xdebug's new out-of-band control sockets, which use the abstract Unix domain socket namespace (socket names starting with a \0 character).

Xdebug uses names that are the maximum allowed length for these socket names (108 bytes).

However, when I tried to run:

<?php
$name = "\0xdebug-ctrl.2078073yxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
echo strlen($name), "\n";
$f = stream_socket_client("unix://$name");

I got the following output and error:

108

Notice: stream_socket_client(): socket path exceeded the maximum allowed length of 108 bytes and was truncated in Standard input code on line 2

But the path is exactly 108 characters, not more. It turned out that the check for this maximum length is wrong.

No new test can easily be created, as the 108 is implementation specific, and ext/standard/tests/streams/bug60106.phpt already covers it.

With this change, I can now correctly connection to these sockets.

@nielsdos
Copy link
Member

nielsdos commented Jun 5, 2024

The way I'm reading https://man7.org/linux/man-pages/man7/unix.7.html it seems that the >= check should be used for non-abstract namespace sockets while > should be used for abstract namespace sockets.

I'm specifically referring to the section "Three types of address are distinguished in the sockaddr_un structure:"
which says:

a UNIX domain socket can be bound to a null-terminated
filesystem pathname using bind(2).

This means that we must take into account the space for the NUL terminator in the case of a non-abstract namespace socket.

EDIT: I think you need something like this instead https://gist.github.com/nielsdos/ee33321f88632a8c228e6d3c0efcdf5b

@iluuu1994
Copy link
Member

I read it the same way as @nielsdos, his Gist looks correct to me.

@derickr derickr force-pushed the fix-anonymous-socket-at-length-boundary branch from 8c2b70f to 9f005f8 Compare June 6, 2024 15:26
Bug #60106 (stream_socket_server with abstract unix unix socket paths)
--SKIPIF--
<?php
if( substr(PHP_OS, 0, 3) == "WIN" )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like either macOS nor FreeBSD support abstract socket names. So you should likely skip this test for everything but Linux.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I forgot about that. I'm sad that OSX/FBSD don't support it though.

@derickr derickr force-pushed the fix-anonymous-socket-at-length-boundary branch from 9f005f8 to ad56ec7 Compare June 13, 2024 11:46
@derickr derickr merged commit e0e9eb4 into php:PHP-8.2 Jun 13, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants