Repository navigation
feof() behavior change for UNIX based socket resources in PHP 8.2. #10406
Description
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
Activity
ndossche commentedon Jan 21, 2023
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 commentedon Jan 22, 2023
I tend to agree. (And thank you for looking into this!)
Girgias commentedon Jan 23, 2023
@MaxKellermann could you have a look at this?
MaxKellermann commentedon Jan 23, 2023
Calling
feof()on astream_socket_serverdoes not make sense.Prior to my change, the
poll()returned0 (Timeout), and therecv()was never invoked:(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 toEINVAL, just like my code path does.)After my change, there's no
poll(), andrecv()is called right away, which however is an illegal operation on a listener socket.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:
As you see, same error as with PHP 8.2!
MaxKellermann commentedon Jan 23, 2023
Also see
feof()documentation, https://www.php.net/manual/en/function.feof.phpThe socket in this issue was not opened by
fopen()orfsockopen(), therefore passing it tofeof()is illegal.cmb69 commentedon Jan 23, 2023
But why doesn't
feof()warn (or even throw) in that case?MaxKellermann commentedon Jan 23, 2023
I guess the
php_streamdoesn'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 commentedon Jan 25, 2023
I suppose it might be possible to check the
stream->ops->labelfield of aphp_streamand see if callingfeof()on it makes sense?MaxKellermann commentedon Jan 25, 2023
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 commentedon Jan 25, 2023
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 commentedon Jan 25, 2023
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.
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 commentedon Jan 27, 2023
I checked - no, it doesn't work.
Can't see whether this is a listener socket or a connection socket.
Fix phpGH-10406: fgets on a redis socket connection fails on PHP 8.3
Fix GH-10406: fgets on a redis socket connection fails on PHP 8.3