| Message ID | 20241205-424-reftable-writer-add-check-for-limits-v1-1-b287b055204e@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable/writer: ensure valid range for log's update_index | expand |
On Thu, Dec 05, 2024 at 04:49:57PM +0100, Karthik Nayak wrote: > Each reftable addition has an associated update_index. While writing > refs, the update_index is verified to be within the range of the > reftable writer, i.e. `writer.min_update_index < ref.update_index` and > `writer.max_update_index > ref.update_index`. These should probably be `<=` and `>=`, respectively. > diff --git a/reftable/writer.c b/reftable/writer.c > index fd136794d5a27b33b5017f36fbd6b095ab8dac5b..f87086777cd20a9890a63f10c5d6932310dd5610 100644 > --- a/reftable/writer.c > +++ b/reftable/writer.c > @@ -412,6 +412,18 @@ int reftable_writer_add_log(struct reftable_writer *w, > if (log->value_type == REFTABLE_LOG_DELETION) > return reftable_writer_add_log_verbatim(w, log); > > + /* > + * Verify only the upper limit of the update_index. Each reflog entry > + * is tied to a specific update_index. Entries in the reflog can be > + * replaced by adding a new entry with the same update_index, > + * effectively canceling the old one. > + * > + * Consequently, reflog updates may include update_index values lower > + * than the writer's min_update_index. > + */ > + if (log->update_index > w->max_update_index) > + return REFTABLE_API_ERROR; Yup, looks sensible. > if (!log->refname) > return REFTABLE_API_ERROR; > > diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c > index d279b86df0aeda11b3fb4d2c15803760ae394941..5ad1c72f6901abcfe7fdc6c3e69e26b58d0013a6 100644 > --- a/t/unit-tests/t-reftable-readwrite.c > +++ b/t/unit-tests/t-reftable-readwrite.c > @@ -151,6 +151,45 @@ static void t_log_overflow(void) > reftable_buf_release(&buf); > } > > +static void t_log_write_limits(void) > +{ > + struct reftable_write_options opts = { 0 }; > + struct reftable_buf buf = REFTABLE_BUF_INIT; > + struct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts); > + struct reftable_log_record log = { > + .refname = (char *)"refs/head/master", > + .update_index = 1, > + .value_type = REFTABLE_LOG_UPDATE, > + .value = { > + .update = { > + .old_hash = { 1 }, > + .new_hash = { 2 }, > + .name = (char *)"Han-Wen Nienhuys", > + .email = (char *)"hanwen@google.com", > + .tz_offset = 100, > + .time = 0x5e430672, > + }, > + }, > + }; > + int err; > + > + reftable_writer_set_limits(w, 1, 2); > + > + err = reftable_writer_add_log(w, &log); > + check_int(err, ==, 0); > + > + log.update_index = 2; > + err = reftable_writer_add_log(w, &log); > + check_int(err, ==, 0); > + > + log.update_index = 3; > + err = reftable_writer_add_log(w, &log); > + check_int(err, ==, REFTABLE_API_ERROR); > + > + reftable_writer_free(w); > + reftable_buf_release(&buf); > +} Makes sense, as well. We could trivially extend this test to also assert that we can successfully write a log record with update index 0, which would be smaller than the lower bound. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Thu, Dec 05, 2024 at 04:49:57PM +0100, Karthik Nayak wrote: >> Each reftable addition has an associated update_index. While writing >> refs, the update_index is verified to be within the range of the >> reftable writer, i.e. `writer.min_update_index < ref.update_index` and >> `writer.max_update_index > ref.update_index`. > > These should probably be `<=` and `>=`, respectively. > Indeed, good catch, will fix. >> diff --git a/reftable/writer.c b/reftable/writer.c >> index fd136794d5a27b33b5017f36fbd6b095ab8dac5b..f87086777cd20a9890a63f10c5d6932310dd5610 100644 >> --- a/reftable/writer.c >> +++ b/reftable/writer.c >> @@ -412,6 +412,18 @@ int reftable_writer_add_log(struct reftable_writer *w, >> if (log->value_type == REFTABLE_LOG_DELETION) >> return reftable_writer_add_log_verbatim(w, log); >> >> + /* >> + * Verify only the upper limit of the update_index. Each reflog entry >> + * is tied to a specific update_index. Entries in the reflog can be >> + * replaced by adding a new entry with the same update_index, >> + * effectively canceling the old one. >> + * >> + * Consequently, reflog updates may include update_index values lower >> + * than the writer's min_update_index. >> + */ >> + if (log->update_index > w->max_update_index) >> + return REFTABLE_API_ERROR; > > Yup, looks sensible. > >> if (!log->refname) >> return REFTABLE_API_ERROR; >> >> diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c >> index d279b86df0aeda11b3fb4d2c15803760ae394941..5ad1c72f6901abcfe7fdc6c3e69e26b58d0013a6 100644 >> --- a/t/unit-tests/t-reftable-readwrite.c >> +++ b/t/unit-tests/t-reftable-readwrite.c >> @@ -151,6 +151,45 @@ static void t_log_overflow(void) >> reftable_buf_release(&buf); >> } >> >> +static void t_log_write_limits(void) >> +{ >> + struct reftable_write_options opts = { 0 }; >> + struct reftable_buf buf = REFTABLE_BUF_INIT; >> + struct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts); >> + struct reftable_log_record log = { >> + .refname = (char *)"refs/head/master", >> + .update_index = 1, >> + .value_type = REFTABLE_LOG_UPDATE, >> + .value = { >> + .update = { >> + .old_hash = { 1 }, >> + .new_hash = { 2 }, >> + .name = (char *)"Han-Wen Nienhuys", >> + .email = (char *)"hanwen@google.com", >> + .tz_offset = 100, >> + .time = 0x5e430672, >> + }, >> + }, >> + }; >> + int err; >> + >> + reftable_writer_set_limits(w, 1, 2); >> + >> + err = reftable_writer_add_log(w, &log); >> + check_int(err, ==, 0); >> + >> + log.update_index = 2; >> + err = reftable_writer_add_log(w, &log); >> + check_int(err, ==, 0); >> + >> + log.update_index = 3; >> + err = reftable_writer_add_log(w, &log); >> + check_int(err, ==, REFTABLE_API_ERROR); >> + >> + reftable_writer_free(w); >> + reftable_buf_release(&buf); >> +} > > Makes sense, as well. We could trivially extend this test to also assert > that we can successfully write a log record with update index 0, which > would be smaller than the lower bound. > > Patrick Yeah, that does make sense, will add and send in a new version. Thanks for the quick review! Karthik
diff --git a/reftable/writer.c b/reftable/writer.c index fd136794d5a27b33b5017f36fbd6b095ab8dac5b..f87086777cd20a9890a63f10c5d6932310dd5610 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -412,6 +412,18 @@ int reftable_writer_add_log(struct reftable_writer *w, if (log->value_type == REFTABLE_LOG_DELETION) return reftable_writer_add_log_verbatim(w, log); + /* + * Verify only the upper limit of the update_index. Each reflog entry + * is tied to a specific update_index. Entries in the reflog can be + * replaced by adding a new entry with the same update_index, + * effectively canceling the old one. + * + * Consequently, reflog updates may include update_index values lower + * than the writer's min_update_index. + */ + if (log->update_index > w->max_update_index) + return REFTABLE_API_ERROR; + if (!log->refname) return REFTABLE_API_ERROR; diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c index d279b86df0aeda11b3fb4d2c15803760ae394941..5ad1c72f6901abcfe7fdc6c3e69e26b58d0013a6 100644 --- a/t/unit-tests/t-reftable-readwrite.c +++ b/t/unit-tests/t-reftable-readwrite.c @@ -90,7 +90,7 @@ static void t_log_buffer_size(void) int i; struct reftable_log_record log = { .refname = (char *) "refs/heads/master", - .update_index = 0xa, + .update_index = update_index, .value_type = REFTABLE_LOG_UPDATE, .value = { .update = { .name = (char *) "Han-Wen Nienhuys", @@ -127,7 +127,7 @@ static void t_log_overflow(void) int err; struct reftable_log_record log = { .refname = (char *) "refs/heads/master", - .update_index = 0xa, + .update_index = update_index, .value_type = REFTABLE_LOG_UPDATE, .value = { .update = { @@ -151,6 +151,45 @@ static void t_log_overflow(void) reftable_buf_release(&buf); } +static void t_log_write_limits(void) +{ + struct reftable_write_options opts = { 0 }; + struct reftable_buf buf = REFTABLE_BUF_INIT; + struct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts); + struct reftable_log_record log = { + .refname = (char *)"refs/head/master", + .update_index = 1, + .value_type = REFTABLE_LOG_UPDATE, + .value = { + .update = { + .old_hash = { 1 }, + .new_hash = { 2 }, + .name = (char *)"Han-Wen Nienhuys", + .email = (char *)"hanwen@google.com", + .tz_offset = 100, + .time = 0x5e430672, + }, + }, + }; + int err; + + reftable_writer_set_limits(w, 1, 2); + + err = reftable_writer_add_log(w, &log); + check_int(err, ==, 0); + + log.update_index = 2; + err = reftable_writer_add_log(w, &log); + check_int(err, ==, 0); + + log.update_index = 3; + err = reftable_writer_add_log(w, &log); + check_int(err, ==, REFTABLE_API_ERROR); + + reftable_writer_free(w); + reftable_buf_release(&buf); +} + static void t_log_write_read(void) { struct reftable_write_options opts = { @@ -917,6 +956,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED) TEST(t_corrupt_table_empty(), "read-write on an empty table"); TEST(t_log_buffer_size(), "buffer extension for log compression"); TEST(t_log_overflow(), "log overflow returns expected error"); + TEST(t_log_write_limits(), "writer limits for writing log records"); TEST(t_log_write_read(), "read-write on log records"); TEST(t_log_zlib_corruption(), "reading corrupted log record returns expected error"); TEST(t_table_read_api(), "read on a table"); diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c index 72f6747064f62106e87c9a90e5fe315139d46e60..52b81475c36aa94ff09f3bf77a7d23992957deb4 100644 --- a/t/unit-tests/t-reftable-stack.c +++ b/t/unit-tests/t-reftable-stack.c @@ -770,8 +770,12 @@ static void t_reftable_stack_tombstone(void) } logs[i].refname = xstrdup(buf); - /* update_index is part of the key. */ - logs[i].update_index = 42; + /* + * update_index is part of the key so should be constant. + * The value itself should be less than the writer's upper + * limit. + */ + logs[i].update_index = 1; if (i % 2 == 0) { logs[i].value_type = REFTABLE_LOG_UPDATE; t_reftable_set_hash(logs[i].value.update.new_hash, i,
Each reftable addition has an associated update_index. While writing refs, the update_index is verified to be within the range of the reftable writer, i.e. `writer.min_update_index < ref.update_index` and `writer.max_update_index > ref.update_index`. The corresponding check for reflogs in `reftable_writer_add_log` is however missing. Add a similar check, but only check for the upper limit. This is because reflogs are treated a bit differently than refs. Each reflog entry in reftable has an associated update_index and we also allow expiring entries in the middle, which is done by simply writing a new reflog entry with the same update_index. This means, writing reflog entries with update_index lesser than the writer's update_index is an expected scenario. Add a new unit test to check for the limits and fix some of the existing tests, which were setting arbitrary values for the update_index by ensuring they stay within the now checked limits. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- reftable/writer.c | 12 ++++++++++ t/unit-tests/t-reftable-readwrite.c | 44 +++++++++++++++++++++++++++++++++++-- t/unit-tests/t-reftable-stack.c | 8 +++++-- 3 files changed, 60 insertions(+), 4 deletions(-) --- --- base-commit: cc01bad4a9f566cf4453c7edd6b433851b0835e2 change-id: 20241120-424-reftable-writer-add-check-for-limits-01b1fb703528 Thanks - Karthik