Re: [PATCH] name-rev: avoid cutoff timestamp underflow

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Sun, Sep 22, 2019 at 07:57:36PM +0100, Phillip Wood wrote:
> On 22/09/2019 19:01, SZEDER Gábor wrote:
> >When 'git name-rev' is invoked with commit-ish parameters, it tries to
> >save some work, and doesn't visit commits older than the committer
> >date of the oldest given commit minus a one day worth of slop.  Since
> >our 'timestamp_t' is an unsigned type, this leads to a timestamp
> >underflow when the committer date of the oldest given commit is within
> >a day of the UNIX epoch.  As a result the cutoff timestamp ends up
> >far-far in the future, and 'git name-rev' doesn't visit any commits,
> >and names each given commit as 'undefined'.
> >
> >Check whether substacting the slop from the oldest committer date
> >would lead to an underflow, and use a 0 as cutoff in that case.  This
> >way it will handle commits shortly after the epoch even if we were to
> >switch to a signed 'timestamp_t' (but then we'll have to worry about
> >signed underflow for very old commits).
> >
> >Note that the type of the cutoff timestamp variable used to be signed
> >before 5589e87fd8 (name-rev: change a "long" variable to timestamp_t,
> >2017-05-20).  The behavior was still the same even back then, but the
> >underflow didn't happen when substracting the slop from the oldest
> >committer date, but when comparing the signed cutoff timestamp with
> >unsigned committer dates in name_rev().  IOW, this underflow bug is as
> >old as 'git name-rev' itself.
> >
> >Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> >---
> >
> >This patch adds a test at the end of 't6120-describe.sh', so it will
> >conflict with my non-recursive name-rev patch series, which adds a
> >test there as well, but the conflict should be wasy to resolve.
> >
> >   https://public-inbox.org/git/20190919214712.7348-7-szeder.dev@gmail.com/
> >
> >  builtin/name-rev.c  | 15 ++++++++++++---
> >  t/t6120-describe.sh | 15 +++++++++++++++
> >  2 files changed, 27 insertions(+), 3 deletions(-)
> >
> >diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> >index c785fe16ba..a4d8d312ab 100644
> >--- a/builtin/name-rev.c
> >+++ b/builtin/name-rev.c
> >@@ -9,7 +9,11 @@
> >  #include "sha1-lookup.h"
> >  #include "commit-slab.h"
> >-#define CUTOFF_DATE_SLOP 86400 /* one day */
> >+/*
> >+ * One day.  See the 'name a rev close to epoch' test in t6120 when
> >+ * changing this value
> >+ */
> >+#define CUTOFF_DATE_SLOP 86400
> >  typedef struct rev_name {
> >  	const char *tip_name;
> >@@ -481,8 +485,13 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
> >  		add_object_array(object, *argv, &revs);
> >  	}
> >-	if (cutoff)
> >-		cutoff = cutoff - CUTOFF_DATE_SLOP;
> >+	if (cutoff) {
> >+		/* check for undeflow */
> >+		if (cutoff - CUTOFF_DATE_SLOP < cutoff)
> 
> Nice catch but wouldn't this be clearer as
>   if (cutoff > CUTOFF_DATE_SLOP) ?

It would only be clearer now, with an unsigned 'timestamp_t'.  I
tried to future-proof for a signed 'timestamp_t' and a cutoff date
before the UNIX epoch.

> >+			cutoff = cutoff - CUTOFF_DATE_SLOP;
> >+		else
> >+			cutoff = 0;
> >+	}