-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix socket_recvfrom overflow on buffer size. #16382
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
Conversation
ab7afe8
to
85f45d1
Compare
85f45d1
to
e40cde0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from the test failure.
--EXTENSIONS-- | ||
sockets | ||
--FILE-- | ||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test fails on Windows, also a bit of a weird indentation: normally we don't indent tests outer scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that was just quick copy paste from my local messy test :)
@@ -1402,7 +1402,8 @@ PHP_FUNCTION(socket_recvfrom) | |||
|
|||
/* overflow check */ | |||
/* Shouldthrow ? */ | |||
if ((arg3 + 2) < 3) { | |||
|
|||
if (arg3 <= 0 || arg3 > ZEND_LONG_MAX - 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code also intended to disallow ZEND_LONG_MAX-1
, not sure why. I think what you did is fine though.
when passing PHP_INT_MAX for the $length param we get this (with ubsan) `ext/sockets/sockets.c:1409:36: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long int'`
e40cde0
to
3a861aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use PHP_OS_FAMILY == "Windows"
, which is simpler, but what you did is fine too.
when passing PHP_INT_MAX for the $length param we get this (with ubsan)
ext/sockets/sockets.c:1409:36: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long int'