Skip to content

Commit 3423051

Browse files
gitstergitster
committed
Merge branch 'hn/reftable-coverity-fixes'
Problems identified by Coverity in the reftable code have been corrected. * hn/reftable-coverity-fixes: reftable: add print functions to the record types reftable: make reftable_record a tagged union reftable: remove outdated file reftable.c reftable: implement record equality generically reftable: make reftable-record.h function signatures const correct reftable: handle null refnames in reftable_ref_record_equal reftable: drop stray printf in readwrite_test reftable: order unittests by complexity reftable: all xxx_free() functions accept NULL arguments reftable: fix resource warning reftable: ignore remove() return value in stack_test.c reftable: check reftable_stack_auto_compact() return value reftable: fix resource leak blocksource.c reftable: fix resource leak in block.c error path reftable: fix OOB stack write in print functions
2 parents dd77ff8 + 01033de commit 3423051

File tree

19 files changed

+620
-529
lines changed

19 files changed

+620
-529
lines changed

‎reftable/block.c‎

Lines changed: 18 additions & 10 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)
@@ -413,7 +421,7 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,
413421
done:
414422
strbuf_release(&key);
415423
strbuf_release(&next.last_key);
416-
reftable_record_destroy(&rec);
424+
reftable_record_release(&rec);
417425

418426
return err;
419427
}

‎reftable/block_test.c‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,9 @@ static void test_block_read_write(void)
2626
struct block_writer bw = {
2727
.last_key = STRBUF_INIT,
2828
};
29-
struct reftable_ref_record ref = { NULL };
30-
struct reftable_record rec = { NULL };
29+
struct reftable_record rec = {
30+
.type = BLOCK_TYPE_REF,
31+
};
3132
int i = 0;
3233
int n;
3334
struct block_reader br = { 0 };
@@ -40,22 +41,21 @@ static void test_block_read_write(void)
4041
block.source = malloc_block_source();
4142
block_writer_init(&bw, BLOCK_TYPE_REF, block.data, block_size,
4243
header_off, hash_size(GIT_SHA1_FORMAT_ID));
43-
reftable_record_from_ref(&rec, &ref);
4444

4545
for (i = 0; i < N; i++) {
4646
char name[100];
4747
uint8_t hash[GIT_SHA1_RAWSZ];
4848
snprintf(name, sizeof(name), "branch%02d", i);
4949
memset(hash, i, sizeof(hash));
5050

51-
ref.refname = name;
52-
ref.value_type = REFTABLE_REF_VAL1;
53-
ref.value.val1 = hash;
51+
rec.u.ref.refname = name;
52+
rec.u.ref.value_type = REFTABLE_REF_VAL1;
53+
rec.u.ref.value.val1 = hash;
5454

5555
names[i] = xstrdup(name);
5656
n = block_writer_add(&bw, &rec);
57-
ref.refname = NULL;
58-
ref.value_type = REFTABLE_REF_DELETION;
57+
rec.u.ref.refname = NULL;
58+
rec.u.ref.value_type = REFTABLE_REF_DELETION;
5959
EXPECT(n == 0);
6060
}
6161

@@ -74,7 +74,7 @@ static void test_block_read_write(void)
7474
if (r > 0) {
7575
break;
7676
}
77-
EXPECT_STREQ(names[j], ref.refname);
77+
EXPECT_STREQ(names[j], rec.u.ref.refname);
7878
j++;
7979
}
8080

@@ -92,15 +92,15 @@ static void test_block_read_write(void)
9292
n = block_iter_next(&it, &rec);
9393
EXPECT(n == 0);
9494

95-
EXPECT_STREQ(names[i], ref.refname);
95+
EXPECT_STREQ(names[i], rec.u.ref.refname);
9696

9797
want.len--;
9898
n = block_reader_seek(&br, &it, &want);
9999
EXPECT(n == 0);
100100

101101
n = block_iter_next(&it, &rec);
102102
EXPECT(n == 0);
103-
EXPECT_STREQ(names[10 * (i / 10)], ref.refname);
103+
EXPECT_STREQ(names[10 * (i / 10)], rec.u.ref.refname);
104104

105105
block_iter_close(&it);
106106
}

‎reftable/blocksource.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,10 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,
134134
}
135135

136136
err = fstat(fd, &st);
137-
if (err < 0)
138-
return -1;
137+
if (err < 0) {
138+
close(fd);
139+
return REFTABLE_IO_ERROR;
140+
}
139141

140142
p = reftable_calloc(sizeof(struct file_block_source));
141143
p->size = st.st_size;

‎reftable/generic.c‎

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ license that can be found in the LICENSE file or at
77
*/
88

