Timestamp of zero in reflog considered invalid

Timestamp of zero in reflog considered invalid

[Erik Bray]

Hi all,

I found this issue through a test suite for a Python git interface,
which during the tests at some point sets

GIT_COMMITTER_DATE=1970-01-01T00:00:00

To reproduce the issue:

$ git init
$ echo foo > testfile
$ git add testfile
$ git commit -m "test"
$ echo bar >> testfile
$ export GIT_COMMITTER_DATE=1970-01-01T00:00:00
$ git stash save
$ git stash apply
refs/stash@{0} is not a valid reference

At this point one can see:

$ git rev-parse --symbolic --verify 'refs/stash@{0}'
fatal: Log for refs/stash is empty.

Expected:

$ git rev-parse --symbolic --verify 'refs/stash@{0}'
refs/stash@{0}

I tracked the issue to refs/files-backend.c in show_one_reflog_ent :

https://github.com/git/git/blob/11529ecec914d2f0d7575e6d443c2d5a6ff75424/refs/files-backend.c#L2923

in which

!(timestamp = strtoul(email_end + 2, &message, 10)) ||

implies an invalid reflog entry.  Why should 0 be treated as an
invalid timestamp (even if it's unlikely outside of corner cases)?

Thanks,
Erik

Re: Timestamp of zero in reflog considered invalid

[Junio C Hamano]

Erik Bray <erik.m.bray@gmail.com> writes:

> I tracked the issue to refs/files-backend.c in show_one_reflog_ent :
>
> https://github.com/git/git/blob/11529ecec914d2f0d7575e6d443c2d5a6ff75424/refs/files-backend.c#L2923
>
> in which
>
> !(timestamp = strtoul(email_end + 2, &message, 10)) ||
>
> implies an invalid reflog entry.  Why should 0 be treated as an
> invalid timestamp (even if it's unlikely outside of corner cases)?

Thanks for a report.

I think this dates back to 883d60fa (Sanitize for_each_reflog_ent(),
2007-01-08):

commit 883d60fa97c6397450fb129634054e0a6101baac
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date:   Mon Jan 8 01:59:54 2007 +0100

    Sanitize for_each_reflog_ent()
    
    It used to ignore the return value of the helper function; now, it
    expects it to return 0, and stops iteration upon non-zero return
    values; this value is then passed on as the return value of
    for_each_reflog_ent().
    
    Further, it makes no sense to force the parsing upon the helper
    functions; for_each_reflog_ent() now calls the helper function with
    old and new sha1, the email, the timestamp & timezone, and the message.
    
    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    Signed-off-by: Junio C Hamano <junkio@cox.net>

I do not see a corresponding "timestamp must be non-zero" check in
the lines removed by that commit.  I suspect that there was some
confusion as to how strtoul() signals an error condition when the
commit was written, or something.  I do not think existing
implementations of Git supports timestamps before the epoch (the
timestamp on the common object headers are read into unsigned long
variables and the code is not prepared to see anything negative
there), but if anything the code should be detecting errors from
strtoul() properly, i.e. if a timestamp is way long into the future
and does not fit in "unsigned long", we should detect it.

Checking the value against ULONG_MAX and errno==ERANGE would be an
improvement.  It may be debatable if we should silently ignore an
entry with an invalid timestamp, but that is a separate issue.

 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 4e15f60..ff24184 100644
--- a/refs.c
+++ b/refs.c
@@ -3701,7 +3701,8 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
 	    get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||
 	    !(email_end = strchr(sb->buf + 82, '>')) ||
 	    email_end[1] != ' ' ||
-	    !(timestamp = strtoul(email_end + 2, &message, 10)) ||
+	    ((timestamp = strtoul(email_end + 2, &message, 10)) == ULONG_MAX &&
+	     errno == ERANGE) ||
 	    !message || message[0] != ' ' ||
 	    (message[1] != '+' && message[1] != '-') ||
 	    !isdigit(message[2]) || !isdigit(message[3]) ||

Re: Timestamp of zero in reflog considered invalid

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

> Checking the value against ULONG_MAX and errno==ERANGE would be an
> improvement.  It may be debatable if we should silently ignore an
> entry with an invalid timestamp, but that is a separate issue.
>
>  refs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 4e15f60..ff24184 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3701,7 +3701,8 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
>  	    get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||
>  	    !(email_end = strchr(sb->buf + 82, '>')) ||
>  	    email_end[1] != ' ' ||
> -	    !(timestamp = strtoul(email_end + 2, &message, 10)) ||
> +	    ((timestamp = strtoul(email_end + 2, &message, 10)) == ULONG_MAX &&
> +	     errno == ERANGE) ||

You need to set errno = 0 before calling strtoul, to distinguish the
valid return of ULONG_MAX (which would keep errno intact) and a real
overflow.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Timestamp of zero in reflog considered invalid

[Johannes Schindelin]

Hi Junio,

On Tue, 5 Apr 2016, Junio C Hamano wrote:

> Erik Bray <erik.m.bray@gmail.com> writes:
> 
> > I tracked the issue to refs/files-backend.c in show_one_reflog_ent :
> >
> > https://github.com/git/git/blob/11529ecec914d2f0d7575e6d443c2d5a6ff75424/refs/files-backend.c#L2923
> >
> > in which
> >
> > !(timestamp = strtoul(email_end + 2, &message, 10)) ||
> >
> > implies an invalid reflog entry.  Why should 0 be treated as an
> > invalid timestamp (even if it's unlikely outside of corner cases)?
> 
> Thanks for a report.
> 
> I think this dates back to 883d60fa (Sanitize for_each_reflog_ent(),
> 2007-01-08):

Wow, what a blast from the past.

Unsurprisingly, I do not recall *any* detail about this, but this sounds
like it is the most probable explanation:

> I suspect that there was some confusion as to how strtoul() signals an
> error condition when the commit was written, or something.

FWIW: ACK on the patch:

> diff --git a/refs.c b/refs.c
> index 4e15f60..ff24184 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3701,7 +3701,8 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
>  	    get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||
>  	    !(email_end = strchr(sb->buf + 82, '>')) ||
>  	    email_end[1] != ' ' ||
> -	    !(timestamp = strtoul(email_end + 2, &message, 10)) ||
> +	    ((timestamp = strtoul(email_end + 2, &message, 10)) == ULONG_MAX &&
> +	     errno == ERANGE) ||
>  	    !message || message[0] != ' ' ||
>  	    (message[1] != '+' && message[1] != '-') ||
>  	    !isdigit(message[2]) || !isdigit(message[3]) ||

Thanks,
Dscho

Re: Timestamp of zero in reflog considered invalid

[Erik Bray]

On Tue, Apr 5, 2016 at 5:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks for a report.

Thanks for looking into it!

> I think this dates back to 883d60fa (Sanitize for_each_reflog_ent(),
> 2007-01-08):
>
> commit 883d60fa97c6397450fb129634054e0a6101baac
> Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Date:   Mon Jan 8 01:59:54 2007 +0100

Ah yes, I tracked it to this too--forgot to mention that, sorry.

> I suspect that there was some
> confusion as to how strtoul() signals an error condition when the
> commit was written, or something.

Yep--the next line *did* check for (!message) which would indicate
that strtoul failed, but checking for errno==ERANGE sounds good.  +1
on the patch modulo setting errno = 0 first.

Thanks,
Erik