[v2,2/2,BZ,#16141] strptime: fix %z minutes calculation
Commit Message
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(-)
Comments
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
@@ -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;
}