reftable: prevent 'update_index' changes after adding records
The function `reftable_writer_set_limits()` allows updating the 'min_update_index' and 'max_update_index' of a reftable writer. These values are written to both the writer's header and footer. Since the header is written during the first block write, any subsequent changes to the update index would create a mismatch between the header and footer values. The footer would contain the newer values while the header retained the original ones. To protect against this bug, prevent callers from updating these values after any record is written. To do this, modify the function to return an error whenever the limits are modified after any record adds. Check for record adds within `reftable_writer_set_limits()` by checking the `last_key` and `next` variable. The former is updated after each record added, but is reset at certain points. The latter is set after writing the first block. Modify all callers of the function to anticipate a return type and handle it accordingly. Add a unit test to also ensure the function returns the error as expected. Helped-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Karthik Nayak committed
Jan 22, 2025 at 06:35 UTC
017bd8923986acd4992fd21f3451fdd15ec6edce
6 files changed
+99
-21
refs/reftable-backend.c
+15
-5
@@ -1443,7 +1443,9 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
1443
* multiple entries. Each entry will contain a different update_index,
1444
* so set the limits accordingly.
1445
*/
1446
- reftable_writer_set_limits(writer, ts, ts + arg->max_index);
1446
+ ret = reftable_writer_set_limits(writer, ts, ts + arg->max_index);
1447
+ if (ret < 0)
1448
+ goto done;
1449
1450
for (i = 0; i < arg->updates_nr; i++) {
1451
struct reftable_transaction_update *tx_update = &arg->updates[i];
@@ -1766,7 +1768,9 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
1768
deletion_ts = creation_ts = reftable_stack_next_update_index(arg->be->stack);
1769
if (arg->delete_old)
1770
creation_ts++;
1769
- reftable_writer_set_limits(writer, deletion_ts, creation_ts);
1771
+ ret = reftable_writer_set_limits(writer, deletion_ts, creation_ts);
1772
+ if (ret < 0)
1773
+ goto done;
1774
1775
/*
1776
* Add the new reference. If this is a rename then we also delete the
@@ -2298,7 +2302,9 @@ static int write_reflog_existence_table(struct reftable_writer *writer,
2302
if (ret <= 0)
2303
goto done;
2304
2301
- reftable_writer_set_limits(writer, ts, ts);
2305
+ ret = reftable_writer_set_limits(writer, ts, ts);
2306
+ if (ret < 0)
2307
+ goto done;
2308
2309
/*
2310
* The existence entry has both old and new object ID set to the
@@ -2357,7 +2363,9 @@ static int write_reflog_delete_table(struct reftable_writer *writer, void *cb_da
2363
uint64_t ts = reftable_stack_next_update_index(arg->stack);
2364
int ret;
2365
2360
- reftable_writer_set_limits(writer, ts, ts);
2366
+ ret = reftable_writer_set_limits(writer, ts, ts);
2367
+ if (ret < 0)
2368
+ goto out;
2369
2370
ret = reftable_stack_init_log_iterator(arg->stack, &it);
2371
if (ret < 0)
@@ -2434,7 +2442,9 @@ static int write_reflog_expiry_table(struct reftable_writer *writer, void *cb_da
2442
if (arg->records[i].value_type == REFTABLE_LOG_UPDATE)
2443
live_records++;
2444
2437
- reftable_writer_set_limits(writer, ts, ts);
2445
+ ret = reftable_writer_set_limits(writer, ts, ts);
2446
+ if (ret < 0)
2447
+ return ret;
2448
2449
if (!is_null_oid(&arg->update_oid)) {
2450
struct reftable_ref_record ref = {0};
reftable/reftable-error.h
+1
@@ -30,6 +30,7 @@ enum reftable_error {
30
31
/* Misuse of the API:
32
* - on writing a record with NULL refname.
33
+ * - on writing a record before setting the writer limits.
34
* - on writing a reftable_ref_record outside the table limits
35
* - on writing a ref or log record before the stack's
36
* next_update_inde*x
reftable/reftable-writer.h
+14
-10
@@ -124,17 +124,21 @@ int reftable_writer_new(struct reftable_writer **out,
124
int (*flush_func)(void *),
125
void *writer_arg, const struct reftable_write_options *opts);
126
127
-/* Set the range of update indices for the records we will add. When writing a
128
- table into a stack, the min should be at least
129
- reftable_stack_next_update_index(), or REFTABLE_API_ERROR is returned.
130
-
131
- For transactional updates to a stack, typically min==max, and the
132
- update_index can be obtained by inspeciting the stack. When converting an
133
- existing ref database into a single reftable, this would be a range of
134
- update-index timestamps.
127
+/*
128
+ * Set the range of update indices for the records we will add. When writing a
129
+ * table into a stack, the min should be at least
130
+ * reftable_stack_next_update_index(), or REFTABLE_API_ERROR is returned.
131
+ *
132
+ * For transactional updates to a stack, typically min==max, and the
133
+ * update_index can be obtained by inspeciting the stack. When converting an
134
+ * existing ref database into a single reftable, this would be a range of
135
+ * update-index timestamps.
136
+ *
137
+ * The function should be called before adding any records to the writer. If not
138
+ * it will fail with REFTABLE_API_ERROR.
139
*/
136
-void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,
137
- uint64_t max);
140
+int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,
141
+ uint64_t max);
142
143
/*
144
Add a reftable_ref_record. The record should have names that come after
reftable/stack.c
+4
-2
@@ -1058,8 +1058,10 @@ static int stack_write_compact(struct reftable_stack *st,
1058
1059
for (size_t i = first; i <= last; i++)
1060
st->stats.bytes += st->readers[i]->size;
1061
- reftable_writer_set_limits(wr, st->readers[first]->min_update_index,
1062
- st->readers[last]->max_update_index);
1061
+ err = reftable_writer_set_limits(wr, st->readers[first]->min_update_index,
1062
+ st->readers[last]->max_update_index);
1063
+ if (err < 0)
1064
+ goto done;
1065
1066
err = reftable_merged_table_new(&mt, st->readers + first, subtabs_len,
1067
st->opts.hash_id);
reftable/writer.c
+14
-1
@@ -179,11 +179,24 @@ int reftable_writer_new(struct reftable_writer **out,
179
return 0;
180
}
181
182
-void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,
182
+int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,
183
uint64_t max)
184
{
185
+ /*
186
+ * Set the min/max update index limits for the reftable writer.
187
+ * This must be called before adding any records, since:
188
+ * - The 'next' field gets set after writing the first block.
189
+ * - The 'last_key' field updates with each new record (but resets
190
+ * after sections).
191
+ * Returns REFTABLE_API_ERROR if called after writing has begun.
192
+ */
193
+ if (w->next || w->last_key.len)
194
+ return REFTABLE_API_ERROR;
195
+
196
w->min_update_index = min;
197
w->max_update_index = max;
198
+
199
+ return 0;
200
}
201
202
static void writer_release(struct reftable_writer *w)
t/unit-tests/t-reftable-stack.c
+51
-3
@@ -103,7 +103,8 @@ static void t_read_file(void)
103
static int write_test_ref(struct reftable_writer *wr, void *arg)
104
{
105
struct reftable_ref_record *ref = arg;
106
- reftable_writer_set_limits(wr, ref->update_index, ref->update_index);
106
+ check(!reftable_writer_set_limits(wr, ref->update_index,
107
+ ref->update_index));
108
return reftable_writer_add_ref(wr, ref);
109
}
110
@@ -143,7 +144,8 @@ static int write_test_log(struct reftable_writer *wr, void *arg)
144
{
145
struct write_log_arg *wla = arg;
146
146
- reftable_writer_set_limits(wr, wla->update_index, wla->update_index);
147
+ check(!reftable_writer_set_limits(wr, wla->update_index,
148
+ wla->update_index));
149
return reftable_writer_add_log(wr, wla->log);
150
}
151
@@ -961,7 +963,7 @@ static void t_reflog_expire(void)
963
964
static int write_nothing(struct reftable_writer *wr, void *arg UNUSED)
965
{
964
- reftable_writer_set_limits(wr, 1, 1);
966
+ check(!reftable_writer_set_limits(wr, 1, 1));
967
return 0;
968
}
969
@@ -1369,11 +1371,57 @@ static void t_reftable_stack_reload_with_missing_table(void)
1371
clear_dir(dir);
1372
}
1373
1374
+static int write_limits_after_ref(struct reftable_writer *wr, void *arg)
1375
+{
1376
+ struct reftable_ref_record *ref = arg;
1377
+ check(!reftable_writer_set_limits(wr, ref->update_index, ref->update_index));
1378
+ check(!reftable_writer_add_ref(wr, ref));
1379
+ return reftable_writer_set_limits(wr, ref->update_index, ref->update_index);
1380
+}
1381
+
1382
+static void t_reftable_invalid_limit_updates(void)
1383
+{
1384
+ struct reftable_ref_record ref = {
1385
+ .refname = (char *) "HEAD",
1386
+ .update_index = 1,
1387
+ .value_type = REFTABLE_REF_SYMREF,
1388
+ .value.symref = (char *) "master",
1389
+ };
1390
+ struct reftable_write_options opts = {
1391
+ .default_permissions = 0660,
1392
+ };
1393
+ struct reftable_addition *add = NULL;
1394
+ char *dir = get_tmp_dir(__LINE__);
1395
+ struct reftable_stack *st = NULL;
1396
+ int err;
1397
+
1398
+ err = reftable_new_stack(&st, dir, &opts);
1399
+ check(!err);
1400
+
1401
+ reftable_addition_destroy(add);
1402
+
1403
+ err = reftable_stack_new_addition(&add, st, 0);
1404
+ check(!err);
1405
+
1406
+ /*
1407
+ * write_limits_after_ref also updates the update indexes after adding
1408
+ * the record. This should cause an err to be returned, since the limits
1409
+ * must be set at the start.
1410
+ */
1411
+ err = reftable_addition_add(add, write_limits_after_ref, &ref);
1412
+ check_int(err, ==, REFTABLE_API_ERROR);
1413
+
1414
+ reftable_addition_destroy(add);
1415
+ reftable_stack_destroy(st);
1416
+ clear_dir(dir);
1417
+}
1418
+
1419
int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
1420
{
1421
TEST(t_empty_add(), "empty addition to stack");
1422
TEST(t_read_file(), "read_lines works");
1423
TEST(t_reflog_expire(), "expire reflog entries");
1424
+ TEST(t_reftable_invalid_limit_updates(), "prevent limit updates after adding records");
1425
TEST(t_reftable_stack_add(), "add multiple refs and logs to stack");
1426
TEST(t_reftable_stack_add_one(), "add a single ref record to stack");
1427
TEST(t_reftable_stack_add_performs_auto_compaction(), "addition to stack triggers auto-compaction");