reftable/record: don't `BUG()` in `reftable_record_cmp()`
The reftable library aborts with a bug in case `reftable_record_cmp()` is invoked with two records of differing types. This would cause the program to die without the caller being able to handle the error, which is not something we want in the context of library code. And it ties us to the Git codebase. Refactor the code such that `reftable_record_cmp()` returns an error code separate from the actual comparison result. This requires us to also adapt some callers up the callchain in a similar fashion. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Patrick Steinhardt committed
Feb 18, 2025 at 10:20 UTC
6f6127decde6785b9ba5f22a07a7754d1fda1a59
7 files changed
+92
-34
reftable/merged.c
+15
-5
@@ -66,8 +66,11 @@ static int merged_iter_seek(struct merged_iter *mi, struct reftable_record *want
66
int err;
67
68
mi->advance_index = -1;
69
- while (!merged_iter_pqueue_is_empty(mi->pq))
70
- merged_iter_pqueue_remove(&mi->pq);
69
+ while (!merged_iter_pqueue_is_empty(mi->pq)) {
70
+ err = merged_iter_pqueue_remove(&mi->pq, NULL);
71
+ if (err < 0)
72
+ return err;
73
+ }
74
75
for (size_t i = 0; i < mi->subiters_len; i++) {
76
err = iterator_seek(&mi->subiters[i].iter, want);
@@ -120,7 +123,9 @@ static int merged_iter_next_entry(struct merged_iter *mi,
123
if (empty)
124
return 1;
125
123
- entry = merged_iter_pqueue_remove(&mi->pq);
126
+ err = merged_iter_pqueue_remove(&mi->pq, &entry);
127
+ if (err < 0)
128
+ return err;
129
130
/*
131
One can also use reftable as datacenter-local storage, where the ref
@@ -134,11 +139,16 @@ static int merged_iter_next_entry(struct merged_iter *mi,
139
struct pq_entry top = merged_iter_pqueue_top(mi->pq);
140
int cmp;
141
137
- cmp = reftable_record_cmp(top.rec, entry.rec);
142
+ err = reftable_record_cmp(top.rec, entry.rec, &cmp);
143
+ if (err < 0)
144
+ return err;
145
if (cmp > 0)
146
break;
147
141
- merged_iter_pqueue_remove(&mi->pq);
148
+ err = merged_iter_pqueue_remove(&mi->pq, NULL);
149
+ if (err < 0)
150
+ return err;
151
+
152
err = merged_iter_advance_subiter(mi, top.index);
153
if (err < 0)
154
return err;
reftable/pq.c
+29
-7
@@ -15,13 +15,18 @@ https://developers.google.com/open-source/licenses/bsd
15
16
int pq_less(struct pq_entry *a, struct pq_entry *b)
17
{
18
- int cmp = reftable_record_cmp(a->rec, b->rec);
18
+ int cmp, err;
19
+
20
+ err = reftable_record_cmp(a->rec, b->rec, &cmp);
21
+ if (err < 0)
22
+ return err;
23
+
24
if (cmp == 0)
25
return a->index > b->index;
26
return cmp < 0;
27
}
28
24
-struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)
29
+int merged_iter_pqueue_remove(struct merged_iter_pqueue *pq, struct pq_entry *out)
30
{
31
size_t i = 0;
32
struct pq_entry e = pq->heap[0];
@@ -32,17 +37,34 @@ struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)
37
size_t min = i;
38
size_t j = 2 * i + 1;
39
size_t k = 2 * i + 2;
35
- if (j < pq->len && pq_less(&pq->heap[j], &pq->heap[i]))
36
- min = j;
37
- if (k < pq->len && pq_less(&pq->heap[k], &pq->heap[min]))
38
- min = k;
40
+ int cmp;
41
+
42
+ if (j < pq->len) {
43
+ cmp = pq_less(&pq->heap[j], &pq->heap[i]);
44
+ if (cmp < 0)
45
+ return -1;
46
+ else if (cmp)
47
+ min = j;
48
+ }
49
+
50
+ if (k < pq->len) {
51
+ cmp = pq_less(&pq->heap[k], &pq->heap[min]);
52
+ if (cmp < 0)
53
+ return -1;
54
+ else if (cmp)
55
+ min = k;
56
+ }
57
+
58
if (min == i)
59
break;
60
SWAP(pq->heap[i], pq->heap[min]);
61
i = min;
62
}
63
45
- return e;
64
+ if (out)
65
+ *out = e;
66
+
67
+ return 0;
68
}
69
70
int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry *e)
reftable/pq.h
+1
-1
@@ -22,7 +22,7 @@ struct merged_iter_pqueue {
22
size_t cap;
23
};
24
25
-struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq);
25
+int merged_iter_pqueue_remove(struct merged_iter_pqueue *pq, struct pq_entry *out);
26
int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry *e);
27
void merged_iter_pqueue_release(struct merged_iter_pqueue *pq);
28
int pq_less(struct pq_entry *a, struct pq_entry *b);
reftable/record.c
+6
-4
@@ -1195,12 +1195,14 @@ int reftable_record_is_deletion(struct reftable_record *rec)
1195
reftable_record_data(rec));
1196
}
1197
1198
-int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b)
1198
+int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b,
1199
+ int *cmp)
1200
{
1201
if (a->type != b->type)
1201
- BUG("cannot compare reftable records of different type");
1202
- return reftable_record_vtable(a)->cmp(
1203
- reftable_record_data(a), reftable_record_data(b));
1202
+ return -1;
1203
+ *cmp = reftable_record_vtable(a)->cmp(reftable_record_data(a),
1204
+ reftable_record_data(b));
1205
+ return 0;
1206
}
1207
1208
int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, uint32_t hash_size)
reftable/record.h
+1
-1
@@ -134,7 +134,7 @@ struct reftable_record {
134
int reftable_record_init(struct reftable_record *rec, uint8_t typ);
135
136
/* see struct record_vtable */
137
-int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b);
137
+int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b, int *cmp);
138
int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, uint32_t hash_size);
139
int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest);
140
int reftable_record_copy_from(struct reftable_record *rec,
t/unit-tests/t-reftable-pq.c
+12
-4
@@ -21,7 +21,9 @@ static void merged_iter_pqueue_check(const struct merged_iter_pqueue *pq)
21
22
static int pq_entry_equal(struct pq_entry *a, struct pq_entry *b)
23
{
24
- return !reftable_record_cmp(a->rec, b->rec) && (a->index == b->index);
24
+ int cmp;
25
+ check(!reftable_record_cmp(a->rec, b->rec, &cmp));
26
+ return !cmp && (a->index == b->index);
27
}
28
29
static void t_pq_record(void)
@@ -49,7 +51,9 @@ static void t_pq_record(void)
51
52
while (!merged_iter_pqueue_is_empty(pq)) {
53
struct pq_entry top = merged_iter_pqueue_top(pq);
52
- struct pq_entry e = merged_iter_pqueue_remove(&pq);
54
+ struct pq_entry e;
55
+
56
+ check(!merged_iter_pqueue_remove(&pq, &e));
57
merged_iter_pqueue_check(&pq);
58
59
check(pq_entry_equal(&top, &e));
@@ -90,7 +94,9 @@ static void t_pq_index(void)
94
95
for (i = N - 1; i > 0; i--) {
96
struct pq_entry top = merged_iter_pqueue_top(pq);
93
- struct pq_entry e = merged_iter_pqueue_remove(&pq);
97
+ struct pq_entry e;
98
+
99
+ check(!merged_iter_pqueue_remove(&pq, &e));
100
merged_iter_pqueue_check(&pq);
101
102
check(pq_entry_equal(&top, &e));
@@ -129,7 +135,9 @@ static void t_merged_iter_pqueue_top(void)
135
136
for (i = N - 1; i > 0; i--) {
137
struct pq_entry top = merged_iter_pqueue_top(pq);
132
- struct pq_entry e = merged_iter_pqueue_remove(&pq);
138
+ struct pq_entry e;
139
+
140
+ check(!merged_iter_pqueue_remove(&pq, &e));
141
142
merged_iter_pqueue_check(&pq);
143
check(pq_entry_equal(&top, &e));
t/unit-tests/t-reftable-record.c
+28
-12
@@ -100,16 +100,20 @@ static void t_reftable_ref_record_comparison(void)
100
.u.ref.value.symref = (char *) "refs/heads/master",
101
},
102
};
103
+ int cmp;
104
105
check(!reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
105
- check(!reftable_record_cmp(&in[0], &in[1]));
106
+ check(!reftable_record_cmp(&in[0], &in[1], &cmp));
107
+ check(!cmp);
108
109
check(!reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1));
108
- check_int(reftable_record_cmp(&in[1], &in[2]), >, 0);
110
+ check(!reftable_record_cmp(&in[1], &in[2], &cmp));
111
+ check_int(cmp, >, 0);
112
113
in[1].u.ref.value_type = in[0].u.ref.value_type;
114
check(reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
112
- check(!reftable_record_cmp(&in[0], &in[1]));
115
+ check(!reftable_record_cmp(&in[0], &in[1], &cmp));
116
+ check(!cmp);
117
}
118
119
static void t_reftable_ref_record_compare_name(void)
@@ -209,17 +213,20 @@ static void t_reftable_log_record_comparison(void)
213
.u.log.update_index = 22,
214
},
215
};
216
+ int cmp;
217
218
check(!reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
219
check(!reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1));
215
- check_int(reftable_record_cmp(&in[1], &in[2]), >, 0);
220
+ check(!reftable_record_cmp(&in[1], &in[2], &cmp));
221
+ check_int(cmp, >, 0);
222
/* comparison should be reversed for equal keys, because
223
* comparison is now performed on the basis of update indices */
218
- check_int(reftable_record_cmp(&in[0], &in[1]), <, 0);
224
+ check(!reftable_record_cmp(&in[0], &in[1], &cmp));
225
+ check_int(cmp, <, 0);
226
227
in[1].u.log.update_index = in[0].u.log.update_index;
228
check(reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
222
- check(!reftable_record_cmp(&in[0], &in[1]));
229
+ check(!reftable_record_cmp(&in[0], &in[1], &cmp));
230
}
231
232
static void t_reftable_log_record_compare_key(void)
@@ -396,16 +403,20 @@ static void t_reftable_obj_record_comparison(void)
403
.u.obj.hash_prefix_len = 5,
404
},
405
};
406
+ int cmp;
407
408
check(!reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
401
- check(!reftable_record_cmp(&in[0], &in[1]));
409
+ check(!reftable_record_cmp(&in[0], &in[1], &cmp));
410
+ check(!cmp);
411
412
check(!reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1));
404
- check_int(reftable_record_cmp(&in[1], &in[2]), >, 0);
413
+ check(!reftable_record_cmp(&in[1], &in[2], &cmp));
414
+ check_int(cmp, >, 0);
415
416
in[1].u.obj.offset_len = in[0].u.obj.offset_len;
417
check(reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
408
- check(!reftable_record_cmp(&in[0], &in[1]));
418
+ check(!reftable_record_cmp(&in[0], &in[1], &cmp));
419
+ check(!cmp);
420
}
421
422
static void t_reftable_obj_record_roundtrip(void)
@@ -486,19 +497,24 @@ static void t_reftable_index_record_comparison(void)
497
.u.idx.last_key = REFTABLE_BUF_INIT,
498
},
499
};
500
+ int cmp;
501
+
502
check(!reftable_buf_addstr(&in[0].u.idx.last_key, "refs/heads/master"));
503
check(!reftable_buf_addstr(&in[1].u.idx.last_key, "refs/heads/master"));
504
check(!reftable_buf_addstr(&in[2].u.idx.last_key, "refs/heads/branch"));
505
506
check(!reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
494
- check(!reftable_record_cmp(&in[0], &in[1]));
507
+ check(!reftable_record_cmp(&in[0], &in[1], &cmp));
508
+ check(!cmp);
509
510
check(!reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1));
497
- check_int(reftable_record_cmp(&in[1], &in[2]), >, 0);
511
+ check(!reftable_record_cmp(&in[1], &in[2], &cmp));
512
+ check_int(cmp, >, 0);
513
514
in[1].u.idx.offset = in[0].u.idx.offset;
515
check(reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
501
- check(!reftable_record_cmp(&in[0], &in[1]));
516
+ check(!reftable_record_cmp(&in[0], &in[1], &cmp));
517
+ check(!cmp);
518
519
for (size_t i = 0; i < ARRAY_SIZE(in); i++)
520
reftable_record_release(&in[i]);