[2/3] approxidate: handle pending number for "specials"
diff mbox series

Message ID 20181102052309.GB19234@sigill.intra.peff.net
State New
Headers show
Series
  • mixed bag of -Wunused-parameter bugfixes
Related show

Commit Message

Jeff King Nov. 2, 2018, 5:23 a.m. UTC
The approxidate parser has a table of special keywords like
"yesterday", "noon", "pm", etc. Some of these, like "pm", do
the right thing if we've recently seen a number: "3pm" is
what you'd think.

However, most of them do not look at or modify the
pending-number flag at all, which means a number may "jump"
across a significant keyword and be used unexpectedly. For
example, when parsing:

  January 5th noon pm

we'd connect the "5" to "pm", and ignore it as a
day-of-month. This is obviously a bit silly, as "noon"
already implies "pm". And other mis-parsed things are
generally as silly ("January 5th noon, years ago" would
connect the 5 to "years", but probably nobody would type
that).

However, the fix is simple: when we see a keyword like
"noon", we should flush the pending number (as we would if
we hit another number, or the end of the string). In a few
of the specials that actually modify the day, we can simply
throw away the number (saying "Jan 5 yesterday" should not
respect the number at all).

Note that we have to either move or forward-declare the
static pending_number() to make it accessible to these
functions; this patch moves it.

Signed-off-by: Jeff King <peff@peff.net>
---
 date.c          | 60 +++++++++++++++++++++++++++----------------------
 t/t0006-date.sh |  1 +
 2 files changed, 34 insertions(+), 27 deletions(-)

Comments

