Skip to content

Fixed int32 underflow on DateTime::createFromTimestamp - #12775

Merged
derickr merged 5 commits into
php:masterfrom
marc-mabe:fix-createFromTimestamp-32bit
Feb 27, 2024
Merged

Fixed int32 underflow on DateTime::createFromTimestamp#12775
derickr merged 5 commits into
php:masterfrom
marc-mabe:fix-createFromTimestamp-32bit

Conversation

@marc-mabe

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

Copy link
Copy Markdown
Contributor

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

iluuu1994 commented Nov 25, 2023

Copy link
Copy Markdown
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

marc-mabe commented Dec 8, 2023

Copy link
Copy Markdown
Contributor Author

Ping @derickr

@derickr

derickr commented Dec 9, 2023

Copy link
Copy Markdown
Member

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

marc-mabe commented Dec 9, 2023

Copy link
Copy Markdown
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

@derickr derickr left a comment

Copy link
Copy Markdown
Member

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

@iluuu1994 iluuu1994 left a comment

Copy link
Copy Markdown
Member

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!

Comment thread ext/date/php_date.h Outdated
Comment thread ext/date/php_date.h Outdated
Comment thread ext/date/tests/createFromTimestamp.phpt Outdated
Comment thread 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

@iluuu1994 iluuu1994 Dec 18, 2023

Copy link
Copy Markdown
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.

@marc-mabe marc-mabe Dec 18, 2023

Copy link
Copy Markdown
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.

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

Copy link
Copy Markdown
Contributor Author

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

@iluuu1994 iluuu1994 left a comment

Copy link
Copy Markdown
Member

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.

Comment thread 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",

@iluuu1994 iluuu1994 Dec 20, 2023

Copy link
Copy Markdown
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 ...

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

Copy link
Copy Markdown
Contributor Author

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?

@marc-mabe marc-mabe Dec 20, 2023

Copy link
Copy Markdown
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

marc-mabe commented Feb 3, 2024

Copy link
Copy Markdown
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
@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