Fix GH-14709 overflow on recurrences for DatePeriod::__construct by devnexen · Pull Request #14710 · php/php-src

devnexen · 2024-06-29T05:47:22Z

No description provided.

devnexen · 2024-06-29T05:47:51Z

Note : apparently needs adaptation with higher branches.

ndossche

This is more or less right: recurrences is an int in _php_period_obj so we can't store a higher number. The issue isn't fully solved though because if include_start_date and include_end_date are set then you still get an integer overflow because you can add 2 to INT_MAX.

ndossche · 2024-06-29T15:53:30Z

From a technical PoV this is right and stops the undefined behaviour.
Me being pragmatic I think I would've just capped it at INT_MAX-2 because we don't lose much here. It would be a simpler patch.
I don't know what Derick prefers, I'm leaving the decision to him as this is his ext, not mine.

devnexen · 2024-06-29T15:55:56Z

in fact I tried to separate to let know of the second edge case separately but yes it s not so urgent anyway we ll see :).

derickr

I'm with @nielsdos here — capping it at INT_MAX - 8 (in case we get further options), seems perfectly cromulent.

derickr · 2024-09-11T14:24:42Z

between 1 is ambiguous between > 1 or >= 1. Please be precise.

That's standard wording (and always refers to the closed interval). cc @Girgias

We have always used it to mean an inclusive range AFAIK.

Might help to have helper functions for this, similar to what I did for ValueErrors which are empty.

should I revert back to the previous wording ?

Maybe wait a while on @derickr's opinion.

cmb69 · 2024-12-03T16:25:24Z

I think this is superfluous. Doesn't the test run fine on 32bit platforms, too?

I do not recall why, usually I do not disable for 32 bits just like this. I ll give it a go in a bit later.

seems working indeed, might had been just a case I forgot to remove after code changes.

cmb69 · 2024-12-03T19:10:32Z

This should not be applied to PHP-8.2 any longer.

devnexen · 2024-12-03T19:29:29Z

Ah good point, time flies :)

derickr

I think this looks OK as is, but please squash the commits first, and rebase for PHP 8.3.

derickr · 2024-12-16T12:41:39Z

I don't know why you removed the specific extension here? We had an RFC for this:

https://wiki.php.net/rfc/datetime-exceptions

s/extension/exception ?

ok I see, going to fix these..

derickr · 2024-12-16T12:41:41Z

nit: No need to add empty lines

derickr · 2024-12-16T12:41:51Z

Also use the specific extension here again.

derickr · 2024-12-16T12:43:30Z

Please add the exception class, like in the other tests, by using:

echo $e::class, ': ', echo $e->getMessage() . PHP_EOL;

derickr

LGTM, but please address my last nit. And then also please squash before merging.

derickr · 2024-12-20T13:08:53Z

nit: extraneous line at end of file

devnexen requested a review from derickr as a code owner June 29, 2024 05:47

github-actions Bot added the Extension: date label Jun 29, 2024

devnexen force-pushed the gh14709 branch from 9dd8588 to 8317de6 Compare June 29, 2024 06:19

devnexen changed the title ~~Fix GH-14709 overflow on reccurences for DatePeriod::__construct~~ Fix GH-14709 overflow on recurrences for DatePeriod::__construct Jun 29, 2024

devnexen force-pushed the gh14709 branch from 8317de6 to dc1b026 Compare June 29, 2024 07:14

devnexen requested a review from ndossche June 29, 2024 15:23

ndossche reviewed Jun 29, 2024

View reviewed changes

devnexen force-pushed the gh14709 branch from dc1b026 to 5cb9f64 Compare June 29, 2024 15:50

derickr requested changes Sep 11, 2024

View reviewed changes

devnexen force-pushed the gh14709 branch from 5cb9f64 to 1eecc67 Compare November 2, 2024 05:14

devnexen requested a review from derickr November 2, 2024 05:39

devnexen mentioned this pull request Dec 3, 2024

Signed integer overflow in ext/date/php_date.c #14709

Closed

cmb69 linked an issue Dec 3, 2024 that may be closed by this pull request

Signed integer overflow in ext/date/php_date.c #14709

Closed

cmb69 reviewed Dec 3, 2024

View reviewed changes

devnexen force-pushed the gh14709 branch from 1eecc67 to 74f2d64 Compare December 3, 2024 18:40

ndossche mentioned this pull request Dec 6, 2024

Fix integer overflow UB in timelib #17060

Closed

derickr approved these changes Dec 12, 2024

View reviewed changes

devnexen force-pushed the gh14709 branch from 74f2d64 to 7b999fb Compare December 12, 2024 17:49

devnexen requested review from SakiTakamachi, TimWolla, arnaud-lb, bukka, dstogov, kocsismate and zeriyoshi as code owners December 12, 2024 17:49

github-actions Bot added Category: Optimizer ABI break labels Dec 12, 2024

devnexen removed request for NattyNarwhal, SakiTakamachi, TimWolla, adoy, alexdowad, arnaud-lb, bukka, dstogov, kamil-tekiela, kocsismate, youkidearitai and zeriyoshi December 12, 2024 17:50

TimWolla requested a review from derickr December 12, 2024 17:50

devnexen force-pushed the gh14709 branch from 7b999fb to d75ea22 Compare December 12, 2024 18:08

github-actions Bot added Extension: date and removed Category: Build System Category: Engine Extension: bcmath Category: Optimizer ABI break labels Dec 12, 2024

derickr requested changes Dec 16, 2024

View reviewed changes

derickr approved these changes Dec 20, 2024

View reviewed changes

Comment thread ext/date/tests/date_period_bad_iso_format.phpt Outdated

derickr Dec 20, 2024

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.

nit: extraneous line at end of file

Fix phpGH-14709 overflow on recurrences for DatePeriod::__construct

3f8e9c9

devnexen force-pushed the gh14709 branch from 7066f4e to 3f8e9c9 Compare December 20, 2024 14:38

devnexen closed this in 16c0e57 Dec 20, 2024

Conversation

devnexen commented Jun 29, 2024

Uh oh!

devnexen commented Jun 29, 2024

Uh oh!

ndossche left a comment

Choose a reason for hiding this comment

Uh oh!

ndossche commented Jun 29, 2024

Uh oh!

devnexen commented Jun 29, 2024

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmb69 commented Dec 3, 2024

Uh oh!

devnexen commented Dec 3, 2024

Uh oh!

derickr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

derickr left a comment •

edited

Loading