Skip to content

Commit 24d4d38

Browse files
hanwengitster
hanwen
authored and
gitster
committed
reftable: fix resource leak in block.c error path
Add test coverage for corrupt zlib data. Fix memory leaks demonstrated by unittest. This problem was discovered by a Coverity scan. Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 32d9c0e commit 24d4d38

File tree

3 files changed

+97
-18
lines changed

3 files changed

+97
-18
lines changed

‎reftable/block.c‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -188,13 +188,16 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
188188
uint32_t full_block_size = table_block_size;
189189
uint8_t typ = block->data[header_off];
190190
uint32_t sz = get_be24(block->data + header_off + 1);
191-
191+
int err = 0;
192192
uint16_t restart_count = 0;
193193
uint32_t restart_start = 0;
194194
uint8_t *restart_bytes = NULL;
195+
uint8_t *uncompressed = NULL;
195196

196-
if (!reftable_is_block_type(typ))
197-
return REFTABLE_FORMAT_ERROR;
197+
if (!reftable_is_block_type(typ)) {
198+
err = REFTABLE_FORMAT_ERROR;
199+
goto done;
200+
}
198201

199202
if (typ == BLOCK_TYPE_LOG) {
200203
int block_header_skip = 4 + header_off;
@@ -203,7 +206,7 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
203206
uLongf src_len = block->len - block_header_skip;
204207
/* Log blocks specify the *uncompressed* size in their header.
205208
*/
206-
uint8_t *uncompressed = reftable_malloc(sz);
209+
uncompressed = reftable_malloc(sz);
207210

208211
/* Copy over the block header verbatim. It's not compressed. */
209212
memcpy(uncompressed, block->data, block_header_skip);
@@ -212,16 +215,19 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
212215
if (Z_OK !=
213216
uncompress2(uncompressed + block_header_skip, &dst_len,
214217
block->data + block_header_skip, &src_len)) {
215-
reftable_free(uncompressed);
216-
return REFTABLE_ZLIB_ERROR;
218+
err = REFTABLE_ZLIB_ERROR;
219+
goto done;
217220
}
218221

219-
if (dst_len + block_header_skip != sz)
220-
return REFTABLE_FORMAT_ERROR;
222+
if (dst_len + block_header_skip != sz) {
223+
err = REFTABLE_FORMAT_ERROR;
224+
goto done;
225+
}
221226

222227
/* We're done with the input data. */
223228
reftable_block_done(block);
224229
block->data = uncompressed;
230+
uncompressed = NULL;
225231
block->len = sz;
226232
block->source = malloc_block_source();
227233
full_block_size = src_len + block_header_skip;
@@ -251,7 +257,9 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
251257
br->restart_count = restart_count;
252258
br->restart_bytes = restart_bytes;
253259

254-
return 0;
260+
done:
261+
reftable_free(uncompressed);
262+
return err;
255263
}
256264

257265
static uint32_t block_reader_restart_offset(struct block_reader *br, int i)

‎reftable/reader.c‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -290,28 +290,33 @@ int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,
290290

291291
err = reader_get_block(r, &block, next_off, guess_block_size);
292292
if (err < 0)
293-
return err;
293+
goto done;
294294

295295
block_size = extract_block_size(block.data, &block_typ, next_off,
296296
r->version);
297-
if (block_size < 0)
298-
return block_size;
299-
297+
if (block_size < 0) {
298+
err = block_size;
299+
goto done;
300+
}
300301
if (want_typ != BLOCK_TYPE_ANY && block_typ != want_typ) {
301-
reftable_block_done(&block);
302-
return 1;
302+
err = 1;
303+
goto done;
303304
}
304305

305306
if (block_size > guess_block_size) {
306307
reftable_block_done(&block);
307308
err = reader_get_block(r, &block, next_off, block_size);
308309
if (err < 0) {
309-
return err;
310+
goto done;
310311
}
311312
}
312313

313-
return block_reader_init(br, &block, header_off, r->block_size,
314-
hash_size(r->hash_id));
314+
err = block_reader_init(br, &block, header_off, r->block_size,
315+
hash_size(r->hash_id));
316+
done:
317+
reftable_block_done(&block);
318+
319+
return err;
315320
}
316321

317322
static int table_iter_next_block(struct table_iter *dest,

‎reftable/readwrite_test.c‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,71 @@ static void test_log_write_read(void)
254254
reader_close(&rd);
255255
}
256256

257+
static void test_log_zlib_corruption(void)
258+
{
259+
struct reftable_write_options opts = {
260+
.block_size = 256,
261+
};
262+
struct reftable_iterator it = { 0 };
263+
struct reftable_reader rd = { 0 };
264+
struct reftable_block_source source = { 0 };
265+
struct strbuf buf = STRBUF_INIT;
266+
struct reftable_writer *w =
267+
reftable_new_writer(&strbuf_add_void, &buf, &opts);
268+
const struct reftable_stats *stats = NULL;
269+
uint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };
270+
uint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };
271+
char message[100] = { 0 };
272+
int err, i, n;
273+
274+
struct reftable_log_record log = {
275+
.refname = "refname",
276+
.value_type = REFTABLE_LOG_UPDATE,
277+
.value = {
278+
.update = {
279+
.new_hash = hash1,
280+
.old_hash = hash2,
281+
.name = "My Name",
282+
.email = "myname@invalid",
283+
.message = message,
284+
},
285+
},
286+
};
287+
288+
for (i = 0; i < sizeof(message) - 1; i++)
289+
message[i] = (uint8_t)(rand() % 64 + ' ');
290+
291+
reftable_writer_set_limits(w, 1, 1);
292+
293+
err = reftable_writer_add_log(w, &log);
294+
EXPECT_ERR(err);
295+
296+
n = reftable_writer_close(w);
297+
EXPECT(n == 0);
298+
299+
stats = writer_stats(w);
300+
EXPECT(stats->log_stats.blocks > 0);
301+
reftable_writer_free(w);
302+
w = NULL;
303+
304+
/* corrupt the data. */
305+
buf.buf[50] ^= 0x99;
306+
307+
block_source_from_strbuf(&source, &buf);
308+
309+
err = init_reader(&rd, &source, "file.log");
310+
EXPECT_ERR(err);
311+
312+
err = reftable_reader_seek_log(&rd, &it, "refname");
313+
EXPECT(err == REFTABLE_ZLIB_ERROR);
314+
315+
reftable_iterator_destroy(&it);
316+
317+
/* cleanup. */
318+
strbuf_release(&buf);
319+
reader_close(&rd);
320+
}
321+
257322
static void test_table_read_write_sequential(void)
258323
{
259324
char **names;
@@ -633,6 +698,7 @@ static void test_corrupt_table(void)
633698

634699
int readwrite_test_main(int argc, const char *argv[])
635700
{
701+
RUN_TEST(test_log_zlib_corruption);
636702
RUN_TEST(test_corrupt_table);
637703
RUN_TEST(test_corrupt_table_empty);
638704
RUN_TEST(test_log_write_read);

0 commit comments

Comments
 (0)