[v2,1/2,BZ,#16141] strptime: extend %z range limits
diff mbox

Message ID 1417573661-9902-1-git-send-email-james@loowit.net
State New
Headers show

Commit Message

James Perkins Dec. 3, 2014, 2:27 a.m. UTC
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. It supports timezone offsets between
UTC-12:00 and UTC+12:00, returning an error (NULL) for values outside
that range.

However, time zone offsets outside the current range limits exist both
currently and historically:

* Present day:
  - Pacific/Auckland (New Zealand) summer time +13:00
  - Pacific/Kiritimati (Christmas Island) +14:00
  - Pacific/Apia (Samoa) summer time +14:00
* Historical offsets exceeded +15:00/-15:00
* POSIX supports -24:00 to +25:00
* Paul Eggert notes:
  - https://sourceware.org/ml/libc-alpha/2014-12/msg00068.html

So, extend the timezone offset range to UTC-24:00 to UTC+25:00.

James

2014-12-02  James Perkins  james@loowit.net

	[BZ #16141]
	* time/strptime_l.c (__strptime_internal): Extend %z timezone
	offset range limits to UTC-24:00 to UTC+25:00 to parse current
	and historical uses.
---
 time/strptime_l.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Will Newton Dec. 3, 2014, 9:41 a.m. UTC | #1
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. It supports timezone offsets between
> UTC-12:00 and UTC+12:00, returning an error (NULL) for values outside
> that range.
>
> However, time zone offsets outside the current range limits exist both
> currently and historically:
>
> * Present day:
>   - Pacific/Auckland (New Zealand) summer time +13:00
>   - Pacific/Kiritimati (Christmas Island) +14:00
>   - Pacific/Apia (Samoa) summer time +14:00
> * Historical offsets exceeded +15:00/-15:00
> * POSIX supports -24:00 to +25:00
> * Paul Eggert notes:
>   - https://sourceware.org/ml/libc-alpha/2014-12/msg00068.html
>
> So, extend the timezone offset range to UTC-24:00 to UTC+25:00.
>
> James
>
> 2014-12-02  James Perkins  james@loowit.net
>
>         [BZ #16141]
>         * time/strptime_l.c (__strptime_internal): Extend %z timezone
>         offset range limits to UTC-24:00 to UTC+25:00 to parse current
>         and historical uses.
> ---
>  time/strptime_l.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

The patch looks ok modulo the nit below. It would be good to add a test however.

> diff --git a/time/strptime_l.c b/time/strptime_l.c
> index b3a612e..1cdce41 100644
> --- a/time/strptime_l.c
> +++ b/time/strptime_l.c
> @@ -777,7 +777,10 @@ __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM)
>                   return NULL;
>                 val = (val / 100) * 100 + ((val % 100) * 50) / 30;
>               }
> -           if (val > 1200)
> +           /* valid range UTC-24 to +25, ala POSIX */

The comment should be a sentence so start with a capital and end with
a full-stop and a couple of spaces.

> +           if (neg && val > 2400)
> +             return NULL;
> +           if (!neg && val > 2500)
>               return NULL;
>             tm->tm_gmtoff = (val * 3600) / 100;
>             if (neg)
> --
> 1.7.9.5
>
James Perkins Dec. 3, 2014, 10:28 p.m. UTC | #2
On Wed, Dec 3, 2014 at 1:41 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.
> The patch looks ok modulo the nit below. It would be good to add a test however.

I'm reworking it to add tests.

>> +           /* valid range UTC-24 to +25, ala POSIX */
>
> The comment should be a sentence so start with a capital and end with
> a full-stop and a couple of spaces.

Thanks for the feedback, Will. In my rework I will also drop the range limit per
discussion with Paul Eggert, to honor a -9959 to +9959 input offset range,
so the comment will go away.

Cheers,
James
Carlos O'Donell Dec. 4, 2014, 5:41 a.m. UTC | #3
On 12/03/2014 05:28 PM, James Perkins wrote:
> On Wed, Dec 3, 2014 at 1:41 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.
>> The patch looks ok modulo the nit below. It would be good to add a test however.
> 
> I'm reworking it to add tests.

Please keep in mind that as your change gets more complicated it will
eventually require copyright assignment.

Please review:
https://sourceware.org/glibc/wiki/Contribution%20checklist

All glibc developers should remind new contributors to look through
the contribution checklist :-)

Cheers,
Carlos.
James Perkins Dec. 4, 2014, 9:23 p.m. UTC | #4
On Wed, Dec 3, 2014 at 9:41 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> Please keep in mind that as your change gets more complicated it will
> eventually require copyright assignment.

Thank you, Carlos. I would agree that a Copyright assignment is required if I
contribute any creative work.

As it stands I've added a few lines to an array initializer in one
file and removed
more lines than I added in another, so I can safely say none of my patch content
is creative.

> Please review:
> https://sourceware.org/glibc/wiki/Contribution%20checklist

I am glad to say I read through once to determine how best to submit my
patches. It was very helpful in explaining the patch format and precise expected
content. I will make sure to read through it again before post my next
submission.

> All glibc developers should remind new contributors to look through
> the contribution checklist :-)

Agreed.

As this patch series represents my first contribution to glibc, I regret there
are preferences of the community I missed the first pass through. I'm
grateful for the constructive reviews I have received from Paul Eggert, Will
Newton, and yourself. I feel I've had an encouraging introduction.

Cheers,
James
Carlos O'Donell Dec. 4, 2014, 9:48 p.m. UTC | #5
On 12/04/2014 04:23 PM, James Perkins wrote:
> As this patch series represents my first contribution to glibc, I regret there
> are preferences of the community I missed the first pass through. I'm
> grateful for the constructive reviews I have received from Paul Eggert, Will
> Newton, and yourself. I feel I've had an encouraging introduction.

I'm very glad to hear that. I look forward to your contributions.

Cheers,
Carlos.

Patch
diff mbox

diff --git a/time/strptime_l.c b/time/strptime_l.c
index b3a612e..1cdce41 100644
--- a/time/strptime_l.c
+++ b/time/strptime_l.c
@@ -777,7 +777,10 @@  __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM)
 		  return NULL;
 		val = (val / 100) * 100 + ((val % 100) * 50) / 30;
 	      }
-	    if (val > 1200)
+	    /* 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;
 	    if (neg)