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

[Rich Felker <dalias@libc.org>]

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.

> +				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.

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...

Rich