Fix bug #75851: Year component overflow with date formats c, o, r and y#3028
Fix bug #75851: Year component overflow with date formats c, o, r and y#3028DaveRandom wants to merge 1 commit intophp:PHP-7.1from
Conversation
ext/date/php_date.c
Outdated
| case 'o': | ||
| if(!weekYearSet) { timelib_isoweek_from_date(t->y, t->m, t->d, &isoweek, &isoyear); weekYearSet = 1; } | ||
| length = slprintf(buffer, 32, "%d", (int) isoyear); break; /* iso year */ | ||
| length = slprintf(buffer, 32, "%lld", (zend_long) isoyear); break; /* iso year */ |
There was a problem hiding this comment.
I don't think you can just use %lld as I believe that doesn't work on Windows. PHP defines a few constants for this, such as ZEND_LONG_FMT_SPEC, used somewhere else in this file: https://github.com/php/php-src/blob/master/ext/date/php_date.c#L4451
There was a problem hiding this comment.
@derickr I tested this on Windows - I actually only tested it on W10 x64 as it was the nearest working build env I had - and it works fine there, but it's possible it doesn't work on 32-bit Windows (which is exactly why I opened the PR rather than just merging, I felt this needed review from someone more experienced). I had been hoping that Travis/Appveyor would provide some wider test feedback, but both envs failed for reasons unrelated to the content of the PR :-/
The choice of %lld was based on https://github.com/DaveRandom/php-src/blob/caed4b0387e95215817e6a5aea558bf6f985580a/ext/date/php_date.c#L1233, does this need migrating as well or is there something I'm missing?
There was a problem hiding this comment.
That is printing a timelib_sll, which uses %lld. Here you are using a zend_long, which uses ZEND_LONG_FMT.
There was a problem hiding this comment.
As both are, afaict, essentially aliases of the same type (64-bit signed integer) after a lot of tapdancing, would you mind explaining the functional difference here? I'm sure you're right, I'd just like to understand why
There was a problem hiding this comment.
timelib_sll is int64_t, zend_long is int64_t or int32_t, depending on platform.
ext/date/php_date.c
Outdated
| /* year */ | ||
| case 'L': length = slprintf(buffer, 32, "%d", timelib_is_leap((int) t->y)); break; | ||
| case 'y': length = slprintf(buffer, 32, "%02d", (int) t->y % 100); break; | ||
| case 'y': length = slprintf(buffer, 32, "%02lld", (zend_long) t->y % 100); break; |
There was a problem hiding this comment.
Printing a 2-digit number as long long doesn't make sense. Maybe you're looking to change the precedence of the cast? (int) (t->y % 100).
There was a problem hiding this comment.
You're absolutely right, will do that instead
DaveRandom
force-pushed
the
bugfix/75851
branch
from
caed4b0 to
8ba07f0
Compare
| --TEST-- | ||
| Test for bug #75851: Year component overflow with date formats "c", "o", "r" and "y" | ||
| --INI-- | ||
| date.timezone = UTC |
There was a problem hiding this comment.
This test needs a SKIPIF for 32-bit, like in the other test you modified.
|
Closed in favour of #3380 |
4 participants
No description provided.