{"thread":{"id":"41931","subject":"Timestamp of zero in reflog considered invalid","startedAt":"2016-04-05T11:28:31Z","lastAt":"2016-04-06T13:20:02Z","messageCount":5,"participants":["Erik Bray","Junio C Hamano","Andreas Schwab","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"282698","messageId":"CAOTD34YpTXperaC0=Sy=21Q9_+0eGO1y-bP-WzrkxFvrDykRXg@mail.gmail.com","threadId":"41931","inReplyTo":null,"subject":"Timestamp of zero in reflog considered invalid","fromName":"Erik Bray","fromEmail":"erik.m.bray@gmail.com","sentAt":"2016-04-05T11:28:31Z","receivedAt":"2016-04-05T11:28:31Z","isPatch":false,"sender":{"key":"erik.m.bray@gmail.com","avatar":null},"body":"Hi all,\n\nI found this issue through a test suite for a Python git interface,\nwhich during the tests at some point sets\n\nGIT_COMMITTER_DATE=1970-01-01T00:00:00\n\nTo reproduce the issue:\n\n$ git init\n$ echo foo > testfile\n$ git add testfile\n$ git commit -m \"test\"\n$ echo bar >> testfile\n$ export GIT_COMMITTER_DATE=1970-01-01T00:00:00\n$ git stash save\n$ git stash apply\nrefs/stash@{0} is not a valid reference\n\nAt this point one can see:\n\n$ git rev-parse --symbolic --verify 'refs/stash@{0}'\nfatal: Log for refs/stash is empty.\n\nExpected:\n\n$ git rev-parse --symbolic --verify 'refs/stash@{0}'\nrefs/stash@{0}\n\nI tracked the issue to refs/files-backend.c in show_one_reflog_ent :\n\nhttps://github.com/git/git/blob/11529ecec914d2f0d7575e6d443c2d5a6ff75424/refs/files-backend.c#L2923\n\nin which\n\n!(timestamp = strtoul(email_end + 2, &message, 10)) ||\n\nimplies an invalid reflog entry. Why should 0 be treated as an\ninvalid timestamp (even if it's unlikely outside of corner cases)?\n\nThanks,\nErik\n"},{"id":"282709","messageId":"xmqqbn5o2gv1.fsf@gitster.mtv.corp.google.com","threadId":"41931","inReplyTo":"CAOTD34YpTXperaC0=Sy=21Q9_+0eGO1y-bP-WzrkxFvrDykRXg@mail.gmail.com","subject":"Re: Timestamp of zero in reflog considered invalid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-05T15:52:02Z","receivedAt":"2016-04-05T15:52:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Bray writes:\n\n> I tracked the issue to refs/files-backend.c in show_one_reflog_ent :\n>\n> https://github.com/git/git/blob/11529ecec914d2f0d7575e6d443c2d5a6ff75424/refs/files-backend.c#L2923\n>\n> in which\n>\n> !(timestamp = strtoul(email_end + 2, &message, 10)) ||\n>\n> implies an invalid reflog entry. Why should 0 be treated as an\n> invalid timestamp (even if it's unlikely outside of corner cases)?\n\nThanks for a report.\n\nI think this dates back to 883d60fa (Sanitize for_each_reflog_ent(),\n2007-01-08):\n\ncommit 883d60fa97c6397450fb129634054e0a6101baac\nAuthor: Johannes Schindelin \nDate: Mon Jan 8 01:59:54 2007 +0100\n\n Sanitize for_each_reflog_ent()\n \n It used to ignore the return value of the helper function; now, it\n expects it to return 0, and stops iteration upon non-zero return\n values; this value is then passed on as the return value of\n for_each_reflog_ent().\n \n Further, it makes no sense to force the parsing upon the helper\n functions; for_each_reflog_ent() now calls the helper function with\n old and new sha1, the email, the timestamp & timezone, and the message.\n \n Signed-off-by: Johannes Schindelin \n Signed-off-by: Junio C Hamano \n\nI do not see a corresponding \"timestamp must be non-zero\" check in\nthe lines removed by that commit. I suspect that there was some\nconfusion as to how strtoul() signals an error condition when the\ncommit was written, or something. I do not think existing\nimplementations of Git supports timestamps before the epoch (the\ntimestamp on the common object headers are read into unsigned long\nvariables and the code is not prepared to see anything negative\nthere), but if anything the code should be detecting errors from\nstrtoul() properly, i.e. if a timestamp is way long into the future\nand does not fit in \"unsigned long\", we should detect it.\n\nChecking the value against ULONG_MAX and errno==ERANGE would be an\nimprovement. It may be debatable if we should silently ignore an\nentry with an invalid timestamp, but that is a separate issue.\n\n refs.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4e15f60..ff24184 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3701,7 +3701,8 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c\n \t get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||\n \t !(email_end = strchr(sb->buf + 82, '>')) ||\n \t email_end[1] != ' ' ||\n-\t !(timestamp = strtoul(email_end + 2, &message, 10)) ||\n+\t ((timestamp = strtoul(email_end + 2, &message, 10)) == ULONG_MAX &&\n+\t errno == ERANGE) ||\n \t !message || message[0] != ' ' ||\n \t (message[1] != '+' && message[1] != '-') ||\n \t !isdigit(message[2]) || !isdigit(message[3]) ||\n"},{"id":"282724","messageId":"87r3ek3qc9.fsf@linux-m68k.org","threadId":"41931","inReplyTo":"xmqqbn5o2gv1.fsf@gitster.mtv.corp.google.com","subject":"Re: Timestamp of zero in reflog considered invalid","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2016-04-05T17:41:58Z","receivedAt":"2016-04-05T17:41:58Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Junio C Hamano writes:\n\n> Checking the value against ULONG_MAX and errno==ERANGE would be an\n> improvement. It may be debatable if we should silently ignore an\n> entry with an invalid timestamp, but that is a separate issue.\n>\n> refs.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 4e15f60..ff24184 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3701,7 +3701,8 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c\n> \t get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||\n> \t !(email_end = strchr(sb->buf + 82, '>')) ||\n> \t email_end[1] != ' ' ||\n> -\t !(timestamp = strtoul(email_end + 2, &message, 10)) ||\n> +\t ((timestamp = strtoul(email_end + 2, &message, 10)) == ULONG_MAX &&\n> +\t errno == ERANGE) ||\n\nYou need to set errno = 0 before calling strtoul, to distinguish the\nvalid return of ULONG_MAX (which would keep errno intact) and a real\noverflow.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"282761","messageId":"alpine.DEB.2.20.1604061416350.3371@virtualbox","threadId":"41931","inReplyTo":"xmqqbn5o2gv1.fsf@gitster.mtv.corp.google.com","subject":"Re: Timestamp of zero in reflog considered invalid","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-06T12:18:27Z","receivedAt":"2016-04-06T12:18:27Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 5 Apr 2016, Junio C Hamano wrote:\n\n> Erik Bray writes:\n> \n> > I tracked the issue to refs/files-backend.c in show_one_reflog_ent :\n> >\n> > https://github.com/git/git/blob/11529ecec914d2f0d7575e6d443c2d5a6ff75424/refs/files-backend.c#L2923\n> >\n> > in which\n> >\n> > !(timestamp = strtoul(email_end + 2, &message, 10)) ||\n> >\n> > implies an invalid reflog entry. Why should 0 be treated as an\n> > invalid timestamp (even if it's unlikely outside of corner cases)?\n> \n> Thanks for a report.\n> \n> I think this dates back to 883d60fa (Sanitize for_each_reflog_ent(),\n> 2007-01-08):\n\nWow, what a blast from the past.\n\nUnsurprisingly, I do not recall *any* detail about this, but this sounds\nlike it is the most probable explanation:\n\n> I suspect that there was some confusion as to how strtoul() signals an\n> error condition when the commit was written, or something.\n\nFWIW: ACK on the patch:\n\n> diff --git a/refs.c b/refs.c\n> index 4e15f60..ff24184 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3701,7 +3701,8 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c\n> \t get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||\n> \t !(email_end = strchr(sb->buf + 82, '>')) ||\n> \t email_end[1] != ' ' ||\n> -\t !(timestamp = strtoul(email_end + 2, &message, 10)) ||\n> +\t ((timestamp = strtoul(email_end + 2, &message, 10)) == ULONG_MAX &&\n> +\t errno == ERANGE) ||\n> \t !message || message[0] != ' ' ||\n> \t (message[1] != '+' && message[1] != '-') ||\n> \t !isdigit(message[2]) || !isdigit(message[3]) ||\n\nThanks,\nDscho\n"},{"id":"282763","messageId":"CAOTD34Z-tJ4-aB7uMy5Hw9D3_N+EOwHsqJ6F+cJ5yHZZzowjug@mail.gmail.com","threadId":"41931","inReplyTo":"xmqqbn5o2gv1.fsf@gitster.mtv.corp.google.com","subject":"Re: Timestamp of zero in reflog considered invalid","fromName":"Erik Bray","fromEmail":"erik.m.bray@gmail.com","sentAt":"2016-04-06T13:20:02Z","receivedAt":"2016-04-06T13:20:02Z","isPatch":false,"sender":{"key":"erik.m.bray@gmail.com","avatar":null},"body":"On Tue, Apr 5, 2016 at 5:52 PM, Junio C Hamano wrote:\n> Thanks for a report.\n\nThanks for looking into it!\n\n> I think this dates back to 883d60fa (Sanitize for_each_reflog_ent(),\n> 2007-01-08):\n>\n> commit 883d60fa97c6397450fb129634054e0a6101baac\n> Author: Johannes Schindelin \n> Date: Mon Jan 8 01:59:54 2007 +0100\n\nAh yes, I tracked it to this too--forgot to mention that, sorry.\n\n> I suspect that there was some\n> confusion as to how strtoul() signals an error condition when the\n> commit was written, or something.\n\nYep--the next line *did* check for (!message) which would indicate\nthat strtoul failed, but checking for errno==ERANGE sounds good. +1\non the patch modulo setting errno = 0 first.\n\nThanks,\nErik\n"}]}