Merge pull request #4045 from lhchavez/fix-unpack-double-free

Fix unpack double free

3 files changed, 68 insertions, 8 deletions

diff --git a/src/indexer.c b/src/indexer.c
index 7eec0d612..a5e842272 100644
--- a/src/indexer.c
+++ b/src/indexer.c
@@ -844,6 +844,7 @@ static int fix_thin_pack(git_indexer *idx, git_transfer_progress *stats)
 static int resolve_deltas(git_indexer *idx, git_transfer_progress *stats)
 {
 	unsigned int i;
+	int error;
 	struct delta_info *delta;
 	int progressed = 0, non_null = 0, progress_cb_result;
 
@@ -858,8 +859,13 @@ static int resolve_deltas(git_indexer *idx, git_transfer_progress *stats)
 
 			non_null = 1;
 			idx->off = delta->delta_off;
-			if (git_packfile_unpack(&obj, idx->pack, &idx->off) < 0)
-				continue;
+			if ((error = git_packfile_unpack(&obj, idx->pack, &idx->off)) < 0) {
+				if (error == GIT_PASSTHROUGH) {
+					/* We have not seen the base object, we'll try again later. */
+					continue;
+				}
+				return -1;
+			}
 
 			if (hash_and_save(idx, &obj, delta->delta_off) < 0)
 				continue;
diff --git a/src/pack.c b/src/pack.c
index b87d22d53..9ed3ec1af 100644
--- a/src/pack.c
+++ b/src/pack.c
@@ -716,8 +716,11 @@ int git_packfile_unpack(
 		error = packfile_unpack_compressed(&delta, p, &w_curs, &curpos, elem->size, elem->type);
 		git_mwindow_close(&w_curs);
 
-		if (error < 0)
+		if (error < 0) {
+			/* We have transferred ownership of the data to the cache. */
+			obj->data = NULL;
 			break;
+		}
 
 		/* the current object becomes the new base, on which we apply the delta */
 		base = *obj;
diff --git a/tests/pack/indexer.c b/tests/pack/indexer.c
index 4d2d9f60e..f3c2204bd 100644
--- a/tests/pack/indexer.c
+++ b/tests/pack/indexer.c
@@ -41,6 +41,29 @@ static const unsigned char thin_pack[] = {
 static const unsigned int thin_pack_len = 78;
 
 /*
+ * Packfile with one object. It references an object which is not in the
+ * packfile and has a corrupt length (states the deltified stream is 1 byte
+ * long, where it is actually 6).
+ */
+static const unsigned char corrupt_thin_pack[] = {
+  0x50, 0x41, 0x43, 0x4b, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x01,
+  0x71, 0xe6, 0x8f, 0xe8, 0x12, 0x9b, 0x54, 0x6b, 0x10, 0x1a, 0xee, 0x95,
+  0x10, 0xc5, 0x32, 0x8e, 0x7f, 0x21, 0xca, 0x1d, 0x18, 0x78, 0x9c, 0x63,
+  0x62, 0x66, 0x4e, 0xcb, 0xcf, 0x07, 0x00, 0x02, 0xac, 0x01, 0x4d, 0x07,
+  0x67, 0x03, 0xc5, 0x40, 0x99, 0x49, 0xb1, 0x3b, 0x7d, 0xae, 0x9b, 0x0e,
+  0xdd, 0xde, 0xc6, 0x76, 0x43, 0x24, 0x64
+};
+static const unsigned int corrupt_thin_pack_len = 67;
+
+/*
+ * Packfile with a missing trailer.
+ */
+static const unsigned char missing_trailer_pack[] = {
+  0x50, 0x41, 0x43, 0x4b, 0x00, 0x00, 0x00, 0x03, 0x00, 0x50, 0xf4, 0x3b,
+};
+static const unsigned int missing_trailer_pack_len = 12;
+
+/*
  * Packfile that causes the packfile stream to open in a way in which it leaks
  * the stream reader.
  */
@@ -71,14 +94,14 @@ void test_pack_indexer__out_of_order(void)
 	git_indexer_free(idx);
 }
 
-void test_pack_indexer__leaky(void)
+void test_pack_indexer__missing_trailer(void)
 {
 	git_indexer *idx = 0;
 	git_transfer_progress stats = { 0 };
 
 	cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL));
 	cl_git_pass(git_indexer_append(
-		idx, leaky_pack, leaky_pack_len, &stats));
+		idx, missing_trailer_pack, missing_trailer_pack_len, &stats));
 	cl_git_fail(git_indexer_commit(idx, &stats));
 
 	cl_assert(giterr_last() != NULL);
@@ -87,15 +110,14 @@ void test_pack_indexer__leaky(void)
 	git_indexer_free(idx);
 }
 
-void test_pack_indexer__missing_trailer(void)
+void test_pack_indexer__leaky(void)
 {
 	git_indexer *idx = 0;
 	git_transfer_progress stats = { 0 };
 
 	cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL));
-	/* Truncate a valid packfile */
 	cl_git_pass(git_indexer_append(
-		idx, out_of_order_pack, out_of_order_pack_len - 20, &stats));
+		idx, leaky_pack, leaky_pack_len, &stats));
 	cl_git_fail(git_indexer_commit(idx, &stats));
 
 	cl_assert(giterr_last() != NULL);
@@ -170,6 +192,35 @@ void test_pack_indexer__fix_thin(void)
 	}
 }
 
+void test_pack_indexer__corrupt_length(void)
+{
+	git_indexer *idx = NULL;
+	git_transfer_progress stats = { 0 };
+	git_repository *repo;
+	git_odb *odb;
+	git_oid id, should_id;
+
+	cl_git_pass(git_repository_init(&repo, "thin.git", true));
+	cl_git_pass(git_repository_odb(&odb, repo));
+
+	/* Store the missing base into your ODB so the indexer can fix the pack */
+	cl_git_pass(git_odb_write(&id, odb, base_obj, base_obj_len, GIT_OBJ_BLOB));
+	git_oid_fromstr(&should_id, "e68fe8129b546b101aee9510c5328e7f21ca1d18");
+	cl_assert_equal_oid(&should_id, &id);
+
+	cl_git_pass(git_indexer_new(&idx, ".", 0, odb, NULL, NULL));
+	cl_git_pass(git_indexer_append(
+		idx, corrupt_thin_pack, corrupt_thin_pack_len, &stats));
+	cl_git_fail(git_indexer_commit(idx, &stats));
+
+	cl_assert(giterr_last() != NULL);
+	cl_assert_equal_i(giterr_last()->klass, GITERR_ZLIB);
+
+	git_indexer_free(idx);
+	git_odb_free(odb);
+	git_repository_free(repo);
+}
+
 static int find_tmp_file_recurs(void *opaque, git_buf *path)
 {
 	int error = 0;