Fix unpack double free

If an element has been cached, but then the call to
packfile_unpack_compressed() fails, the very next thing that happens is
that its data is freed and then the element is not removed from the
cache, which frees the data again.

This change sets obj->data to NULL to avoid the double-free. It also
stops trying to resolve deltas after two continuous failed rounds of
resolution, and adds a test for this.

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;