Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed int32 underflow on DateTime::createFromTimestamp #12775

Merged
merged 5 commits into from
Feb 27, 2024
Merged

Fixed int32 underflow on DateTime::createFromTimestamp #12775

merged 5 commits into from
Feb 27, 2024

Conversation

marc-mabe
Copy link
Contributor

@marc-mabe marc-mabe commented Nov 24, 2023 •
edited
Loading

This fixes an integer underflow on passing PHP_INT_MIN - 0.X to DateTime[Immutable]::createFromTimestamp().
This is due to the fact that the fractional part is handled separately ...

	if (UNEXPECTED(usec < 0)) {
		sec = sec - 1;
		usec = 1000000 + usec;
	}

PS: PHP_INT_MAX + 0.X does not have this issue

Similar to ZEND_DOUBLE_FITS_LONG this is introducing PHP_DATE_DOUBLE_FITS_LONG based on TIMELIB_LONG_MIN/MAX

/cc @iluuu1994 @derickr

@marc-mabe marc-mabe requested a review from derickr as a code owner November 24, 2023 22:55
@marc-mabe marc-mabe force-pushed the fix-createFromTimestamp-32bit branch from 9c07b42 to 548fe60 Compare November 24, 2023 23:46
@iluuu1994
Copy link
Member

ext/date has more UB due to overflows (see ext/date/tests/gh-124.phpt). Derick should decide what to do with this case.

@marc-mabe marc-mabe force-pushed the fix-createFromTimestamp-32bit branch from 2f57d4e to 491db8b Compare November 25, 2023 10:39
@marc-mabe
Copy link
Contributor Author

Ping @derickr

@derickr
Copy link
Member

derickr commented Dec 9, 2023

The ASAN tests still seem to fail... I did try restarting it, but it's the same result. They need to get fixed before we can merge this.

@marc-mabe marc-mabe force-pushed the fix-createFromTimestamp-32bit branch from 491db8b to 26d3153 Compare December 9, 2023 08:14
@marc-mabe
Copy link
Contributor Author

@derickr it's fixed now. I have rebased against master and for some reason I was using SIZEOF_TIMELIB_LONG which doesn't exist.

About TIMELIB_LONG vs. ZEND_LONG can this ever differ? I have seen some logic to force 32bit in it or would it be better to simply fail compile if zend long != timelib long?

derickr
derickr reviewed Dec 18, 2023
View reviewed changes
Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

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

This LGTM, but I would want to have a second opinion. @iluuu1994 @kocsismate ?

iluuu1994
iluuu1994 approved these changes Dec 18, 2023
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

The code itself looks good to me. Thanks @marc-mabe!

ext/date/php_date.h Outdated Show resolved Hide resolved
ext/date/php_date.h Outdated Show resolved Hide resolved
ext/date/tests/createFromTimestamp.phpt Outdated Show resolved Hide resolved
ext/date/php_date.c Outdated
"Seconds must be a finite number between " TIMELIB_LONG_FMT " and " TIMELIB_LONG_FMT ", %g given",
TIMELIB_LONG_MIN,
TIMELIB_LONG_MAX,
sec_dval
Copy link
Member

Choose a reason for hiding this comment

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

Not printing usec would be confusing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mh to be precise the timestamp must be between TIMELIB_LONG_MIN.0 and TIMELIB_LONG_MAX.999999.
Should this be included in the message as well as currently it's about the seconds part only.

Copy link
Contributor Author

@marc-mabe marc-mabe Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have updated the message to Timestamp must be a finite number between {TIMELIB_LONG_MIN} and {TIMELIB_LONG_MAX}.999999, {ARGUMENT} given

iluuu1994
iluuu1994 approved these changes Dec 20, 2023
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM! @derickr Please merge if you are happy too.

ext/date/php_date.c Outdated
zend_throw_error(
date_ce_date_range_error,
"Seconds must be a finite number between " TIMELIB_LONG_FMT " and " TIMELIB_LONG_FMT ", %g given",
"Timestamp must be a finite number between " TIMELIB_LONG_FMT " and " TIMELIB_LONG_FMT ".999999, %g given",
Copy link
Member

Choose a reason for hiding this comment

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

This should probably use our default zend_argument_error format:

Argument #1 ($timestamp) must be a finite number between ...

Copy link
Contributor Author

@marc-mabe marc-mabe Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I thought it's better matching the DateRangeError event though the documentation needs to be updated. https://www.php.net/manual/en/class.daterangeerror.php

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I got it. This makes sense 👍
-> Updated the PR accordingly

@marc-mabe marc-mabe mentioned this pull request Dec 20, 2023
Merged
@marc-mabe
Copy link
Contributor Author

@derickr @iluuu1994 can this PR be merged now?

@iluuu1994 iluuu1994 requested a review from derickr February 6, 2024 16:00
@derickr derickr merged commit 9f586f6 into php:master Feb 27, 2024
8 checks passed
@marc-mabe marc-mabe deleted the fix-createFromTimestamp-32bit branch June 16, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iluuu1994 iluuu1994 iluuu1994 approved these changes

@kocsismate kocsismate Awaiting requested review from kocsismate

@derickr derickr Awaiting requested review from derickr derickr is a code owner

Assignees
No one assigned
Labels
Extension: date
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marc-mabe @iluuu1994 @derickr