Re: git log - crash and core dump

René Scharfe [off-list ref] writes:

Looks like a sensible change.  split_ident_line() decided that the
given input was mangled and decided there is no valid date (the
input had <> where the timestamp string was required), so the
updated code leaves the time/tz unspecified.

It still is curious how a malformed line was created in the first
place.  I wouldn't worry if a private tool used hash-object to
create such a commit, but if it is something that is commonly used
(e.g. "git commit"), others may suffer from the same and the tool
needs to be tightened a bit.

First, lest I forget again: Thank you, Ivan, for the very useful
bug report!

Am 16.04.2013 21:45, schrieb Junio C Hamano:

René Scharfe [off-list ref] writes:

quoted

Does this patch help?

  pretty.c | 10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/pretty.c b/pretty.c
index d3a82d2..713eefc 100644
--- a/pretty.c
+++ b/pretty.c

@@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,
  	const char *mailbuf, *namebuf;
  	size_t namelen, maillen;
  	int max_length = 78; /* per rfc2822 */
-	unsigned long time;
-	int tz;
+	unsigned long time = 0;
+	int tz = 0;
  
  	if (pp->fmt == CMIT_FMT_ONELINE)
  		return;

@@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,
  	strbuf_add(&name, namebuf, namelen);
  
  	namelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */
-	time = strtoul(ident.date_begin, &date, 10);
-	tz = strtol(date, NULL, 10);
+	if (ident.date_begin) {
+		time = strtoul(ident.date_begin, &date, 10);
+		tz = strtol(date, NULL, 10);
+	}
  
  	if (pp->fmt == CMIT_FMT_EMAIL) {
  		strbuf_addstr(sb, "From: ");

Looks like a sensible change.  split_ident_line() decided that the
given input was mangled and decided there is no valid date (the
input had <> where the timestamp string was required), so the
updated code leaves the time/tz unspecified.

We'd need update pretty.c::format_person_part() and
builtin/blame.c::get_ac_line() as well, though.

How about making split_ident_line() a bit friendlier be letting it
provide the epoch as default time stamp instead of NULL?  We shouldn't
do that if we'd like to be able to tell a missing/broken time stamp
apart from a commit that was actually made back in 1970 (e.g. an
imported one).  Or if we'd like to not show a time stamp in git log
output at all in that case.

-- >8 --
Subject: ident: let split_ident_line() provide a default time stamp

If a commit has a broken time stamp, split_ident_line() sets
date_begin, date_end, tz_begin and tz_end to NULL.  Not all callers
are prepared to handle that case and segfault.

Instead of fixing them and having to be careful while implementing
the next caller, provide a string consisting of the number zero as
default value, representing the UNIX epoch.  That's the value that
git log showed before it was converted to use split_ident_line().

Reported-by: Ivan Lyapunov <redacted>
Signed-off-by: Rene Scharfe <redacted>
---
 ident.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/ident.c b/ident.c
index 1c123e6..ee840f4 100644
--- a/ident.c
+++ b/ident.c

@@ -191,6 +191,8 @@ static void strbuf_addstr_without_crud(struct strbuf *sb, const char *src)
 	sb->buf[sb->len] = '\0';
 }
 
+static const char zero_string[] = "0";
+
 /*
  * Reverse of fmt_ident(); given an ident line, split the fields
  * to allow the caller to parse it.

@@ -254,10 +256,10 @@ int split_ident_line(struct ident_split *split, const char *line, int len)
 	return 0;
 
 person_only:
-	split->date_begin = NULL;
-	split->date_end = NULL;
-	split->tz_begin = NULL;
-	split->tz_end = NULL;
+	split->date_begin = zero_string;
+	split->date_end = zero_string + strlen(zero_string);
+	split->tz_begin = zero_string;
+	split->tz_end = zero_string + strlen(zero_string);
 	return 0;
 }
 

-- 
1.8.2.1

On Tue, Apr 16, 2013 at 9:45 PM, Junio C Hamano [off-list ref] wrote:

It still is curious how a malformed line was created in the first
place.  I wouldn't worry if a private tool used hash-object to
create such a commit, but if it is something that is commonly used
(e.g. "git commit"), others may suffer from the same and the tool
needs to be tightened a bit.

I already happened to see one like that, and it was clearly imported
through remote-hg. I've not been able to reproduce though, and the
parser in git-fast-import seemed already robust enough to me to not
allow this kind of messed-up line. I will see if I can find some time
to reproduce/investigate this deeper.

On Tue, Apr 16, 2013 at 12:45:03PM -0700, Junio C Hamano wrote:

René Scharfe [off-list ref] writes:

quoted

Does this patch help?

 pretty.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/pretty.c b/pretty.c
index d3a82d2..713eefc 100644
--- a/pretty.c
+++ b/pretty.c

@@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,
 	const char *mailbuf, *namebuf;
 	size_t namelen, maillen;
 	int max_length = 78; /* per rfc2822 */
-	unsigned long time;
-	int tz;
+	unsigned long time = 0;
+	int tz = 0;
 
 	if (pp->fmt == CMIT_FMT_ONELINE)
 		return;

@@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,
 	strbuf_add(&name, namebuf, namelen);
 
 	namelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */
-	time = strtoul(ident.date_begin, &date, 10);
-	tz = strtol(date, NULL, 10);
+	if (ident.date_begin) {
+		time = strtoul(ident.date_begin, &date, 10);
+		tz = strtol(date, NULL, 10);
+	}
 
 	if (pp->fmt == CMIT_FMT_EMAIL) {
 		strbuf_addstr(sb, "From: ");

Looks like a sensible change.  split_ident_line() decided that the
given input was mangled and decided there is no valid date (the
input had <> where the timestamp string was required), so the
updated code leaves the time/tz unspecified.

Hmm. This seemed oddly familiar to me, and indeed:

  http://thread.gmane.org/gmane.comp.version-control.git/216870/focus=217077

We need to fix blame, too, and there is a question of how "cat-file -p"
behaves.

-Peff