99
#include "basics.h"
10+
#include "constants.h"
1011
#include "record.h"
1112
#include "generic.h"
1213
#include "reftable-iterator.h"
@@ -15,23 +16,21 @@ license that can be found in the LICENSE file or at
1516
int reftable_table_seek_ref(struct reftable_table *tab,
1617
struct reftable_iterator *it, const char *name)
1718
{
18-
struct reftable_ref_record ref = {
19-
.refname = (char *)name,
20-
};
21-
struct reftable_record rec = { NULL };
22-
reftable_record_from_ref(&rec, &ref);
19+
struct reftable_record rec = { .type = BLOCK_TYPE_REF,
20+
.u.ref = {
21+
.refname = (char *)name,
22+
} };
2323
return tab->ops->seek_record(tab->table_arg, it, &rec);
2424
}
2525

2626
int reftable_table_seek_log(struct reftable_table *tab,
2727
struct reftable_iterator *it, const char *name)
2828
{
29-
struct reftable_log_record log = {
30-
.refname = (char *)name,
31-
.update_index = ~((uint64_t)0),
32-
};
33-
struct reftable_record rec = { NULL };
34-
reftable_record_from_log(&rec, &log);
29+
struct reftable_record rec = { .type = BLOCK_TYPE_LOG,
30+
.u.log = {
31+
.refname = (char *)name,
32+
.update_index = ~((uint64_t)0),
33+
} };
3534
return tab->ops->seek_record(tab->table_arg, it, &rec);
3635
}
3736

@@ -129,17 +128,25 @@ void reftable_iterator_destroy(struct reftable_iterator *it)
129128
int reftable_iterator_next_ref(struct reftable_iterator *it,
130129
struct reftable_ref_record *ref)
131130
{
132-
struct reftable_record rec = { NULL };
133-
reftable_record_from_ref(&rec, ref);
134-
return iterator_next(it, &rec);
131+
struct reftable_record rec = {
132+
.type = BLOCK_TYPE_REF,
133+
.u.ref = *ref,
134+
};
135+
int err = iterator_next(it, &rec);
136+
*ref = rec.u.ref;
137+
return err;
135138
}
136139

137140
int reftable_iterator_next_log(struct reftable_iterator *it,
138141
struct reftable_log_record *log)
139142
{
140-
struct reftable_record rec = { NULL };
141-
reftable_record_from_log(&rec, log);
142-
return iterator_next(it, &rec);
143+
struct reftable_record rec = {
144+
.type = BLOCK_TYPE_LOG,
145+
.u.log = *log,
146+
};
147+
int err = iterator_next(it, &rec);
148+
*log = rec.u.log;
149+
return err;
143150
}
144151

145152
int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)

