[Toybox] [PATCH] Fix several bugs in date.
enh
enh at google.com
Tue Aug 11 12:10:31 PDT 2015
On Mon, Aug 10, 2015 at 11:58 PM, Rob Landley <rob at landley.net> wrote:
> On 08/10/2015 10:39 PM, enh wrote:
>> Fix several bugs in date.
>
> Sigh. I honestly did test it, although clearly not thoroughly enough.
> (That's how I set my clock to the year 2157, which networkmangler did
> _not_ like...)
yeah, that's the advantage of having real unit tests. -d lets us test
the parsing reasonably well though (even if toybox's -d is
incompatible with coreutils').
> Ow. I see I checked in debug code. (I put my debug code flush with the
> left edge so it's easy to spot and yank again.) Sorry about that.
>
>> We were exiting before actually setting the date, chkmktime was always
>> failing, and we would segfault in glibc's strftime for non-Unix dates.
>> Plus one of the tests was mangled.
>
> Regression test suite. This is another one where I need to run it in a
> VM with root access...
the date tests run fine without. (well, in the version below where
they don't assume the system timezone is UTC.) the trade-off, of
course, is that they don't actually test setting the date. but they
still catch all the errors we've had so far...
>> Also fix existing tests and add some more, and stop using the overly
>> vague "bad" in errors.
>
> I intentionally use "bad" because I'm trying to restrict the error
> message vocabulary so non-native english speakers have a fighting chance
> to follow along without a dictionary. See "Error messages and
> internationalization" in http://landley.net/toybox/design.html
>
> It's likely localized man pages are available, or posix standards, or a
> translation of toybox's help -ah output web page. But annotating a bunch
> of strings with _("thingy") in each command is something I'd prefer to
> avoid because it's intrusive and waaaay too easy to introduce printf
> bugs. (And half the command names are english words anyway so a certain
> amount of english is probably unavoidable, but I would like to minimize
> the requirement for the 2/3 of the planet it's inconvenient for.)
the problem with this is that the resulting error messages suck
because they're so vague, and you can't even tell what the problem was
by looking at the source (because it's one error for every possible
point of failure). i agree that "invalid" isn't great, but i couldn't
come up with a short way to say what it actually means (other than
"non-normal", which is accurate but not helpful even for 99.9% of
native English speakers).
(your latest patch actually makes things worse because now you're not
even showing the string you failed to parse! fixed in the patch
below.)
> I had two messages this time because failure to handle input and failure
> to handle output are two distinct error conditions. (Failure to parse
> date is because it's a bad date. Failure to _set_ the date is usually a
> permissions issue, you'll note the "cannot set date" and "bad format"
> messages are perror_exit() and the libc message probably _is_
> internationalized.)
>
> Generally when I want to provide more details about failure to parse a
> string I say "bad stringname" and then give the string and the offset in
> it, ala sed's error_exit("bad pattern '%s'@%ld (%c)", errstart,
> line-errstart+1L, *line);
>
> Other times I use "bad $ADJECTIVE date" where you can treat $ADJECTIVE
> as a swear word and still get the general idea. You're distinguishing
> failure to parse ("bad date format", and since format is already a word
> in the help text it's not _entirely_ new) from the date itself being
> invalid ("bad date").
>
> Rather than asking non english speakers to distinguish "invalid" from
> "bad", I'd rather try to print the input and output dates to show why
> they don't match, ala error_msg("%s != %s", unfiltered, filtered);
yeah, that's a good idea. done in the patch below. this also turned up
another couple of bugs (also fixed in the patch below).
> I checked in about 1/3 of your patch (most obvious bugfixes), I need to
> look at the rest in the morning. (I wanted to turn chmktime() into
> xmktime() and stick it in lib/xwrap.c but it didn't error_exit().
> Changing the semantics of that makes life easier there. But not
> something to fiddle with at 2am... :)
what you did actually made date worse (with the exception of removing
the exit; at least what's currently checked in should work if you
supply a Unix time with @). if you didn't have such an urge to win the
obfuscated C contest, toybox would be much better off...
> Thanks,
>
> Rob
the above-promised patch follows...
Fix more date bugs.
Correctly and portably check for non-normal dates, and explicitly show
the "before" and "after" dates (in the format of the user's choosing).
Clear the struct tm in date_main rather than parse_default because on
one path the struct tm is actually initialized. Explicitly clear the
tm_sec field in parse_default because -- experiment shows -- that
should not be preserved. Only do the "what does this 2-digit year
mean?" dance if we actually parsed a 2-digit year. Show the right
string in the error message if strptime fails.
Also add more tests, and use UTC in the tests to avoid flakiness.
diff --git a/tests/date.test b/tests/date.test
index d72e50c..94a4157 100644
--- a/tests/date.test
+++ b/tests/date.test
@@ -4,7 +4,19 @@
#testing "name" "command" "result" "infile" "stdin"
+# Test Unix date parsing.
+testing "date -d @0" "TZ=UTC date -d @0 2>&1" "Thu Jan 1 00:00:00
GMT 1970\n" "" ""
+testing "date -d @0x123" "TZ=UTC date -d @0x123 2>&1" "date: bad date
'@0x123'\n" "" ""
+
+# Test basic date parsing.
+# Note that toybox's -d format is not the same as coreutils'.
+testing "date -d 06021234" "TZ=UTC date -d 06021234 2>&1" "Sun Jun 2
12:34:00 UTC 1900\n" "" ""
+testing "date -d 060212341982" "TZ=UTC date -d 060212341982 2>&1"
"Sun Jun 2 12:34:00 UTC 1982\n" "" ""
+testing "date -d 123" "TZ=UTC date -d 123 2>&1" "date: bad date '123'\n" "" ""
+
# Accidentally given a Unix time, we should trivially reject that.
-testing "date Unix time" "date 1438053157 2>&1" "date: bad date
'1438053157'\n" "" ""
+testing "date Unix time missing @" "TZ=UTC date 1438053157 2>&1" \
+ "date: bad date '1438053157'; Tue February 38 05:31:00 UTC 2057 !=
Sun Mar 10 05:31:00 UTC 2058\n" "" ""
# But some invalid dates are more subtle, like Febuary 29th in a non-leap year.
-testing "date Feb 29th" "date 022900001975 2>&1" "date: bad date
+testing "date Feb 29th" "TZ=UTC date 022900001975 2>&1" \
+ "date: bad date '022900001975'; Tue Feb 29 00:00:00 UTC 2075 != Fri
Mar 1 00:00:00 UTC 2075\n" "" ""
diff --git a/toys/posix/date.c b/toys/posix/date.c
index 909ca5a..a42de50 100644
--- a/toys/posix/date.c
+++ b/toys/posix/date.c
@@ -56,18 +56,24 @@ GLOBALS(
)
// mktime(3) normalizes the struct tm fields, but date(1) shouldn't.
-static time_t chkmktime(struct tm *tm)
+static time_t chkmktime(struct tm *tm, const char *str, const char* fmt)
{
- struct tm tm2;
+ struct tm tm0 = *tm;
+ struct tm tm1;
time_t t = mktime(tm);
- int *tt1 = (void *)tm, *tt2=(void *)&tm2, i;
- if (t != -1 && localtime_r(&t, &tm2)) {
- for (i=0; i<6; i++) if (tt1[i] != tt2[i]) break;
- if (i == 5) return t;
- }
+ if (t == -1 || !localtime_r(&t, &tm1) ||
+ tm0.tm_sec != tm1.tm_sec || tm0.tm_min != tm1.tm_min ||
+ tm0.tm_hour != tm1.tm_hour || tm0.tm_mday != tm1.tm_mday ||
+ tm0.tm_mon != tm1.tm_mon) {
+ int len;
- return -1;
+ strftime(toybuf, sizeof(toybuf), fmt, &tm0);
+ len = strlen(toybuf) + 1;
+ strftime(toybuf + len, sizeof(toybuf) - len, fmt, &tm1);
+ error_exit("bad date '%s'; %s != %s", str, toybuf, toybuf + len);
+ }
+ return t;
}
static void utzset(void)
@@ -92,8 +98,6 @@ static int parse_default(char *str, struct tm *tm)
{
int len = 0;
- memset(tm, 0, sizeof(struct tm));
-
// Parse @UNIXTIME[.FRACTION]
if (*str == '@') {
long long ll;
@@ -139,16 +143,18 @@ static int parse_default(char *str, struct tm *tm)
// 2 digit years, next 50 years are "future", last 50 years are "past".
// A "future" date in past is a century ahead.
// A non-future date in the future is a century behind.
- if ((r1 < r2) ? (r1 < year && year < r2) : (year < r1 || year > r2)) {
- if (year < r1) year += 100;
- } else if (year > r1) year -= 100;
+ if (len == 2) {
+ if ((r1 < r2) ? (r1 < year && year < r2) : (year < r1 || year > r2)) {
+ if (year < r1) year += 100;
+ } else if (year > r1) year -= 100;
+ }
tm->tm_year = year + century;
}
if (*str == '.') {
len = 0;
sscanf(str, ".%u%n", &tm->tm_sec, &len);
str += len;
- }
+ } else tm->tm_sec = 0;
return *str;
}
@@ -158,6 +164,8 @@ void date_main(void)
char *setdate = *toys.optargs, *format_string = "%a %b %e %H:%M:%S %Z %Y";
struct tm tm;
+ memset(&tm, 0, sizeof(struct tm));
+
// We can't just pass a timezone to mktime because posix.
if (toys.optflags & FLAG_u) utzset();
@@ -165,8 +173,8 @@ void date_main(void)
if (TT.setfmt) {
char *s = strptime(TT.showdate, TT.setfmt+(*TT.setfmt=='+'), &tm);
- if (!s || *s) goto bad_date;
- } else if (parse_default(TT.showdate, &tm)) goto bad_date;
+ if (!s || *s) goto bad_showdate;
+ } else if (parse_default(TT.showdate, &tm)) goto bad_showdate;
} else {
time_t now;
@@ -191,15 +199,14 @@ void date_main(void)
} else if (setdate) {
struct timeval tv;
- if (parse_default(setdate, &tm)) goto bad_date;
+ if (parse_default(setdate, &tm)) error_exit("bad date '%s'", setdate);
if (toys.optflags & FLAG_u) {
// We can't just pass a timezone to mktime because posix.
utzset();
- tv.tv_sec = chkmktime(&tm);
+ tv.tv_sec = chkmktime(&tm, setdate, format_string);
utzreset();
- } else tv.tv_sec = chkmktime(&tm);
- if (tv.tv_sec == (time_t)-1) goto bad_date;
+ } else tv.tv_sec = chkmktime(&tm, setdate, format_string);
tv.tv_usec = TT.nano/1000;
if (settimeofday(&tv, NULL) < 0) perror_msg("cannot set date");
@@ -212,6 +219,6 @@ void date_main(void)
return;
-bad_date:
- error_exit("bad date '%s'", setdate);
+bad_showdate:
+ error_exit("bad date '%s'", TT.showdate);
}
1439320231.0
More information about the Toybox
mailing list