From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/13050 Path: news.gmane.org!.POSTED!not-for-mail From: Will Dietz Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] strptime: add basic support for '%s' (seconds since epoch) Date: Mon, 16 Jul 2018 14:41:20 -0500 Message-ID: References: <20180716001116.GL1392@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="00000000000095a4a50571230320" X-Trace: blaine.gmane.org 1531769972 14457 195.159.176.226 (16 Jul 2018 19:39:32 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 16 Jul 2018 19:39:32 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-13066-gllmg-musl=m.gmane.org@lists.openwall.com Mon Jul 16 21:39:28 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 1ff9LD-0003fr-C0 for gllmg-musl@m.gmane.org; Mon, 16 Jul 2018 21:39:27 +0200 Original-Received: (qmail 1905 invoked by uid 550); 16 Jul 2018 19:41:34 -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 1885 invoked from network); 16 Jul 2018 19:41:33 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wdtz.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=YKj1w6/960jR31DhwcPMn0QUIehYb1VRMFHEpU0w29s=; b=ZwsI7HkIQEPmNtsIhKieGQS6Fnle6+50p/VdeyVbnHj0/Yq1OnrA3VI/vbApDVvtwY gr7B4+8PxoFBb1pb9ahncvDQJJnp/z4B41T30btXKGmm+3JovWkrwYFGrIUGrGpsle7P o8rbbzVDN1Gy1y5kzDNvOvHpkEWiOnQtqt+bM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=YKj1w6/960jR31DhwcPMn0QUIehYb1VRMFHEpU0w29s=; b=ik6DXZ0VjuVMDl0WrA3LsEJQ9jSCg/crfSjpcrYfWBKp/uqje9LlmnIpE6liPhuj2h ApBYFdzkTflGxlTxjMdNo/u/ZuAD4Qpv0dU3JBnkX0+YeWhZu22UhEq31bYfgWqUQFxr NOv2jXJA/DloEOKiUe+LKdRR8nZVe7lx8wRrVxiwsqU3CSLkiRSY4C3u0DUBsg3s0krq qY1RdNSwsIZDDqxTs55aK2PF5ijYxDzi1/Wpd9jTLjGjx02q3bUiOaSXWXBr198laKs/ OyNvp1LKbV6+4A9SZvPNNQbe8sYtzWYXMymqh0aoLgu3xMa7KBsI+9d0pwemjq4RF7Oc GXIw== X-Gm-Message-State: AOUpUlHYeZu8Ay6lSXPY7Cb9F/i8EWI5JBkObaU96HMCstSFQZxZ62/x 36+gtNX84BEBQkYESIk99zHxvRAiX+k/dvrD7TC/5ngQKA== X-Google-Smtp-Source: AAOMgpemDlqIR33fajwQ7+3DddwdXoosUoNQiVuSon0lUJUJGP4pRsOpAAz6VN8dQUl6NmbbKrTQk6CmiPvh3IrFqck= X-Received: by 2002:aca:30d4:: with SMTP id w203-v6mr712599oiw.8.1531770081440; Mon, 16 Jul 2018 12:41:21 -0700 (PDT) X-Originating-IP: [99.4.166.28] In-Reply-To: <20180716001116.GL1392@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:13050 Archived-At: --00000000000095a4a50571230320 Content-Type: text/plain; charset="UTF-8" On Sun, Jul 15, 2018 at 7:11 PM, Rich Felker 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 >> 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. 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 --00000000000095a4a50571230320 Content-Type: text/x-patch; charset="US-ASCII"; name="strptime-s.patch" Content-Disposition: attachment; filename="strptime-s.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_jjoocfrs0 RnJvbSBjYjFjYmRkYWNhMDY1ZTNkZWJhNTA4YmY4NjcwMDRlMjBjMTBlODI0IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBXaWxsIERpZXR6IDx3QHdkdHoub3JnPgpEYXRlOiBXZWQsIDEx IEp1bCAyMDE4IDEzOjA4OjIyIC0wNTAwClN1YmplY3Q6IFtQQVRDSF0gc3RycHRpbWU6IGFkZCBi YXNpYyBzdXBwb3J0IGZvciAnJXMnIChzZWNvbmRzIHNpbmNlIGVwb2NoKQoKLS0tCiBzcmMvdGlt ZS9zdHJwdGltZS5jIHwgMTUgKysrKysrKysrKysrKysrCiAxIGZpbGUgY2hhbmdlZCwgMTUgaW5z ZXJ0aW9ucygrKQoKZGlmZiAtLWdpdCBhL3NyYy90aW1lL3N0cnB0aW1lLmMgYi9zcmMvdGltZS9z dHJwdGltZS5jCmluZGV4IGM1NGEwZDhjLi44N2ZhNzdhOCAxMDA2NDQKLS0tIGEvc3JjL3RpbWUv c3RycHRpbWUuYworKysgYi9zcmMvdGltZS9zdHJwdGltZS5jCkBAIC01LDYgKzUsOSBAQAogI2lu Y2x1ZGUgPHN0ZGRlZi5oPgogI2luY2x1ZGUgPHN0cmluZy5oPgogI2luY2x1ZGUgPHN0cmluZ3Mu aD4KKyNpbmNsdWRlICJ0aW1lX2ltcGwuaCIKKworc3RydWN0IHRtICpfX2xvY2FsdGltZV9yKGNv bnN0IHRpbWVfdCAqcmVzdHJpY3QsIHN0cnVjdCB0bSAqcmVzdHJpY3QpOwogCiBjaGFyICpzdHJw dGltZShjb25zdCBjaGFyICpyZXN0cmljdCBzLCBjb25zdCBjaGFyICpyZXN0cmljdCBmLCBzdHJ1 Y3QgdG0gKnJlc3RyaWN0IHRtKQogewpAQCAtMTE5LDYgKzEyMiwxOCBAQCBjaGFyICpzdHJwdGlt ZShjb25zdCBjaGFyICpyZXN0cmljdCBzLCBjb25zdCBjaGFyICpyZXN0cmljdCBmLCBzdHJ1Y3Qg dG0gKnJlc3RyaQogCQkJbWluID0gMDsKIAkJCXJhbmdlID0gNjE7CiAJCQlnb3RvIG51bWVyaWNf cmFuZ2U7CisJCWNhc2UgJ3MnOgorCQkJaWYgKCFpc2RpZ2l0KCpzKSkgcmV0dXJuIDA7CisJCQl1 bnNpZ25lZCBsb25nIGxvbmcgc2VjcyA9IDAsIG5ld19zZWNzOworCQkJY29uc3QgdW5zaWduZWQg bG9uZyBsb25nIG1heF90aW1lID0gKDFVTEw8PDgqc2l6ZW9mKHRpbWVfdCktMSktMTsKKwkJCWRv IHsKKwkJCQluZXdfc2VjcyA9IHNlY3MqMTAgKyAoKnMtJzAnKTsKKwkJCQlpZiAobmV3X3NlY3Mg PCBzZWNzIHx8IG5ld19zZWNzID4gbWF4X3RpbWUpIHJldHVybiAwOworCQkJCXNlY3MgPSBuZXdf c2VjczsKKwkJCX0gd2hpbGUoaXNkaWdpdCgqKytzKSk7CisJCQl0aW1lX3QgdCA9IHNlY3M7CisJ CQlpZiAoIV9fbG9jYWx0aW1lX3IoJnQsIHRtKSkgcmV0dXJuIDA7CisJCQlicmVhazsKIAkJY2Fz ZSAnVCc6CiAJCQlzID0gc3RycHRpbWUocywgIiVIOiVNOiVTIiwgdG0pOwogCQkJaWYgKCFzKSBy ZXR1cm4gMDsKLS0gCjIuMTguMAoK --00000000000095a4a50571230320--