From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/13048 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] strptime: add basic support for '%s' (seconds since epoch) Date: Sun, 15 Jul 2018 20:11:16 -0400 Message-ID: <20180716001116.GL1392@brightrain.aerifal.cx> References: Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1531699768 19912 195.159.176.226 (16 Jul 2018 00:09:28 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 16 Jul 2018 00:09:28 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-13064-gllmg-musl=m.gmane.org@lists.openwall.com Mon Jul 16 02:09:23 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1fer4s-00055Q-Vf for gllmg-musl@m.gmane.org; Mon, 16 Jul 2018 02:09:23 +0200 Original-Received: (qmail 5536 invoked by uid 550); 16 Jul 2018 00:11:29 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 5486 invoked from network); 16 Jul 2018 00:11:29 -0000 Content-Disposition: inline In-Reply-To: Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:13048 Archived-At: 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 > 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 > #include > #include > +#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