From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from server28.superhosting.bg (server28.superhosting.bg [217.174.156.11]) by sourceware.org (Postfix) with ESMTPS id 00E72385043C for ; Wed, 18 May 2022 18:48:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 00E72385043C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=dinux.eu Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dinux.eu DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=dinux.eu; s=default; h=In-Reply-To:Content-Transfer-Encoding:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=N59T1VFBh9Noxy9JEOE5qR5az9KY2Lu+t+yWkSnn94s=; b=bt1vsvT00yumIWIkRiUW+kKLkr DeOAiyij7YJTPKRhOdUhq7c57Ut4TMmOtCZZ/PHzxoczhi785BrB92xBBZ1EU59xODVaqModMdhSt JYPCWrbQNrCGYW0Qpp6Ezh5DCvEx0PiBVD3LK0kaJ+lVDfhC5X09WJxqxPMzagUr5u17I6vCX82Z9 VxeTlFpsPa70c2vhgLHkUmrjFIpUA0lh4XruGezFPpLRvzzmRf71NBd7DV+nM54cju7GJW+a3XUjF rjWvABi6rbv2cIyHNEnyIu0CHS06pzKC670Bptpv6U1EFMaCaqU9R4nD1lrVrJ3ag3KWOTpNNVHzv ja6qSnDg==; Received: from [95.87.234.74] (port=36520 helo=localhost) by server28.superhosting.bg with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1nrOit-0009OU-NW; Wed, 18 May 2022 21:48:42 +0300 Date: Wed, 18 May 2022 21:48:38 +0300 From: Dimitar Dimitrov To: jdoubleu Cc: newlib@sourceware.org, Jeff Johnston Subject: Re: [PATCH] update tzset tests Message-ID: References: <822e81a0-ed9f-200e-3318-0495456ad67e@SystematicSw.ab.ca> <20220407233425.2012-1-Brian.Inglis@SystematicSW.ab.ca> <9603e3aa-bd7d-6740-c710-27ace1d80397@SystematicSw.ab.ca> <25cfc7a2-2c66-f9fe-581b-8d3cec5d3bd9@jdoubleu.de> <426d1e93-6c4f-4ccc-9522-c187e10716f0@jdoubleu.de> <985b1a40-9c3a-9f7a-273d-86b59bc90265@jdoubleu.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <985b1a40-9c3a-9f7a-273d-86b59bc90265@jdoubleu.de> X-OutGoing-Spam-Status: No, score=-2.9 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - server28.superhosting.bg X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - dinux.eu X-Get-Message-Sender-Via: server28.superhosting.bg: authenticated_id: dimitar@dinux.eu X-Authenticated-Sender: server28.superhosting.bg: dimitar@dinux.eu X-Source: X-Source-Args: X-Source-Dir: X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: newlib@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Newlib mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 May 2022 18:48:48 -0000 On Tue, May 17, 2022 at 10:45:11AM +0200, jdoubleu wrote: > Sorry, here's the patch. Hi jdoubleu, I managed to test your change with https://sourceware.org/pipermail/newlib/2022/019710.html Only the following test case fails in tzset.c: {"<+0123456789ABCDEF>3:33:33", IN_SECONDS(3, 33, 33), NO_TIME}, // truncates the name (17 + 1) Failure message is: Assertion failed! Expected 1647906533 to equal 1647893720. winter time, timezone = "<+0123456789ABCDEF>3:33:33" The rest of the cases pass on pru-unknown-elf target. BTW, it took me a while to realize that your patch and the source code in newlib's GIT have different line endings :) Regards, Dimitar > > > Cheers > --- > πŸ™ŽπŸ»β€β™‚οΈ jdoubleu > On 5/14/2022 4:39 PM, jdoubleu wrote: > > > To summarize, the following cases are errors: > > > 1. name is too short (less than 3 chars) > > > 2. name is too long (more than TZNAME_MAX) > > > 3. name includes arbitrary chars (not <>+-ALPHANUM) > > > In all of these error cases, the time should be set back to UTC, right? > > > > Here's my patch which adds test cases for the above error cases. > > > > It's based on the latest (pending) changes of tzset[1]. Please apply > > them first. > > > > I wasn't able to run the tests, yet. With glibc they're obviously failing. > > > > Could you please test them? > > > > > > > > [1]: https://sourceware.org/pipermail/newlib/2022/019581.html > > > > > > Cheers > > --- > > πŸ™ŽπŸ»β€β™‚οΈ jdoubleu > > On 4/14/2022 10:59 AM, jdoubleu wrote: > > > On 2022-04-13 14:33, Jeff Johnston wrote: > > > > Looking at the glibc tzset code I have locally (not latest/greatest, but > > > > does support angle brackets): > > > > > > I can confirm the behavior with glibc[1]. As it turns out, glibc > > > does not directly impose a character limit on the timezone name, but > > > requires at least 3 characters. From the man page[2]: > > > > > > > The std string specifies an abbreviation for the timezone and must be > > > > three or more alphabetic characters. > > > > > > To my misunderstanding, they don't even ignore remaining characters, > > > but keep all of them, as you can see in the output[1] and Jeff > > > Johnston explained. > > > > > > > but you imply that glibc in fact uses the equivalent of the > > > > scanf "%m[...]" (malloc) > modifier, and I think using that > > > > would be against the newlib > > > philosophy to keep things > > > > limited and under control to support small targets. > > > > > > I agree, newlib SHOULD impose a limit. Especially, since the POSIX > > > standard[3] already introduces an upper limit, though unspecified. > > > > > > The current limit is 11 characters, if I'm not mistaken. The longest > > > name from the tzdb[4] is "<+1030>" i.e. 5 chars (see all extracted > > > names[5]). All others usually are 3 or 4 chars long. > > > > > > That said, I think 11 is reasonably large enough. > > > > > > However, it could be helpful to get the limit from user-code, > > > because there is no error reporting mechanism used. Right now, the > > > limit is only defined in tzset_r.c[6]. So maybe move it to limits.h? > > > One thing to not forget here is to keep limit in sync with the > > > sscanf format's maximum field width[7]. > > > > > > > > > To summarize, the following cases are errors: > > > 1. name is too short (less than 3 chars) > > > 2. name is too long (more than TZNAME_MAX) > > > 3. name includes arbitrary chars (not <>+-ALPHANUM) > > > In all of these error cases, the time should be set back to UTC, right? > > > > > > > > > I'm going to prepare some test cases for the test suite to check for > > > the errors as well. > > > > > > > > > [1]: https://godbolt.org/z/o93zo3qxv > > > [2]: https://www.man7.org/linux/man-pages/man3/tzset.3.html > > > [3]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03 > > > > > > [4]: https://github.com/eggert/tz > > > [5]: https://raw.githubusercontent.com/nayarsystems/posix_tz_db/master/zones.csv > > > > > > [6]: https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/time/tzset_r.c;h=9cb30b188f989f65ec9eb6417f5d74020f8c72e9;hb=HEAD#l13 > > > > > > [7]: https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/time/tzset_r.c;h=9cb30b188f989f65ec9eb6417f5d74020f8c72e9;hb=HEAD#l57 > > > > > > > > > > > > > > > Cheers > > > --- > > > πŸ™ŽπŸ»β€β™‚οΈ jdoubleu > > > On 4/14/2022 12:19 AM, Brian Inglis wrote: > > > > On 2022-04-13 14:33, Jeff Johnston wrote: > > > > > Looking at the glibc tzset code I have locally (not > > > > > latest/greatest, but > > > > > does support angle brackets): > > > > > > > > > > If there any parse failures, UTC is defaulted. > > > > > > > > We currently leave the time zone info unchanged. > > > > > > > > > Extraneous characters inside brackets or less than 3 characters is a > > > > > parse failure. > > > > βœ” CheckΒ Β Β  βœ” Check > > > > > > > > > Glibc parses the tz name string char by char and allocates space for > > > > > the name strings so there is no max size. > > > > > > > > The suggestion was that glibc ignores the remaining characters, > > > > but you imply that glibc in fact uses the equivalent of the > > > > scanf "%m[...]" (malloc) modifier, and I think using that would > > > > be against the newlib philosophy to keep things limited and > > > > under control to support small targets.Β  Larger targets like > > > > Cygwin (do our own thing including zoneinfo), and perhaps RTEMS, > > > > can supply their own enhancements. > > > > > > > > > the name strings so there is no max size.Β  I am fine if you > > > > > want to mandate a maximum, but if you do, then too many > > > > > chars should be treated as a failure.Β  If you aren't certain > > > > > of the limit, make the limit higher than you expect. > > > > Current limits are 3-10 allowing for e.g. which is > > > > the most ever likely to be used. It might be reasonable to bump > > > > it up to say 15. > > > > > > > > > If people run into max limit with reasonable timezone format > > > > > strings, then > > > > > we can up the limit. > > > > > > > > The conditions are more or less what is implemented, but we > > > > could do with a couple more tweaks to improve things, like check > > > > for more or extraneous chars within the bracket quotes, and that > > > > no characters remain unconsumed at the end of the parse. > From 4423c43be6a730144b776ba4ec4204cf71b52348 Mon Sep 17 00:00:00 2001 > From: jdoubleu > Date: Sat, 14 May 2022 15:41:22 +0200 > Subject: [PATCH] update tzset tests > > Add test cases for parser errors after reworked parsing behavior. > --- > newlib/testsuite/newlib.time/tzset.c | 56 ++++++++++++++++++++++------ > 1 file changed, 44 insertions(+), 12 deletions(-) > > diff --git a/newlib/testsuite/newlib.time/tzset.c b/newlib/testsuite/newlib.time/tzset.c > index 0e5b196c6..8702b58db 100644 > --- a/newlib/testsuite/newlib.time/tzset.c > +++ b/newlib/testsuite/newlib.time/tzset.c > @@ -111,13 +111,43 @@ struct tz_test test_timezones[] = { > { /* Asia/Colombo */ "<+0530>-5:30", -IN_SECONDS(5, 30, 0), NO_TIME}, > { /* Europe/Berlin */ "CET-1CEST,M3.5.0,M10.5.0/3", -IN_SECONDS(1, 0, 0), -IN_SECONDS(2, 0, 0)}, > > - // END of list > - {NULL, NO_TIME, NO_TIME} > + /// test parsing errors > + // 1. names are too long > + {"JUSTEXCEEDI1:11:11", 0, NO_TIME}, > + {"AVERYLONGNAMEWHICHEXCEEDSTZNAMEMAX2:22:22", 0, NO_TIME}, > + {"FIRSTVERYLONGNAME3:33:33SECONDVERYLONGNAME4:44:44", 0, 0}, > + {"5:55:55", 0, NO_TIME}, > + {"3:33:334:44:44", 0, 0}, > + {"<+JUSTEXCEED>5:55:55", 0, NO_TIME}, > + > + // 2. names are too short > + {"JU6:34:47", 0, NO_TIME}, > + {"HE6:34:47LO3:34:47", 0, 0}, > + {"2:34:47", 0, NO_TIME}, > + {"2:34:473:34:47", 0, 0}, > + > + // 3. names contain invalid chars > + {"N?ME2:10:56", 0, NO_TIME}, > + {"N!ME2:10:56", 0, NO_TIME}, > + {"N/ME2:10:56", 0, NO_TIME}, > + {"N$ME2:10:56", 0, NO_TIME}, > + {"NAME?2:10:56", 0, NO_TIME}, > + {"?NAME2:10:56", 0, NO_TIME}, > + {"NAME?UNK4:21:15", 0, NO_TIME}, > + {"NAME!UNK4:22:15NEXT/NAME4:23:15", 0, NO_TIME}, > + > + // 4. bogus strings > + {"NOINFO", 0, NO_TIME}, > + {"HOUR:16:18", 0, NO_TIME}, > + {" + {" + {">WRONG<2:15:00", 0, NO_TIME}, > + {"ST + //{"MANY8:00:00:00", 0, NO_TIME}, > + {"\0", 0, NO_TIME}, > + {"M\0STR7:30:36", 0, NO_TIME} > }; > > -// helper macros > -#define FOR_TIMEZONES(iter_name) for (struct tz_test* iter_name = test_timezones; iter_name->tzstr != NULL; ++iter_name) > - > // END test vectors > > static int failed = 0; > @@ -136,22 +166,24 @@ void test_TimezoneStrings(void) > { > char buffer[128]; > > - FOR_TIMEZONES(ptr) > + for (int i = 0; i < (sizeof(test_timezones) / sizeof(struct tz_test)); ++i) > { > - setenv("TZ", ptr->tzstr, 1); > + struct tz_test ptr = test_timezones[i]; > + > + setenv("TZ", ptr.tzstr, 1); > tzset(); > > - snprintf(buffer, 128, "winter time, timezone = \"%s\"", ptr->tzstr); > + snprintf(buffer, 128, "winter time, timezone = \"%s\"", ptr.tzstr); > > struct tm winter_tm_copy = winter_tm; // copy > - TEST_ASSERT_EQUAL_INT_MESSAGE(winter_time + ptr->offset_seconds, mktime(&winter_tm_copy), buffer); > + TEST_ASSERT_EQUAL_INT_MESSAGE(winter_time + ptr.offset_seconds, mktime(&winter_tm_copy), buffer); > > - if (ptr->dst_offset_seconds != NO_TIME) > + if (ptr.dst_offset_seconds != NO_TIME) > { > - snprintf(buffer, 128, "summer time, timezone = \"%s\"", ptr->tzstr); > + snprintf(buffer, 128, "summer time, timezone = \"%s\"", ptr.tzstr); > > struct tm summer_tm_copy = summer_tm; // copy > - TEST_ASSERT_EQUAL_INT_MESSAGE(summer_time + ptr->dst_offset_seconds, mktime(&summer_tm_copy), buffer); > + TEST_ASSERT_EQUAL_INT_MESSAGE(summer_time + ptr.dst_offset_seconds, mktime(&summer_tm_copy), buffer); > } > } > } > -- > 2.35.1 >