Skip to content

Fix GH-15034: Integer overflow on stream_notification_callback byte_max parameter with files bigger than 2GB #15035

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

Closed
wants to merge 2 commits into from

Conversation

nielsdos
Copy link
Member

We were using atoi, which is only for integers. When the size does not fit in an integer this breaks. Use ZEND_STRTOUL instead. Also make sure invalid data isn't accidentally parsed into a file size.

…e_max parameter with files bigger than 2GB

We were using atoi, which is only for integers. When the size does not
fit in an integer this breaks. Use ZEND_STRTOUL instead. Also make sure
invalid data isn't accidentally parsed into a file size.
@yortx
Copy link

yortx commented Jul 19, 2024

Hi @nielsdos . Thank you for reviewing this so quick!

I've seen your patch, and I have a little suggestion: to check that "parsed" is less than PHP_INT_MAX before setting it as the "file_size".
Since you are parsing the size to an "unsigned long" the result may not fit in a php integer, which is signed. I don't actually know what would receive the php function callback, but it will probably not be the correct result if the result doesn't fit in an php integer. I think it would be better not to set a file_size than set a wrong one.

That check would also avoid incorrect results when running on 32bit systems with PHP_INT_SIZE = 4. And since ZEND_STRTOUL seems to return MAX_ULONG in case the parsed number doesn't fit in a 64 ulong. That check would cover this case, too.

	if (endptr && !*endptr && parsed < ZEND_LONG_MAX) {
		file_size = parsed;
		php_stream_notify_file_size(context, file_size, http_header_line, 0);
	}

I'm not sure if that's the constant to use. It should be the equivalent of PHP_INT_MAX in the C side.

I'm not familiar with the php code, so please, double check my assumptions.
Hope this helps.

@nielsdos
Copy link
Member Author

nielsdos commented Jul 19, 2024

You are indeed right that it will cause issues on e.g. 32-bit platforms, as it already does now. I had thought about adding such a check when I wrote the patch but wasn't too sure about it. The reason is that adding the check will stop a notification event from happening which is a behaviour change, so I assumed we can only add that extra check on the master branch.
I'll let @bukka decide what's the most appropriate here.

I'm not sure if that's the constant to use. It should be the equivalent of PHP_INT_MAX in the C side.

Yes this constant is right.

@yortx
Copy link

yortx commented Jul 20, 2024

I haven't thought about that. I agree that the lack of notification is not the better idea. What about just setting file_size to PHP_INT_MAX in the cases it would overflow?

  • Shouldn't break any current code (I hope nobody relies in getting a negative file size).
  • The value would be "more correct" than the current result. The best we can do with the current API.
  • If documented, it would be easy to check for that condition in the php code.
  • It's the behavior of the ZEND_STRTOUL, so it shouldn't be unexpected.
if (endptr && !*endptr) {
    file_size = parsed < ZEND_LONG_MAX ? parsed :  ZEND_LONG_MAX;
    php_stream_notify_file_size(context, file_size, http_header_line, 0);
}

@nielsdos
Copy link
Member Author

What about just setting file_size to PHP_INT_MAX in the cases it would overflow?

I'm open to this, I think this would indeed be better, although a behaviour change as well... I'll leave the choice to Jakub.

@bukka
Copy link
Member

bukka commented Jul 21, 2024

I think it's fine to trim it to PHP_INT_MAX. No one should be using 32bit anyway so I don't particularly care that much... :)

@nielsdos
Copy link
Member Author

I think it's fine to trim it to PHP_INT_MAX. No one should be using 32bit anyway so I don't particularly care that much... :)

That's fair, I pushed that now.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nielsdos nielsdos closed this in cfcc2a3 Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integer overflow on stream_notification_callback byte_max parameter with files bigger than 2GB
3 participants