Fix GH-16258 overflow on jddayofweek argument. #16263
Conversation
ext/calendar/dow.c
Outdated
| @@ -35,6 +35,10 @@ int DayOfWeek( | |||
| { | |||
| int dow; | |||
|
|
|||
| if ((sdn + 1) > INT_MAX) { | |||
There was a problem hiding this comment.
zend_long can be the same as int; in which case this check is removed by the compiler because it'll be UB.
Note that you can just change the formula and the if-checks a bit because we're working in mod 7 to avoid the overflow; and avoid needing to check the overflow in the first place.
ac8cd42 to
735f7fa
Compare
ext/calendar/dow.c
Outdated
| int dow; | ||
|
|
||
| dow = (sdn + 1) % 7; | ||
| dow = (int)(sdn % 7 + 1); | ||
| if (dow >= 0) { | ||
| return (dow); | ||
| return (dow % 7); | ||
| } else { | ||
| return (dow + 7); | ||
| return (dow + 7) % 7; | ||
| } |
There was a problem hiding this comment.
Can't we just do:
return (sdn % 7 + 8) % 7;instead of what we have now?
There was a problem hiding this comment.
I mean, it is somewhat obscure, and likely not what @nielsdos had in mind. But I think it should work.
There was a problem hiding this comment.
it does work. I think the changed old test, the values make more sense to me.
There was a problem hiding this comment.
Without that (int) cast, bug_53574_2.phpt succeeds for me; either remove the cast, or cast the whole return value to (int) (that shouldn't be necessary since the % 7 ensures that values fit into int, but if some tool complains, we may need to cast).
There was a problem hiding this comment.
Double modulus works and is intuitive as well. It should be possible to avoid the double modulus too but it likely doesn't matter for performance.
No description provided.