Carlo Marcelo Arenas Belón Nov. 7, 2018, 12:48 a.m. UTC | #1
On Thu, Nov 1, 2018 at 10:24 PM Jeff King <peff@peff.net> wrote:
>
>  static void date_yesterday(struct tm *tm, struct tm *now, int *num)
>  {
> +       *num = 0;

the only caller (date_time) for this sends num = NULL, so this
triggers a segfault.

the only reference I could find to that apparently unused parameter comes from:
93cfa7c7a8 ("approxidate_careful() reports errorneous date string", 2010-01-26)
and seems to indicate it is optional and therefore a check for NULL
might make sense for all other cases

Carlo
Jeff King Nov. 7, 2018, 1:12 a.m. UTC | #2
On Tue, Nov 06, 2018 at 04:48:28PM -0800, Carlo Arenas wrote:

> On Thu, Nov 1, 2018 at 10:24 PM Jeff King <peff@peff.net> wrote:
> >
> >  static void date_yesterday(struct tm *tm, struct tm *now, int *num)
> >  {
> > +       *num = 0;
> 
> the only caller (date_time) for this sends num = NULL, so this
> triggers a segfault.

Oof. Thanks for catching.

That's not the only caller. Usually this is called via a function
pointer from the "struct special" array. date_time() is just reusing the
other helper as a quick way to do a one-day subtraction.

> the only reference I could find to that apparently unused parameter comes from:
> 93cfa7c7a8 ("approxidate_careful() reports errorneous date string", 2010-01-26)
> and seems to indicate it is optional and therefore a check for NULL
> might make sense for all other cases

I think date_yesterday() is the only one of those special functions that
gets called like this. Here's what I think we should do to fix it (this
can go right on top of jk/misc-unused-fixes, which is already in next).

-- >8 --
Subject: [PATCH] approxidate: fix NULL dereference in date_time()

When we see a time like "noon", we pass "12" to our date_time() helper,
which sets the hour to 12pm. If the current time is before noon, then we
wrap around to yesterday using date_yesterday(). But unlike the normal
calls to date_yesterday() from approxidate_alpha(), we pass a NULL "num"
parameter. Since c27cc94fad (approxidate: handle pending number for
"specials", 2018-11-02), that causes a segfault.

One way to fix this is by checking for NULL. But arguably date_time() is
abusing our helper by passing NULL in the first place (and this is the
only case where one of these "special" parsers is used this way). So
instead, let's have it just do the 1-day subtraction itself. It's still
just a one-liner due to our update_tm() helper.

Note that the test added here is a little funny, as we say "10am noon",
which makes the "10am" seem pointless.  But this bug can only be
triggered when it the currently-parsed hour is before the special time.
The latest special time is "tea" at 1700, but t0006 uses a hard-coded
TEST_DATE_NOW of 1900. We could reset TEST_DATE_NOW, but that may lead
to confusion in other tests. Just saying "10am noon" makes this test
self-contained.

Reported-by: Carlo Arenas <carenas@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 date.c          | 2 +-
 t/t0006-date.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/date.c b/date.c
index 7adce327a3..9bc15df6f9 100644
--- a/date.c
+++ b/date.c
@@ -929,7 +929,7 @@ static void date_yesterday(struct tm *tm, struct tm *now, int *num)
 static void date_time(struct tm *tm, struct tm *now, int hour)
 {
 	if (tm->tm_hour < hour)
-		date_yesterday(tm, now, NULL);
+		update_tm(tm, now, 24*60*60);
 	tm->tm_hour = hour;
 	tm->tm_min = 0;
 	tm->tm_sec = 0;
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index b7ea5fbc36..ffb2975e48 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -114,6 +114,7 @@ check_approxidate '15:00' '2009-08-30 15:00:00'
 check_approxidate 'noon today' '2009-08-30 12:00:00'
 check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
 check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
+check_approxidate '10am noon' '2009-08-29 12:00:00'
 
 check_approxidate 'last tuesday' '2009-08-25 19:20:00'
 check_approxidate 'July 5th' '2009-07-05 19:20:00'
Junio C Hamano Nov. 7, 2018, 2:08 a.m. UTC | #3
Jeff King <peff@peff.net> writes:

> On Tue, Nov 06, 2018 at 04:48:28PM -0800, Carlo Arenas wrote:
>
> I think date_yesterday() is the only one of those special functions that
> gets called like this. Here's what I think we should do to fix it (this
> can go right on top of jk/misc-unused-fixes, which is already in next).

Thanks, both.  I think the patch makes sense.

> -- >8 --
> Subject: [PATCH] approxidate: fix NULL dereference in date_time()
>
> When we see a time like "noon", we pass "12" to our date_time() helper,
> which sets the hour to 12pm. If the current time is before noon, then we
> wrap around to yesterday using date_yesterday(). But unlike the normal
> calls to date_yesterday() from approxidate_alpha(), we pass a NULL "num"
> parameter. Since c27cc94fad (approxidate: handle pending number for
> "specials", 2018-11-02), that causes a segfault.
>
> One way to fix this is by checking for NULL. But arguably date_time() is
> abusing our helper by passing NULL in the first place (and this is the
> only case where one of these "special" parsers is used this way). So
> instead, let's have it just do the 1-day subtraction itself. It's still
> just a one-liner due to our update_tm() helper.
>
> Note that the test added here is a little funny, as we say "10am noon",
> which makes the "10am" seem pointless.  But this bug can only be
> triggered when it the currently-parsed hour is before the special time.
> The latest special time is "tea" at 1700, but t0006 uses a hard-coded
> TEST_DATE_NOW of 1900. We could reset TEST_DATE_NOW, but that may lead
> to confusion in other tests. Just saying "10am noon" makes this test
> self-contained.
>
> Reported-by: Carlo Arenas <carenas@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  date.c          | 2 +-
>  t/t0006-date.sh | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/date.c b/date.c
> index 7adce327a3..9bc15df6f9 100644
> --- a/date.c
> +++ b/date.c
> @@ -929,7 +929,7 @@ static void date_yesterday(struct tm *tm, struct tm *now, int *num)
>  static void date_time(struct tm *tm, struct tm *now, int hour)
>  {
>  	if (tm->tm_hour < hour)
> -		date_yesterday(tm, now, NULL);
> +		update_tm(tm, now, 24*60*60);
>  	tm->tm_hour = hour;
>  	tm->tm_min = 0;
>  	tm->tm_sec = 0;
> diff --git a/t/t0006-date.sh b/t/t0006-date.sh
> index b7ea5fbc36..ffb2975e48 100755
> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -114,6 +114,7 @@ check_approxidate '15:00' '2009-08-30 15:00:00'
>  check_approxidate 'noon today' '2009-08-30 12:00:00'
>  check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
>  check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
> +check_approxidate '10am noon' '2009-08-29 12:00:00'
>  
>  check_approxidate 'last tuesday' '2009-08-25 19:20:00'
>  check_approxidate 'July 5th' '2009-07-05 19:20:00'

Patch
diff mbox series

diff --git a/date.c b/date.c
index 49f943e25b..7adce327a3 100644
--- a/date.c
+++ b/date.c
@@ -887,13 +887,42 @@  static time_t update_tm(struct tm *tm, struct tm *now, time_t sec)
 	return n;
 }
 
+/*
+ * Do we have a pending number at the end, or when
+ * we see a new one? Let's assume it's a month day,
+ * as in "Dec 6, 1992"
+ */
+static void pending_number(struct tm *tm, int *num)
+{
+	int number = *num;
+
+	if (number) {
+		*num = 0;
+		if (tm->tm_mday < 0 && number < 32)
+			tm->tm_mday = number;
+		else if (tm->tm_mon < 0 && number < 13)
+			tm->tm_mon = number-1;
+		else if (tm->tm_year < 0) {
+			if (number > 1969 && number < 2100)
+				tm->tm_year = number - 1900;
+			else if (number > 69 && number < 100)
+				tm->tm_year = number;
+			else if (number < 38)
+				tm->tm_year = 100 + number;
+			/* We screw up for number = 00 ? */
+		}
+	}
+}
+
 static void date_now(struct tm *tm, struct tm *now, int *num)
 {
+	*num = 0;
 	update_tm(tm, now, 0);
 }
 
 static void date_yesterday(struct tm *tm, struct tm *now, int *num)
 {
+	*num = 0;
 	update_tm(tm, now, 24*60*60);
 }
 
@@ -908,16 +937,19 @@  static void date_time(struct tm *tm, struct tm *now, int hour)
 
 static void date_midnight(struct tm *tm, struct tm *now, int *num)
 {
+	pending_number(tm, num);
 	date_time(tm, now, 0);
 }
 
 static void date_noon(struct tm *tm, struct tm *now, int *num)
 {
+	pending_number(tm, num);
 	date_time(tm, now, 12);
 }
 
 static void date_tea(struct tm *tm, struct tm *now, int *num)
 {
+	pending_number(tm, num);
 	date_time(tm, now, 17);
 }
 
@@ -953,6 +985,7 @@  static void date_never(struct tm *tm, struct tm *now, int *num)
 {
 	time_t n = 0;
 	localtime_r(&n, tm);
+	*num = 0;
 }
 
 static const struct special {
@@ -1110,33 +1143,6 @@  static const char *approxidate_digit(const char *date, struct tm *tm, int *num,
 	return end;
 }
 
-/*
- * Do we have a pending number at the end, or when
- * we see a new one? Let's assume it's a month day,
- * as in "Dec 6, 1992"
- */
-static void pending_number(struct tm *tm, int *num)
-{
-	int number = *num;
-
-	if (number) {
-		*num = 0;
-		if (tm->tm_mday < 0 && number < 32)
-			tm->tm_mday = number;
-		else if (tm->tm_mon < 0 && number < 13)
-			tm->tm_mon = number-1;
-		else if (tm->tm_year < 0) {
-			if (number > 1969 && number < 2100)
-				tm->tm_year = number - 1900;
-			else if (number > 69 && number < 100)
-				tm->tm_year = number;
-			else if (number < 38)
-				tm->tm_year = 100 + number;
-			/* We screw up for number = 00 ? */
-		}
-	}
-}
-
 static timestamp_t approxidate_str(const char *date,
 				   const struct timeval *tv,
 				   int *error_ret)
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 64ff86df8e..b7ea5fbc36 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -113,6 +113,7 @@  check_approxidate '3:00' '2009-08-30 03:00:00'
 check_approxidate '15:00' '2009-08-30 15:00:00'
 check_approxidate 'noon today' '2009-08-30 12:00:00'
 check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
+check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
 
 check_approxidate 'last tuesday' '2009-08-25 19:20:00'
 check_approxidate 'July 5th' '2009-07-05 19:20:00'