[v3,2/4,BZ,#16141] strptime: fix %z minutes calculation
Commit Message
This is part of a fix for [BZ #16141] strptime %z offset restriction.
strptime supports a %z input field descriptor, which parses a time zone
offset from UTC time into the broken-out time field tm_gmtoff.
In the current implementation, the minutes portion calculation is
correct only for minutes evenly divisible by 3. This is because the
minutes value is converted to decimal time, but inadequate precision
leads to rounding which calculates results that are too low for some
values.
For example, due to rounding, a +1159 offset string results in an
incorrect tm_gmtoff of 43128 (== 11 * 3600 + 58.8 * 60) seconds, instead
of 43140 (== 11 * 3600 + 59 * 60) seconds. In contrast, a +1157 offset
(minutes divisible by 3) does not cause the bug, and results in a
correct tm_gmtoff of 43020.
When 'make check' is run on glibc, tst-strptime2 will report the error
with the output:
round 9: tm_gmtoff is 43056
round 10: tm_gmtoff is 43128
This fix calculates the offset from the hour and minute portions
directly, without the rounding errors introduced by decimal time.
The tst-strptime2 test succeeds with this patch applied.
James
2014-12-03 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 03 Dec 2014 14:45, James Perkins wrote:
> --- a/time/strptime_l.c
> +++ b/time/strptime_l.c
>
> 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;
> if (val > 1200)
> return NULL;
> - tm->tm_gmtoff = (val * 3600) / 100;
> + tm->tm_gmtoff = (val / 100) * 3600 + (val % 100) * 60;
looks like you're adding a bug here with the 1200 check. you fix it later in
the patch series, but this follows the general policy of "don't add bugs in the
middle of a series". all in all, i think you should squash these 4 patches into
one.
-mike
@@ -770,16 +770,12 @@ __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;
if (val > 1200)
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;
}