Re: [PATCH] strptime: add basic support for '%s' (seconds since epoch)

[Will Dietz <w@wdtz.org>]

[-- Attachment #1: Type: text/plain, Size: 4381 bytes --]

On Sun, Jul 15, 2018 at 7:11 PM, Rich Felker <dalias@libc.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@wdtz.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

[-- Attachment #2: strptime-s.patch --]
[-- Type: text/x-patch, Size: 1332 bytes --]

From cb1cbddaca065e3deba508bf867004e20c10e824 Mon Sep 17 00:00:00 2001
From: Will Dietz <w@wdtz.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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/time/strptime.c b/src/time/strptime.c
index c54a0d8c..87fa77a8 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,18 @@ 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;
+			unsigned long long secs = 0, new_secs;
+			const unsigned long long max_time = (1ULL<<8*sizeof(time_t)-1)-1;
+			do {
+				new_secs = secs*10 + (*s-'0');
+				if (new_secs < secs || new_secs > max_time) return 0;
+				secs = new_secs;
+			} while(isdigit(*++s));
+			time_t t = secs;
+			if (!__localtime_r(&t, tm)) return 0;
+			break;
 		case 'T':
 			s = strptime(s, "%H:%M:%S", tm);
 			if (!s) return 0;
-- 
2.18.0