# [PATCH] parse_commit_buffer: don't parse invalid commits

3 messages from 2008-01-19 to 2008-01-20. Participants: Martin Koegler, Junio C Hamano.
Thread: https://gitlist.dev/t/11678

## Martin Koegler, 2008-01-19 17:35

Subject: [PATCH] parse_commit_buffer: don't parse invalid commits
Message-ID: <1200764123283-git-send-email-mkoegler@auto.tuwien.ac.at>
URL: https://gitlist.dev/e/1200764123283-git-send-email-mkoegler%40auto.tuwien.ac.at

```
* check, that the tree line ends with \n
* prevent parse_commit_date from reading beyond the buffer,
  - if author line does not end with \n
  - if committer line does not contain >
* verify in parse_commit_date, that the commiter line ends with \n

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 commit.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/commit.c b/commit.c
index f074811..8b8fb04 100644
--- a/commit.c
+++ b/commit.c
@@ -48,19 +48,32 @@ struct commit *lookup_commit(const unsigned char *sha1)
 	return check_commit(obj, sha1, 0);
 }
 
-static unsigned long parse_commit_date(const char *buf)
+static unsigned long parse_commit_date(const char *buf, const char *tail)
 {
 	unsigned long date;
+	const char *dateptr;
 
+	if (buf + 6 >= tail)
+		return 0;
 	if (memcmp(buf, "author", 6))
 		return 0;
-	while (*buf++ != '\n')
+	while (buf < tail && *buf++ != '\n')
 		/* nada */;
+	if (buf + 9 >= tail)
+		return 0;
 	if (memcmp(buf, "committer", 9))
 		return 0;
-	while (*buf++ != '>')
+	while (buf < tail && *buf++ != '>')
 		/* nada */;
-	date = strtoul(buf, NULL, 10);
+	if (buf >= tail)
+		return 0;
+	dateptr = buf;
+	while (buf < tail && *buf++ != '\n')
+		/* nada */;
+	if (buf >= tail)
+		return 0;
+	/* dateptr < buf && buf[-1] == '\n', so strtoul will stop at buf-1 */
+	date = strtoul(dateptr, NULL, 10);
 	if (date == ULONG_MAX)
 		date = 0;
 	return date;
@@ -236,9 +249,9 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)
 		return 0;
 	item->object.parsed = 1;
 	tail += size;
-	if (tail <= bufptr + 5 || memcmp(bufptr, "tree ", 5))
+	if (tail <= bufptr + 46 || memcmp(bufptr, "tree ", 5) || bufptr[45] != '\n')
 		return error("bogus commit object %s", sha1_to_hex(item->object.sha1));
-	if (tail <= bufptr + 45 || get_sha1_hex(bufptr + 5, parent) < 0)
+	if (get_sha1_hex(bufptr + 5, parent) < 0)
 		return error("bad tree pointer in commit %s",
 			     sha1_to_hex(item->object.sha1));
 	item->tree = lookup_tree(parent);
@@ -275,7 +288,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)
 			n_refs++;
 		}
 	}
-	item->date = parse_commit_date(bufptr);
+	item->date = parse_commit_date(bufptr, tail);
 
 	if (track_object_refs) {
 		unsigned i = 0;
-- 
gitgui.0.9.1.g39b9

```

## Junio C Hamano, 2008-01-19 19:52

Subject: Re: [PATCH] parse_commit_buffer: don't parse invalid commits
Message-ID: <7vprvx8ueo.fsf@gitster.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vprvx8ueo.fsf%40gitster.siamese.dyndns.org
In-Reply-To: <1200764123283-git-send-email-mkoegler@auto.tuwien.ac.at>

```
Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:

> * check, that the tree line ends with \n

Ok.

> * prevent parse_commit_date from reading beyond the buffer,
>   - if author line does not end with \n
>   - if committer line does not contain >

"even if" would be nicer, I think.

> * verify in parse_commit_date, that the commiter line ends with \n

Perhaps an additional note to leave for people who will later be
tempted to touch this part of the code is needed; otherwise the
time spent on the thread goes wasted.

 * keep the existing behaviour to return a commit object even
   when non-structural fields such as committer and author are
   malformed, so that tools that need to look at commits to
   clean up a history with such broken commits can still get at
   the structural data (i.e. the parents chain and the tree
   object).

No need to resend if you allow me to amend the patch along the
above lines (please just say so), or you could resend with
improvements on your own.

Thanks.

```

## Martin Koegler, 2008-01-20 16:11

Subject: Re: [PATCH] parse_commit_buffer: don't parse invalid commits
Message-ID: <20080120161105.GA10830@auto.tuwien.ac.at>
URL: https://gitlist.dev/e/20080120161105.GA10830%40auto.tuwien.ac.at
In-Reply-To: <7vprvx8ueo.fsf@gitster.siamese.dyndns.org>

```
On Sat, Jan 19, 2008 at 11:52:47AM -0800, Junio C Hamano wrote:
> No need to resend if you allow me to amend the patch along the
> above lines (please just say so), or you could resend with
> improvements on your own.

Feel free to change it.

mfg Martin Kögler

```