Timestamp of zero in reflog considered invalid

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

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

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(-)

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

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.

Hi Junio,

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

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:

FWIW: ACK on the patch:

Thanks, Dscho

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

Thanks for looking into it!

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

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