Fix GH-17921 socket_read/socket_recv overflows on buffer size. #17923
Conversation
ext/sockets/tests/gh17921.phpt
Outdated
| --EXTENSIONS-- | ||
| sockets | ||
| --INI-- | ||
| memory_limit=-1 |
There was a problem hiding this comment.
Why did you add this INI setting?
There was a problem hiding this comment.
... I have Allowed memory size warning otherwise...
There was a problem hiding this comment.
Hm but these functions all return false 🤔 i.e. they shouldn't reach the code that allocates the memory?
Actually, you adjusted the checks incorrectly though after my review, so you will actually reach this allocation code
There was a problem hiding this comment.
So ... isn't ZSTR_MAX_LEN fit for zend_ulong ? length is a zend_long. ZSTR_MAX_LEN > ZEND_LONG_MAX
There was a problem hiding this comment.
Seems I was wrong adding the ini setting only.
There was a problem hiding this comment.
So ... isn't ZSTR_MAX_LEN fit for zend_ulong ? length is a zend_long. ZSTR_MAX_LEN > ZEND_LONG_MAX
Right, my bad, then this is fine
| @@ -884,7 +884,7 @@ PHP_FUNCTION(socket_read) | |||
| ENSURE_SOCKET_VALID(php_sock); | |||
|
|
|||
| /* overflow check */ | |||
| if ((length + 1) < 2) { | |||
| if (length <= 0 || length == ZEND_LONG_MAX) { | |||
There was a problem hiding this comment.
Actually, the max allowed string length is ZSTR_MAX_LEN, which is less than ZEND_LONG_MAX.
So you should change the checks to
| if (length <= 0 || length == ZEND_LONG_MAX) { | |
| if (length <= 0 || length > ZSTR_MAX_LEN) { |
There was a problem hiding this comment.
Similarly, the length check in socket_recvfrom should be adjusted.
devnexen
force-pushed
the
gh17921
branch
2 times, most recently
from
8b8f7e4 to
9262118
Compare
update the existing checks to be more straightforward instead of counting on undefined behavior.
update the existing checks to be more straightforward instead of counting on undefined behavior.