[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAKGWAO-X0gBKwAwnuxExAbEGjv2pfqbqj4UR5jZSL2MNX4sEig@mail.gmail.com>
Date: Mon, 16 Jul 2018 14:41:20 -0500
From: Will Dietz <w@...z.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] strptime: add basic support for '%s' (seconds
since epoch)
On Sun, Jul 15, 2018 at 7:11 PM, Rich Felker <dalias@...c.org> wrote:
> On Sat, Jul 14, 2018 at 10:14:50PM -0500, Will Dietz wrote:
>> Attached.
>>
>> Background/context:
>>
>> * http://www.openwall.com/lists/musl/2018/01/18/4
>
> This message has some context on how %s is problematic, especially
> with regard to the question of interpreting it with respect to a
> timezone, which is happening here in your patch.
>
>> * http://austingroupbugs.net/view.php?id=169#c283
>>
>> Seems to work on basic usage, but has not yet been thoroughly tested/vetted.
>> Sharing in case useful / good starting point regardless ;).
>>
>> Also available on github:
>> https://github.com/dtzWill/musl/tree/feature/strptime-s-fmt
>>
>> ~Will
>
>> From 8cc60ad0d982d2ef04c062372e1a459e984da22d Mon Sep 17 00:00:00 2001
>> From: Will Dietz <w@...z.org>
>> Date: Wed, 11 Jul 2018 13:08:22 -0500
>> Subject: [PATCH] strptime: add basic support for '%s' (seconds since epoch)
>>
>> ---
>> src/time/strptime.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/src/time/strptime.c b/src/time/strptime.c
>> index c54a0d8c..bec00368 100644
>> --- a/src/time/strptime.c
>> +++ b/src/time/strptime.c
>> @@ -5,6 +5,9 @@
>> #include <stddef.h>
>> #include <string.h>
>> #include <strings.h>
>> +#include "time_impl.h"
>> +
>> +struct tm *__localtime_r(const time_t *restrict, struct tm *restrict);
>>
>> char *strptime(const char *restrict s, const char *restrict f, struct tm *restrict tm)
>> {
>> @@ -119,6 +122,15 @@ char *strptime(const char *restrict s, const char *restrict f, struct tm *restri
>> min = 0;
>> range = 61;
>> goto numeric_range;
>> + case 's':
>> + if (!isdigit(*s)) return 0;
>> + else {
>
> The else seems spurious and weirdly formatted.
Eep, sorry. Was trying to find way to avoid declaring these at
function entry...
But looks like musl already assumes C99 so nevermind.
>
>> + char *new_s;
>> + time_t t = strtoull(s, &new_s, 10);
>
> Directly assigning to time_t precludes handling out-of-range values in
> any meaningful way. I think we need to both check for overflow in
> strtoull, and check that the value fits in time_t. Also it should
> probably be signed and accept negative values, but I'm not sure.
Okay, I've attached an updated patch that should address these concerns.
It also avoids strtoull since our needs are simple.
I opted to reject negative values, since my best reading
(corrections/comments welcome!)
is that the value must be positive.
(I imagine you know these references well, including mostly to explain
myself haha)
>From the austin link above:
-------------
After page 2027 line 64161 section strptime, add:
s The number of seconds since the Epoch as a
decimal number (see [xref to XBD 4.15 Seconds Since the
Epoch]); leading zeros shall be permitted but shall not be
required.
-------------
Particularly that "seconds since the epoch" citation [1] (err I guess
that's 4.16 now)
says a negative "seconds since epoch" is undefined-- which matches the
(optional)
behavior the austin resolution indicates for when
strftime(format="%s") encounters
a time where seconds since epoch would be negative.
(I suppose this is to make things easier if time_t was unsigned? Dunno...)
[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_16
>
> It seems like we don't handle this well anywhere else in strptime, but
> maybe field width limits avoid the overflow issue there.
>
>> + s = new_s;
>> + if (!__localtime_r(&t, tm)) return 0;
>> + }
>
> This looks okay modulo my inherent concern about %s vs time zones, but
> I don't think there's any better way it can be done...
>
Okay, great O:).
Quick general question, if you don't mind:
==========
errno handling:
I see no indication of if/when/how errno should be set for strptime --
would it be useful here to set things like EINVAL and EOVERFLOW for
friendliness?
Alternatively is it important we don't set errno?
Our invocation of __localtime_r will set it in some instances,
should that be handled or is just forwarding out to user okay?
Thanks!
~Will
View attachment "strptime-s.patch" of type "text/x-patch" (1332 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
