Fix #81500: Interval serialization regression since 7.3.14 / 7.4.2 #7575
Conversation
While it may not be desired, `DateInterval::$f` supports negative values, at least with regard to calculations. We still need to guard from assigning double values which are out of range for signed 64bit integers (which would be undefined behavior). While we could restrict this only to values which are actually out of range, the marker for invalid µs is `-1000000`, so we cannot accept smaller numbers. To retain symmetry, we also reject values greater than `1000000`. We must not do that only for unserialization, but also when the property is set in the first place.
|
Sorry @cmb69 but this is still a different behavior than PHP < 8.1 and how second/minute/hour behaves. And actually it should be fine to represent an interval as |
|
I simply think PHP should not try to do any smart and fancy if-else tricks around inputs, it will just replacing existing bugs by even weirder bugs. |
ext/date/php_date.c
Outdated
| if (val > -1000000 && val < 1000000) { | ||
| (*intobj)->diff->us = val; | ||
| } |
There was a problem hiding this comment.
| if (val > -1000000 && val < 1000000) { | |
| (*intobj)->diff->us = val; | |
| } | |
| (*intobj)->diff->us = val; |
ext/date/php_date.c
Outdated
| double val = zval_get_double(value) * 1000000; | ||
| if (val > -1000000 && val < 1000000) { | ||
| obj->diff->us = val; | ||
| } else { | ||
| obj->diff->us = -1000000; | ||
| } |
There was a problem hiding this comment.
| double val = zval_get_double(value) * 1000000; | |
| if (val > -1000000 && val < 1000000) { | |
| obj->diff->us = val; | |
| } else { | |
| obj->diff->us = -1000000; | |
| } | |
| obj->diff->us = zval_get_double(value) * 1000000; |
|
@kylekatarnls, please read the commit message (first comment). We need to avoid triggering undefined behavior (implementation defined is okay-ish, but not undefined behavior). Thus we need to restrict the values here, although not necessarily that strict as I've done here. |
|
Maybe use zend_dval_to_lval? The implementation is technically still UB on x86, but it makes sure that the behavior is consistent across platforms, and will not trigger ubsan warnings. |
Do you mean just Lines 4278 to 4284 in 3657693 Instead of yielding false, it yields int(-1) (https://3v4l.org/AmubA), because value defaults to -1 instead of -99999, so -1000000 is not really invalid, and we could break existing code when changing this.
|
|
To be clear, the new limitation already broke existing code (I mean 8.1 RC3 disallow to use microseconds that are not in the range, while this is a fair usage and worked until this version). Also the previous bug was scoped to serialize/unserialize, so at least everything went fine as long as you did not serialize the interval. But if we apply the current change, the breaking change will impact way more users as it will no longer be scoped to only serialization. I would be favorable to just Then on serialization, optionally to check possible overflow, but I would recommend to actually target overflow boundaries, not limit to [0:1[ range. |
Codecov Report
@@ Coverage Diff @@
## PHP-7.4 #7575 +/- ##
===========================================
- Coverage 73.09% 73.09% -0.01%
===========================================
Files 803 803
Lines 283990 283998 +8
===========================================
+ Hits 207588 207592 +4
- Misses 76402 76406 +4
Continue to review full report at Codecov.
|
If overflow would occur, the value is set to `0`, what is standard PHP behavior anyway. This way we can avoid setting the invalid marker, which doesn't work as expected anyway. We need to adapt some of the existing tests wrt. this behavior. In particular, we check for an arbitrary value in bug79015.phpt, to cater to differences between 32bit and 64bit architectures.
No. After a more thorough check this cannot happen (it seems to me not even on x86); only unsigned to signed int conversion may overflow, but that is implementation defined behavior (not UB). |
While it may not be desired,
DateInterval::$fsupports negativevalues, at least with regard to calculations. We still need to guard
from assigning double values which are out of range for signed 64bit
integers (which would be undefined behavior). While we could restrict
this only to values which are actually out of range, the marker for
invalid µs is
-1000000, so we cannot accept smaller numbers. Toretain symmetry, we also reject values greater than
1000000. We mustnot do that only for unserialization, but also when the property is set
in the first place.