Fix GH-16234 jewishtojd overflow on year argument. #16243
Conversation
|
|
||
| /* Start with the time of the first molad after creation. */ | ||
| r1 = NEW_MOON_OF_CREATION; | ||
| chk = (zend_long)metonicCycle; |
There was a problem hiding this comment.
Note that if zend_long is 32-bit longs, you still have an issue.
Furthermore, I think the failure should be propagated instead of silently set to 0.
There was a problem hiding this comment.
Furthermore, I think the failure should be propagated instead of silently set to 0.
That sounds more a change for master ?
ext/calendar/jewish.c
Outdated
| chk = (zend_long)metonicCycle; | ||
|
|
||
| if (chk == ZEND_LONG_MAX || | ||
| chk * (HALAKIM_PER_METONIC_CYCLE & 0xFFFF) == ZEND_LONG_MAX || |
There was a problem hiding this comment.
This multiplication can still overflow, no?
devnexen
force-pushed
the
gh16234
branch
2 times, most recently
from
d6658c5 to
d426d7f
Compare
|
|
||
| /* Calculate metonicCycle * HALAKIM_PER_METONIC_CYCLE. The upper 32 | ||
| * bits of the result will be in r2 and the lower 16 bits will be | ||
| * in r1. */ | ||
| r1 += metonicCycle * (HALAKIM_PER_METONIC_CYCLE & 0xFFFF); | ||
| r1 += chk * (HALAKIM_PER_METONIC_CYCLE & 0xFFFF); |
There was a problem hiding this comment.
This can still overflow, because r1 starts with NEW_MOON_OF_CREATION.
Analogous for r2.
There was a problem hiding this comment.
It still isn't right...
You essentially need to check if r1 > ZEND_LONG_MAX which isn't possible to write of course, but if we look at it mathematically:
r1 = NEW_MOON_OF_CREATION + chk * (HALAKIM_PER_METONIC_CYCLE & 0xFFFF)
and so
NEW_MOON_OF_CREATION + chk * (HALAKIM_PER_METONIC_CYCLE & 0xFFFF) > ZEND_LONG_MAX
<=> chk * (HALAKIM_PER_METONIC_CYCLE & 0xFFFF) > ZEND_LONG_MAX - NEW_MOON_OF_CREATION
<=> chk > (ZEND_LONG_MAX - NEW_MOON_OF_CREATION)/(HALAKIM_PER_METONIC_CYCLE & 0xFFFF)
This isn't what you wrote and I don't think it's equivalent. Now you should do the same for r2.
There was a problem hiding this comment.
You misunderstood the notation. In mathematics <=> means "equivalent". So I meant that the last expression was the one that should be tested and the steps before that just show how I arrived there.
You need something like this: https://gist.github.com/nielsdos/527062ecc725d34c5641b48d3450e1f5
No description provided.