feof() behavior change for UNIX based socket resources in PHP 8.2.

[ChadSikorra]

Description

I have been using feof() as a way to determine if a socket resource is actively open. In hindsight, I should've used is_resource for this it seems, as looking at notes for that it will return false if the resource is closed. Regardless, in PHP 8.2 the behavior of feof() changed for UNIX sockets. Unlike TCP based resources, it calling feof on it now returns true even while the resource connection is still open. But I don't see anything in the changelog / release notes describing any change in behavior to this method. If this change was intentional, it seems like it should be documented somewhere? However, since the behavior with TCP isn't officially documented, maybe it's an unfortunate wash 🤷

The code where I originally discovered this was while updating a personal project to verify PHP 8.2 compatibility: FreeDSx/LDAP#56, as that CI run shows that PHP 8.2 is the only version where the unix socket behavior is shown (it tests PHP 7.1 - 8.2 testing). I've simplified the behavior in the below script.

Given the following script:

<?php

$socket_path = '/tmp/test.socket';

if (file_exists($socket_path)) {
    unlink($socket_path);
}

$socket = stream_socket_server('unix://' . $socket_path);
var_dump('is connected? ' . (!feof($socket) ? 'true' : 'false'));

Resulted in this output (PHP 8.2):

string(19) "is connected? false"

But I expected this output instead (PHP <= 8.1):

string(18) "is connected? true"

The behavior for checking a TCP based socket resource did not change however. It seems to be specific to UNIX based sockets for some reason.

PHP Version

PHP 8.2

Operating System

Ubuntu 22.04.1

[nielsdos]

I can confirm the difference in behaviour, thanks for the report.
This is caused by commit 2d98631 in PR #8092. That commit introduced this behavioural change and the PR is meant to be an optimization, so the behavioural change looks unintentional. Therefore I think this probably is a regression.

[cmb69]

Therefore I think this probably is a regression.

I tend to agree. (And thank you for looking into this!)

[Girgias]

@MaxKellermann could you have a look at this?

[MaxKellermann]

Calling feof() on a stream_socket_server does not make sense.

Prior to my change, the poll() returned 0 (Timeout), and the recv() was never invoked:

socket(AF_UNIX, SOCK_STREAM, 0)         = 3
bind(3, {sa_family=AF_UNIX, sun_path="/tmp/test.socket"}, 18) = 0
listen(3, 32)                           = 0
poll([{fd=3, events=POLLIN|POLLPRI|POLLERR|POLLHUP}], 1, 0) = 0 (Timeout)

(PS: the poll() does not check for received data (which the caller intended to do), but instead it checks for incoming connections. Therefore, the return value depended on whether there was a pending connection that was not yet accepted. If so, recv() would be called, which would then throw an error due to EINVAL, just like my code path does.)

After my change, there's no poll(), and recv() is called right away, which however is an illegal operation on a listener socket.

socket(AF_UNIX, SOCK_STREAM, 0)         = 4
bind(4, {sa_family=AF_UNIX, sun_path="/tmp/test.socket"}, 18) = 0
listen(4, 32)                           = 0
recvfrom(4, 0x7ffc8981b648, 1, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = -1 EINVAL (Invalid argument)

The feof() call has always been illegal, it's just that prior to my change, the check that would have detected it was unreachable; not by design, but by accident.

This is indeed not an intentional change, but it's undefined behavior, so it's rather: the old return value was not intentional; it should always have returned an error.

If you want to restore the old behavior, you'd either need to revert my patch, or add special checks to ignore the error condition. I wouldn't do either; I'd just leave it as it is, and document that this call is (and always has been) illegal.

PS2: this is how it looks like (with PHP 7.4, i.e. prior to my change) if there was an incoming connection:

socket(AF_UNIX, SOCK_STREAM, 0)         = 3
bind(3, {sa_family=AF_UNIX, sun_path="/tmp/test.socket"}, 13) = 0
listen(3, 32)                           = 0
clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=2, tv_nsec=0}, 0x7ffd55272320) = 0
poll([{fd=3, events=POLLIN|POLLPRI|POLLERR|POLLHUP}], 1, 0) = 1 ([{fd=3, revents=POLLIN}])
recvfrom(3, 0x7ffd55272278, 1, MSG_PEEK, NULL, NULL) = -1 EINVAL (Invalid argument)
write(1, "string(19) \"", 12)           = 12
write(1, "is connected? false", 19)     = 19
write(1, "\"\n", 2)                     = 2

As you see, same error as with PHP 8.2!

[MaxKellermann]

Also see feof() documentation, https://www.php.net/manual/en/function.feof.php

stream
The file pointer must be valid, and must point to a file successfully opened by fopen() or fsockopen() (and not yet closed by fclose()).

The socket in this issue was not opened by fopen() or fsockopen(), therefore passing it to feof() is illegal.

[cmb69]

The socket in this issue was not opened by fopen() or fsockopen(), therefore passing it to feof() is illegal.

But why doesn't feof() warn (or even throw) in that case?

[MaxKellermann]

But why doesn't feof() warn (or even throw) in that case?

I guess the php_stream doesn't remember that it isn't a connection socket but a listener socket, therefore such a check cannot currently be implemented (without consulting the kernel, which costs a system call, which should be avoided).

I guess I could easily add such a (userspace) flag and add a warning - do you want that?
That would further deviate from PHP 8.1-, but would communicate that mistake clearly.
The old behavior was undefined and random, and that's bad, we shouldn't go back there.

[Girgias]

I suppose it might be possible to check the stream->ops->label field of a php_stream and see if calling feof() on it makes sense?

[MaxKellermann]

I suppose it might be possible to check the stream->ops->label

That would technically be possible, but appears rather fragile to me. What if new stream types get added? (By an extension?) Correct behavior should be ensured by good API design.

[MaxKellermann]

good API design

Regarding good API design: I'd like to question the idea that a listener socket is exposed as "stream" in PHP .... a listener socket is all but a stream. No data is ever transferred over it. Streams and listeners have nothing in common. Even if they are both file descriptors at the system call level, exposing them as the same type of object in a high-level language never made sense. This mixup is what causes the confusion about using feof() on a listener socket - not good API design.

I wonder if it would be possible to expose listeners as a different built-in class?

[cmb69]

I suppose it might be possible to check the stream->ops->label

That would technically be possible, but appears rather fragile to me. What if new stream types get added? (By an extension?) Correct behavior should be ensured by good API design.

Adding a new member would likely constitute an ABI break, in which case we cannot fix this for PHP 8.2 (master would be fine). Using a fragile workaround as a stop gap measure might be sensible.

I wonder if it would be possible to expose listeners as a different built-in class?

That would constitute a serious BC break, so cannot be done prior to PHP 9. Our long term goal is to get rid of all stream resources, but accomplishing that is hard (if not impossible) for practical reasons.

[MaxKellermann]

I suppose it might be possible to check the stream->ops->label field of a php_stream and see if calling feof() on it makes sense?

I checked - no, it doesn't work.

(gdb) p stream.ops.label
$5 = 0x5555566a6c43 "unix_socket"

Can't see whether this is a listener socket or a connection socket.