Message ID | 1417573661-9902-2-git-send-email-james@loowit.net |
---|---|
State | New |
Headers | show |
On 3 December 2014 at 02:27, James Perkins <james@loowit.net> wrote: > This is a fix for [BZ #16141] strptime %z offset restriction. > > strptime supports the parsing of a timezone offset from UTC time into > the broken-out time field tm_gmtoff. The offset includes an hour portion > and an optional minute portion (ex. -08 or -0800 means eight hours > before UTC, whereas -0830 means eight hours and thirty minutes before > UTC). > > However, in the current implementation, the minutes portion calculation > is correct only for minutes portion values evenly divisible by 3. This > is because the minutes value is converted to decimal time, and not > enough precision is used for rounding to cause incorrect results. For > example, a +1159 offset string results in a tm_gmtoff of 43128 (== 11 * > 3600 + 3528) seconds, instead of 43140 (== 11 * 3600 + 59 * 60) seconds. > > This fix calculates the offset from the hour and minute portions directly, > without the rounding errors introduced by decimal time. > > James > > 2014-12-02 James Perkins james@loowit.net > > [BZ #16141] > * time/strptime_l.c (__strptime_internal): Fix %z minutes > calculation, removing incorrect decimal time rounding, so that > all minute values result in a valid seconds value > --- > time/strptime_l.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/time/strptime_l.c b/time/strptime_l.c > index 1cdce41..659fd91 100644 > --- a/time/strptime_l.c > +++ b/time/strptime_l.c > @@ -770,19 +770,15 @@ __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM) > else if (n != 4) > /* Only two or four digits recognized. */ > return NULL; > - else > - { > - /* We have to convert the minutes into decimal. */ > - if (val % 100 >= 60) > - return NULL; > - val = (val / 100) * 100 + ((val % 100) * 50) / 30; > - } > + else if (val % 100 >= 60) > + /* minutes valid range is 0 through 59. */ Comment should start with a capital and have two spaces at the end. > + return NULL; > /* valid range UTC-24 to +25, ala POSIX */ > if (neg && val > 2400) > return NULL; > if (!neg && val > 2500) > return NULL; Are these conditions still correct? It would be good to add a test case or two to ensure the changes work as intended.
On Wed, Dec 3, 2014 at 1:46 AM, Will Newton <will.newton@linaro.org> wrote: > On 3 December 2014 at 02:27, James Perkins <james@loowit.net> wrote: >> This is a fix for [BZ #16141] strptime %z offset restriction. >> + /* minutes valid range is 0 through 59. */ > > Comment should start with a capital and have two spaces at the end. Happy to oblige, Will. I will rework the comment. >> /* valid range UTC-24 to +25, ala POSIX */ >> if (neg && val > 2400) >> return NULL; >> if (!neg && val > 2500) >> return NULL; > > Are these conditions still correct? No, the conditions are not correct; after discussion with Paul Eggert I propose that the code drop the range limit entirely and support -9959 to +9959, converting these to the appropriate seconds tm_gmtoff field. > It would be good to add a test case or two to ensure the changes work > as intended. I'm adding test cases for the existing errors as well as the range changes. Cheers, James
diff --git a/time/strptime_l.c b/time/strptime_l.c index 1cdce41..659fd91 100644 --- a/time/strptime_l.c +++ b/time/strptime_l.c @@ -770,19 +770,15 @@ __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM) else if (n != 4) /* Only two or four digits recognized. */ return NULL; - else - { - /* We have to convert the minutes into decimal. */ - if (val % 100 >= 60) - return NULL; - val = (val / 100) * 100 + ((val % 100) * 50) / 30; - } + else if (val % 100 >= 60) + /* minutes valid range is 0 through 59. */ + return NULL; /* valid range UTC-24 to +25, ala POSIX */ if (neg && val > 2400) return NULL; if (!neg && val > 2500) return NULL; - tm->tm_gmtoff = (val * 3600) / 100; + tm->tm_gmtoff = (val / 100) * 3600 + (val % 100) * 60; if (neg) tm->tm_gmtoff = -tm->tm_gmtoff; }