Use of metadata['unread_bytes'] in Socket.php may cause a receive() call to return too early

[sylbru]

I'm trying this library and using version 2 because the application I'm working on is written for PHP 5.4 (I know...), and while the move to PHP7 is planned, it's less urgent than my need to integrate WebSockets.

After spending a lot of time on trying to make all of this work, I stumbled on a weird issue where the script crashed because of this:

warning: Wrench\ConnectionManager: Wrong input arguments: exception 'InvalidArgumentException' with message 'Invalid request line'
The stack trace below this message showed that the onData method was called with the string "G" as the $data argument. I noticed the cause of this was that the Socket::receive() method returned prematurely because of the 'unread_bytes' value of $metadata being equal to 0 at that point.

Not knowing much about this, but reading in the PHP documentation for stream_get_meta_data() that "You shouldn't use this value in a script," I tried commenting out this whole if ($metadata && isset($metadata['unread_bytes'])) block.

And to my surprise, it worked flawlessly, at least for now. I can now connect to the server, send and receive messages.

Is that a bug, of simply me using the library the wrong way? Is this method (removing the block) a proper patch or will it likely cause other issues?

[dominics]

If it still passes the tests, and works in functional/manual testing, I'd be more than happy to consider a change. Certainly your fix seems reasonable; if we can't justify the use of unread_bytes with a test or documentation it shouldn't be there.

A lot of the weird stuff going on in the stream handling is cargo-culted or dates from the original implementation.

[nexen2]

You was lucky to send a message packet of bytes count equal to inner buffer length, so 'unread_bytes' value was zero.

I'll try to find out a solution. Just some free time needed.

[martijnpolak]

I have the exact same issue. The probleem only seems to occur for me when using SSL; for some reason when using SSL fread only reads 1 byte at a time and ignores the $length parameter, whereas when we don't use SSL it reads the entire $length correctly. Then unread_bytes is always 0 causing the loop to break prematurely.

If I comment out the entire metadata section it works. Note that this does require the "Workaround Chrome behavior" around line 302, otherwise the loop would still break prematurely (because strlen($result) != $length). Because of this comment I did try Firefox but it still ignored $length.

Would it be considered safe if we remove the metadata section for the time being? It seems like a fallback to prevent getting stuck in a loop, though feof() should suffice for this.

[nexen2]

All people who had problems connected with "Chrome behavior" or 'unread_bytes' check: plz download and try commit "82811a9e197b4ff11cccaba62e88c28ecfd907c6". Tested with SSL, both Chrome and Firefox. I hope all issues were solved.

Your reviews will be welcomed.

[martijnpolak]

Looking at it (82811a9) it wont fix my problem.

• The problem is that unread_bytes is 0, so it is set and the value === 0. This commit doesn't change that it will exit the loop prematurely because of this. Seeing as php.net suggest never using unread_bytes a better solution would imho be to drop the entire metadata section. However I'm not sure what its purpose is, to set the socket non blocking and prevent hangs due to a php bug? Is there another way to fix this, maybe always set it non blocking?
• The "Chrome behaviour" has been removed, but that's actually what makes it work for me. It isn't fixing an issue with Chrome, it's fixing the issue that fread over SSL (wss) ignores $length and always reads 1 character at a time. Without that fix it will break out of the loop after reading the first byte. Really that entire $continue bit is superfluous if you drop the unread_bytes part.

[nexen2]

unread_bytes does not reflect real unread data length, just part in the system cache. Real code change is just check it for > 0, without using this value directly. So I don't mind delete this check. May be its a wild guess by i think it is correct now.

Strange behavior about Chrome. Can you describe your environment such like OS, php build, Chrome version? BTW how did you tried to connect socket, directly or apache/nginx proxy?

[martijnpolak]

Alright, but mind you my problem has nothing to do with Chrome, I'm just calling it the "Chrome fix" because thats what it says in the code comment (line 302 "Workaround Chrome behaviour (still needed?)").

What I'm doing is:

• I have supervisor start a cli php process and keep it running.
• This process starts the Wrench server which listens on its own port (so I'm not using a proxy).
• Since I'm using SSL I reference the certificate and private key on the server, that works fine.
• In order to debug the problem I put an error_log('l:' . $length . ', r:' . $result) right after the fread($this->socket, $length)
• When I start the cli process without SSL it shows that fread reads up to 1400 characters and $result contains the entire request.
• When I start it using SSL I can see that $length is still 1400, but $result only contains 'G' (the first character of the request).

Now after this the following happens:

• $continue = false
• strlen($result) == 1 so $continue gets put to true (this is the so called "Chrome fix")
• strlen($result) != $length so this if gets skipped. I guess the idea is that if the entire $length was read that means that there's possibly more data that will require another read so it continues. However, since with SSL it only reads 1 byte this will never happen so it won't continue.
• Then the unread_bytes part: unread_bytes is 0, so we enter that part. Then we get if (!$metadata['unread_bytes']) { on line 314, which has now changed to if (empty($metadata['unread_bytes'])) { which is the same thing in this context. This evaluates to true because unread_bytes == 0. Now $continue == false and the loop ends.
• If I remove the whole bit from line 299 it works and the debug shows fread picking up a single character each time ('G', 'E', 'T', etc)

I have implemented a custom socket class in which I've removed the section and it seems to work just fine :)