diff mbox

[v3,2/4,BZ,#16141] strptime: fix %z minutes calculation

Message ID 1417646760-19563-2-git-send-email-james@loowit.net
State New
Headers show

Commit Message

James Perkins Dec. 3, 2014, 10:45 p.m. UTC 
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

Mike Frysinger March 6, 2015, 9:51 a.m. UTC | #1 
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
diff mbox

Patch

diff --git a/time/strptime_l.c b/time/strptime_l.c
index b3a612e..be35f3b 100644
--- a/time/strptime_l.c
+++ b/time/strptime_l.c
@@ -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;
 	  }