‎reftable/iter.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ static int filtering_ref_iterator_next(void *iter_arg,
3232
struct reftable_record *rec)
3333
{
3434
struct filtering_ref_iterator *fri = iter_arg;
35-
struct reftable_ref_record *ref = rec->data;
35+
struct reftable_ref_record *ref = &rec->u.ref;
3636
int err = 0;
3737
while (1) {
3838
err = reftable_iterator_next_ref(&fri->it, ref);
@@ -127,7 +127,7 @@ static int indexed_table_ref_iter_next_block(struct indexed_table_ref_iter *it)
127127
static int indexed_table_ref_iter_next(void *p, struct reftable_record *rec)
128128
{
129129
struct indexed_table_ref_iter *it = p;
130-
struct reftable_ref_record *ref = rec->data;
130+
struct reftable_ref_record *ref = &rec->u.ref;
131131

132132
while (1) {
133133
int err = block_iter_next(&it->cur, rec);

‎reftable/merged.c‎

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ static int merged_iter_init(struct merged_iter *mi)
3030

3131
if (err > 0) {
3232
reftable_iterator_destroy(&mi->stack[i]);
33-
reftable_record_destroy(&rec);
33+
reftable_record_release(&rec);
3434
} else {
3535
struct pq_entry e = {
3636
.rec = rec,
@@ -57,18 +57,17 @@ static void merged_iter_close(void *p)
5757
static int merged_iter_advance_nonnull_subiter(struct merged_iter *mi,
5858
size_t idx)
5959
{
60-
struct reftable_record rec = reftable_new_record(mi->typ);
6160
struct pq_entry e = {
62-
.rec = rec,
61+
.rec = reftable_new_record(mi->typ),
6362
.index = idx,
6463
};
65-
int err = iterator_next(&mi->stack[idx], &rec);
64+
int err = iterator_next(&mi->stack[idx], &e.rec);
6665
if (err < 0)
6766
return err;
6867

6968
if (err > 0) {
7069
reftable_iterator_destroy(&mi->stack[idx]);
71-
reftable_record_destroy(&rec);
70+
reftable_record_release(&e.rec);
7271
return 0;
7372
}
7473

@@ -126,11 +125,11 @@ static int merged_iter_next_entry(struct merged_iter *mi,
126125
if (err < 0) {
127126
return err;
128127
}
129-
reftable_record_destroy(&top.rec);
128+
reftable_record_release(&top.rec);
130129
}
131130

132131
reftable_record_copy_from(rec, &entry.rec, hash_size(mi->hash_id));
133-
reftable_record_destroy(&entry.rec);
132+
reftable_record_release(&entry.rec);
134133
strbuf_release(&entry_key);
135134
return 0;
136135
}
@@ -290,24 +289,24 @@ int reftable_merged_table_seek_ref(struct reftable_merged_table *mt,
290289
struct reftable_iterator *it,
291290
const char *name)
292291
{
293-
struct reftable_ref_record ref = {
294-
.refname = (char *)name,
292+
struct reftable_record rec = {
293+
.type = BLOCK_TYPE_REF,
294+
.u.ref = {
295+
.refname = (char *)name,
296+
},
295297
};
296-
struct reftable_record rec = { NULL };
297-
reftable_record_from_ref(&rec, &ref);
298298
return merged_table_seek_record(mt, it, &rec);
299299
}
300300

301301
int reftable_merged_table_seek_log_at(struct reftable_merged_table *mt,
302302
struct reftable_iterator *it,
303303
const char *name, uint64_t update_index)
304304
{
305-
struct reftable_log_record log = {
306-
.refname = (char *)name,
307-
.update_index = update_index,
308-
};
309-
struct reftable_record rec = { NULL };
310-
reftable_record_from_log(&rec, &log);
305+
struct reftable_record rec = { .type = BLOCK_TYPE_LOG,
306+
.u.log = {
307+
.refname = (char *)name,
308+
.update_index = update_index,
309+
} };
311310
return merged_table_seek_record(mt, it, &rec);
312311
}
313312

‎reftable/pq.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)
7474
void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, struct pq_entry e)
7575
{
7676
int i = 0;
77+
7778
if (pq->len == pq->cap) {
7879
pq->cap = 2 * pq->cap + 1;
7980
pq->heap = reftable_realloc(pq->heap,
@@ -98,7 +99,7 @@ void merged_iter_pqueue_release(struct merged_iter_pqueue *pq)
9899
{
99100
int i = 0;
100101
for (i = 0; i < pq->len; i++) {
101-
reftable_record_destroy(&pq->heap[i].rec);
102+
reftable_record_release(&pq->heap[i].rec);
102103
}
103104
FREE_AND_NULL(pq->heap);
104105
pq->len = pq->cap = 0;

‎reftable/pq_test.c‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ static void test_pq(void)
3131
int N = ARRAY_SIZE(names) - 1;
3232

3333
struct merged_iter_pqueue pq = { NULL };
34-
const char *last = NULL;
34+
char *last = NULL;
3535

3636
int i = 0;
3737
for (i = 0; i < N; i++) {
@@ -42,32 +42,29 @@ static void test_pq(void)
4242

4343
i = 1;
4444
do {
45-
struct reftable_record rec =
46-
reftable_new_record(BLOCK_TYPE_REF);
47-
struct pq_entry e = { 0 };
48-
49-
reftable_record_as_ref(&rec)->refname = names[i];
50-
e.rec = rec;
45+
struct pq_entry e = { .rec = { .type = BLOCK_TYPE_REF,
46+
.u.ref = {
47+
.refname = names[i],
48+
} } };
5149
merged_iter_pqueue_add(&pq, e);
5250
merged_iter_pqueue_check(pq);
5351
i = (i * 7) % N;
5452
} while (i != 1);
5553

5654
while (!merged_iter_pqueue_is_empty(pq)) {
5755
struct pq_entry e = merged_iter_pqueue_remove(&pq);
58-
struct reftable_ref_record *ref =
59-
reftable_record_as_ref(&e.rec);
60-
56+
struct reftable_record *rec = &e.rec;
6157
merged_iter_pqueue_check(pq);
6258

59+
EXPECT(reftable_record_type(rec) == BLOCK_TYPE_REF);
6360
if (last) {
64-
EXPECT(strcmp(last, ref->refname) < 0);
61+
EXPECT(strcmp(last, rec->u.ref.refname) < 0);
6562
}
66-
last = ref->refname;
67-
ref->refname = NULL;
68-
reftable_free(ref);
63+
// this is names[i], so don't dealloc.
64+
last = rec->u.ref.refname;
65+
rec->u.ref.refname = NULL;
66+
reftable_record_release(rec);
6967
}
70-
7168
for (i = 0; i < N; i++) {
7269
reftable_free(names[i]);
7370
}

0 commit comments

Comments
 (0)