git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: git@vger.kernel.org
Cc: gitster@pobox.com, stolee@gmail.com, peff@peff.net
Subject: [PATCH 2/3] commit-graph.c: handle commit parsing errors
Date: Thu, 5 Sep 2019 18:04:55 -0400	[thread overview]
Message-ID: <7fb6853ab31f35c27e5c220f11ea2de6e229a806.1567720960.git.me@ttaylorr.com> (raw)
In-Reply-To: <cover.1567720960.git.me@ttaylorr.com>

To write a commit graph chunk, 'write_graph_chunk_data()' takes a list
of commits to write and parses each one before writing the necessary
data, and continuing on to the next commit in the list.

Since the majority of these commits are not parsed ahead of time (an
exception is made for the *last* commit in the list, which is parsed
early within 'copy_oids_to_commits'), it is possible that calling
'parse_commit_no_graph()' on them may return an error. Failing to catch
these errors before de-referencing later calls can result in a undefined
memory access and a SIGSEGV.

One such example of this is 'get_commit_tree_oid()', which expects a
parsed object as its input (in this case, the commit-graph code passes
'*list'). If '*list' causes a parse error, the subsequent call will
fail.

Prevent such an issue by checking the return value of
'parse_commit_no_graph()' to avoid passing an unparsed object to a
function which expects a parsed object, thus preventing a segfault.

It is worth noting that this fix is really skirting around the issue in
object.c's 'parse_object()', which makes it difficult to tell how
corrupt an object is without digging into it. Presumably one could
change the meaning of 'parse_object' returns, but this would require
adjusting each callsite accordingly. Instead of that, add an additional
check to the object parsed.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c          | 4 +++-
 t/t5318-commit-graph.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index f2888c203b..6aa6998ecd 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -843,7 +843,9 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
 		uint32_t packedDate[2];
 		display_progress(ctx->progress, ++ctx->progress_cnt);
 
-		parse_commit_no_graph(*list);
+		if (parse_commit_no_graph(*list))
+			die(_("unable to parse commit %s"),
+				oid_to_hex(&(*list)->object.oid));
 		hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);
 
 		parent = (*list)->parents;
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index c855f81930..abde8d4e90 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -585,7 +585,7 @@ test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'corrupt commit-graph write (broken parent)' '
+test_expect_success 'corrupt commit-graph write (broken parent)' '
 	rm -rf repo &&
 	git init repo &&
 	(
-- 
2.23.0


  parent reply	other threads:[~2019-09-05 22:04 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-05 22:04 [PATCH 0/3] commit-graph: harden against various corruptions Taylor Blau
2019-09-05 22:04 ` [PATCH 1/3] t/t5318: introduce failing 'git commit-graph write' tests Taylor Blau
2019-09-06 16:48   ` Derrick Stolee
2019-09-05 22:04 ` Taylor Blau [this message]
2019-09-05 22:04 ` [PATCH 3/3] commit-graph.c: handle corrupt/missing trees Taylor Blau
2019-09-06  6:19   ` Jeff King
2019-09-06 15:42     ` Taylor Blau
2019-09-06 17:34       ` Jeff King
2019-09-06 16:51     ` Derrick Stolee
2019-09-06 17:37       ` Jeff King
2019-09-06 16:57     ` Junio C Hamano
2019-09-06 17:11       ` Junio C Hamano
2019-09-06 17:30         ` Jeff King
2019-09-06 17:28       ` Jeff King
2019-09-09 17:55         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7fb6853ab31f35c27e5c220f11ea2de6e229a806.1567720960.git.me@ttaylorr.com \
    --to=me@ttaylorr.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).