Fix GH-14709 overflow on recurrences for DatePeriod::__construct #14710

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

No description provided.

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

Note : apparently needs adaptation with higher branches.

nielsdos

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.

nielsdos · 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

ext/date/php_date.c

 		zend_string *func = get_active_function_or_method_name();
-		zend_throw_exception_ex(NULL, 0, "%s(): Recurrence count must be greater than 0", ZSTR_VAL(func));
+		zend_throw_exception_ex(NULL, 0, "%s(): Recurrence count must be between 1 and %d", ZSTR_VAL(func), INT_MAX);


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

ext/date/tests/gh14709.phpt

+Bug GH-14709 overflow on reccurences parameter
+--SKIPIF--
+<?php
+if (PHP_INT_SIZE != 8) die("skip this test is for 64bit platform only");


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

ext/date/php_date.c

 		zend_string *func = get_active_function_or_method_name();
-		zend_throw_exception_ex(date_ce_date_malformed_period_string_exception, 0, "%s(): Recurrence count must be greater than 0", ZSTR_VAL(func));
+		zend_throw_exception_ex(NULL, 0, "%s(): Recurrence count must be greater or equal to 1 and lower than " ZEND_LONG_FMT, ZSTR_VAL(func), max_recurrences + 1);


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

ext/date/php_date.c

 		zend_string_release(func);
 		return false;
 	}

+


nit: No need to add empty lines

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

ext/date/php_date.c

+
+	if (UNEXPECTED(recurrences > max_recurrences)) {
+		zend_string *func = get_active_function_or_method_name();
+		zend_throw_exception_ex(NULL, 0, "%s(): Recurrence count must be greater or equal to 1 and lower than " ZEND_LONG_FMT " (including options)", ZSTR_VAL(func), max_recurrences + 1);


Also use the specific extension here again.

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

ext/date/tests/gh14709.phpt

+try {
+	new DatePeriod($start, $interval, 2147483640);
+} catch (Exception $e) {
+	echo $e->getMessage() . PHP_EOL;


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

ext/date/tests/date_period_bad_iso_format.phpt

-Exception: DatePeriod::createFromISO8601String(): Recurrence count must be greater or equal to 1 and lower than %d
+DateMalformedPeriodStringException: DatePeriod::__construct(): Recurrence count must be greater or equal to 1 and lower than %d
+DateMalformedPeriodStringException: DatePeriod::createFromISO8601String(): Recurrence count must be greater or equal to 1 and lower than %d
+


nit: extraneous line at end of file

close phpGH-14710

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 nielsdos June 29, 2024 15:23

nielsdos 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

nielsdos 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 kocsismate, dstogov, arnaud-lb, bukka, TimWolla, zeriyoshi and SakiTakamachi as code owners December 12, 2024 17:49

github-actions bot added Extension: bcmath Category: Optimizer ABI break labels Dec 12, 2024

devnexen removed request for adoy, bukka, TimWolla, youkidearitai, arnaud-lb, alexdowad, dstogov, NattyNarwhal, kocsismate, kamil-tekiela, zeriyoshi and SakiTakamachi 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

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

charmitro pushed a commit to wasix-org/php that referenced this pull request Mar 13, 2025

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

c342b92

close phpGH-14710

Fix GH-14709 overflow on recurrences for DatePeriod::__construct #14710

Fix GH-14709 overflow on recurrences for DatePeriod::__construct #14710

Uh oh!

Conversation

devnexen commented Jun 29, 2024

Uh oh!

devnexen commented Jun 29, 2024

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

nielsdos 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!

Uh oh!

derickr left a comment •

edited

Loading