Commit c3514b0b82e90e660c996e81a525d42c025503ab
2017-12-23T14:59:07
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.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150
diff --git a/src/indexer.c b/src/indexer.c
index 7eec0d6..a5e8422 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 b87d22d..9ed3ec1 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 4d2d9f6..f3c2204 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;