{"thread":{"id":"62602","subject":"[PATCH] reftable/writer: ensure valid range for log's update_index","startedAt":"2024-12-05T15:50:02Z","lastAt":"2024-12-11T13:11:22Z","messageCount":7,"participants":["Karthik Nayak","Patrick Steinhardt","karthik nayak","Junio C Hamano","Toon Claes"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"508672","messageId":"20241205-424-reftable-writer-add-check-for-limits-v1-1-b287b055204e@gmail.com","threadId":"62602","inReplyTo":null,"subject":"[PATCH] reftable/writer: ensure valid range for log's update_index","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-05T15:49:57Z","receivedAt":"2024-12-05T15:50:02Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Each reftable addition has an associated update_index. While writing\nrefs, the update_index is verified to be within the range of the\nreftable writer, i.e. `writer.min_update_index < ref.update_index` and\n`writer.max_update_index > ref.update_index`.\n\nThe corresponding check for reflogs in `reftable_writer_add_log` is\nhowever missing. Add a similar check, but only check for the upper\nlimit. This is because reflogs are treated a bit differently than refs.\nEach reflog entry in reftable has an associated update_index and we also\nallow expiring entries in the middle, which is done by simply writing a\nnew reflog entry with the same update_index. This means, writing reflog\nentries with update_index lesser than the writer's update_index is an\nexpected scenario.\n\nAdd a new unit test to check for the limits and fix some of the existing\ntests, which were setting arbitrary values for the update_index by\nensuring they stay within the now checked limits.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n reftable/writer.c                   | 12 ++++++++++\n t/unit-tests/t-reftable-readwrite.c | 44 +++++++++++++++++++++++++++++++++++--\n t/unit-tests/t-reftable-stack.c     |  8 +++++--\n 3 files changed, 60 insertions(+), 4 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex fd136794d5a27b33b5017f36fbd6b095ab8dac5b..f87086777cd20a9890a63f10c5d6932310dd5610 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -412,6 +412,18 @@ int reftable_writer_add_log(struct reftable_writer *w,\n \tif (log->value_type == REFTABLE_LOG_DELETION)\n \t\treturn reftable_writer_add_log_verbatim(w, log);\n \n+\t/*\n+\t * Verify only the upper limit of the update_index. Each reflog entry\n+\t * is tied to a specific update_index. Entries in the reflog can be\n+\t * replaced by adding a new entry with the same update_index,\n+\t * effectively canceling the old one.\n+\t *\n+\t * Consequently, reflog updates may include update_index values lower\n+\t * than the writer's min_update_index.\n+\t */\n+\tif (log->update_index > w->max_update_index)\n+\t\treturn REFTABLE_API_ERROR;\n+\n \tif (!log->refname)\n \t\treturn REFTABLE_API_ERROR;\n \ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex d279b86df0aeda11b3fb4d2c15803760ae394941..5ad1c72f6901abcfe7fdc6c3e69e26b58d0013a6 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -90,7 +90,7 @@ static void t_log_buffer_size(void)\n \tint i;\n \tstruct reftable_log_record\n \t\tlog = { .refname = (char *) \"refs/heads/master\",\n-\t\t\t.update_index = 0xa,\n+\t\t\t.update_index = update_index,\n \t\t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t\t.value = { .update = {\n \t\t\t\t\t   .name = (char *) \"Han-Wen Nienhuys\",\n@@ -127,7 +127,7 @@ static void t_log_overflow(void)\n \tint err;\n \tstruct reftable_log_record log = {\n \t\t.refname = (char *) \"refs/heads/master\",\n-\t\t.update_index = 0xa,\n+\t\t.update_index = update_index,\n \t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t.value = {\n \t\t\t.update = {\n@@ -151,6 +151,45 @@ static void t_log_overflow(void)\n \treftable_buf_release(&buf);\n }\n \n+static void t_log_write_limits(void)\n+{\n+\tstruct reftable_write_options opts = { 0 };\n+\tstruct reftable_buf buf = REFTABLE_BUF_INIT;\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n+\tstruct reftable_log_record log = {\n+\t\t.refname = (char *)\"refs/head/master\",\n+\t\t.update_index = 1,\n+\t\t.value_type = REFTABLE_LOG_UPDATE,\n+\t\t.value = {\n+\t\t\t.update = {\n+\t\t\t\t.old_hash = { 1 },\n+\t\t\t\t.new_hash = { 2 },\n+\t\t\t\t.name = (char *)\"Han-Wen Nienhuys\",\n+\t\t\t\t.email = (char *)\"hanwen@google.com\",\n+\t\t\t\t.tz_offset = 100,\n+\t\t\t\t.time = 0x5e430672,\n+\t\t\t},\n+\t\t},\n+\t};\n+\tint err;\n+\n+\treftable_writer_set_limits(w, 1, 2);\n+\n+\terr = reftable_writer_add_log(w, &log);\n+\tcheck_int(err, ==, 0);\n+\n+\tlog.update_index = 2;\n+\terr = reftable_writer_add_log(w, &log);\n+\tcheck_int(err, ==, 0);\n+\n+\tlog.update_index = 3;\n+\terr = reftable_writer_add_log(w, &log);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n+\n+\treftable_writer_free(w);\n+\treftable_buf_release(&buf);\n+}\n+\n static void t_log_write_read(void)\n {\n \tstruct reftable_write_options opts = {\n@@ -917,6 +956,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \tTEST(t_corrupt_table_empty(), \"read-write on an empty table\");\n \tTEST(t_log_buffer_size(), \"buffer extension for log compression\");\n \tTEST(t_log_overflow(), \"log overflow returns expected error\");\n+\tTEST(t_log_write_limits(), \"writer limits for writing log records\");\n \tTEST(t_log_write_read(), \"read-write on log records\");\n \tTEST(t_log_zlib_corruption(), \"reading corrupted log record returns expected error\");\n \tTEST(t_table_read_api(), \"read on a table\");\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex 72f6747064f62106e87c9a90e5fe315139d46e60..52b81475c36aa94ff09f3bf77a7d23992957deb4 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -770,8 +770,12 @@ static void t_reftable_stack_tombstone(void)\n \t\t}\n \n \t\tlogs[i].refname = xstrdup(buf);\n-\t\t/* update_index is part of the key. */\n-\t\tlogs[i].update_index = 42;\n+\t\t/*\n+\t\t * update_index is part of the key so should be constant.\n+\t\t * The value itself should be less than the writer's upper\n+\t\t * limit.\n+\t\t */\n+\t\tlogs[i].update_index = 1;\n \t\tif (i % 2 == 0) {\n \t\t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n \t\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i,\n\n---\n\n\n\n--- \n\nbase-commit: cc01bad4a9f566cf4453c7edd6b433851b0835e2\nchange-id: 20241120-424-reftable-writer-add-check-for-limits-01b1fb703528\n\nThanks\n- Karthik\n\n"},{"id":"508715","messageId":"Z1K-rXakmMQHN9If@pks.im","threadId":"62602","inReplyTo":"20241205-424-reftable-writer-add-check-for-limits-v1-1-b287b055204e@gmail.com","subject":"Re: [PATCH] reftable/writer: ensure valid range for log's update_index","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-06T09:06:53Z","receivedAt":"2024-12-06T09:07:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 05, 2024 at 04:49:57PM +0100, Karthik Nayak wrote:\n> Each reftable addition has an associated update_index. While writing\n> refs, the update_index is verified to be within the range of the\n> reftable writer, i.e. `writer.min_update_index < ref.update_index` and\n> `writer.max_update_index > ref.update_index`.\n\nThese should probably be `<=` and `>=`, respectively.\n\n> diff --git a/reftable/writer.c b/reftable/writer.c\n> index fd136794d5a27b33b5017f36fbd6b095ab8dac5b..f87086777cd20a9890a63f10c5d6932310dd5610 100644\n> --- a/reftable/writer.c\n> +++ b/reftable/writer.c\n> @@ -412,6 +412,18 @@ int reftable_writer_add_log(struct reftable_writer *w,\n>  \tif (log->value_type == REFTABLE_LOG_DELETION)\n>  \t\treturn reftable_writer_add_log_verbatim(w, log);\n>  \n> +\t/*\n> +\t * Verify only the upper limit of the update_index. Each reflog entry\n> +\t * is tied to a specific update_index. Entries in the reflog can be\n> +\t * replaced by adding a new entry with the same update_index,\n> +\t * effectively canceling the old one.\n> +\t *\n> +\t * Consequently, reflog updates may include update_index values lower\n> +\t * than the writer's min_update_index.\n> +\t */\n> +\tif (log->update_index > w->max_update_index)\n> +\t\treturn REFTABLE_API_ERROR;\n\nYup, looks sensible.\n\n>  \tif (!log->refname)\n>  \t\treturn REFTABLE_API_ERROR;\n>  \n> diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\n> index d279b86df0aeda11b3fb4d2c15803760ae394941..5ad1c72f6901abcfe7fdc6c3e69e26b58d0013a6 100644\n> --- a/t/unit-tests/t-reftable-readwrite.c\n> +++ b/t/unit-tests/t-reftable-readwrite.c\n> @@ -151,6 +151,45 @@ static void t_log_overflow(void)\n>  \treftable_buf_release(&buf);\n>  }\n>  \n> +static void t_log_write_limits(void)\n> +{\n> +\tstruct reftable_write_options opts = { 0 };\n> +\tstruct reftable_buf buf = REFTABLE_BUF_INIT;\n> +\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n> +\tstruct reftable_log_record log = {\n> +\t\t.refname = (char *)\"refs/head/master\",\n> +\t\t.update_index = 1,\n> +\t\t.value_type = REFTABLE_LOG_UPDATE,\n> +\t\t.value = {\n> +\t\t\t.update = {\n> +\t\t\t\t.old_hash = { 1 },\n> +\t\t\t\t.new_hash = { 2 },\n> +\t\t\t\t.name = (char *)\"Han-Wen Nienhuys\",\n> +\t\t\t\t.email = (char *)\"hanwen@google.com\",\n> +\t\t\t\t.tz_offset = 100,\n> +\t\t\t\t.time = 0x5e430672,\n> +\t\t\t},\n> +\t\t},\n> +\t};\n> +\tint err;\n> +\n> +\treftable_writer_set_limits(w, 1, 2);\n> +\n> +\terr = reftable_writer_add_log(w, &log);\n> +\tcheck_int(err, ==, 0);\n> +\n> +\tlog.update_index = 2;\n> +\terr = reftable_writer_add_log(w, &log);\n> +\tcheck_int(err, ==, 0);\n> +\n> +\tlog.update_index = 3;\n> +\terr = reftable_writer_add_log(w, &log);\n> +\tcheck_int(err, ==, REFTABLE_API_ERROR);\n> +\n> +\treftable_writer_free(w);\n> +\treftable_buf_release(&buf);\n> +}\n\nMakes sense, as well. We could trivially extend this test to also assert\nthat we can successfully write a log record with update index 0, which\nwould be smaller than the lower bound.\n\nPatrick\n"},{"id":"508742","messageId":"CAOLa=ZRVy46gZP3JK5sUHaJVCCXacqbT+aW_Yr0WdboRVks9dw@mail.gmail.com","threadId":"62602","inReplyTo":"Z1K-rXakmMQHN9If@pks.im","subject":"Re: [PATCH] reftable/writer: ensure valid range for log's update_index","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-06T11:24:28Z","receivedAt":"2024-12-06T11:24:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Thu, Dec 05, 2024 at 04:49:57PM +0100, Karthik Nayak wrote:\n>> Each reftable addition has an associated update_index. While writing\n>> refs, the update_index is verified to be within the range of the\n>> reftable writer, i.e. `writer.min_update_index < ref.update_index` and\n>> `writer.max_update_index > ref.update_index`.\n>\n> These should probably be `<=` and `>=`, respectively.\n>\n\nIndeed, good catch, will fix.\n\n>> diff --git a/reftable/writer.c b/reftable/writer.c\n>> index fd136794d5a27b33b5017f36fbd6b095ab8dac5b..f87086777cd20a9890a63f10c5d6932310dd5610 100644\n>> --- a/reftable/writer.c\n>> +++ b/reftable/writer.c\n>> @@ -412,6 +412,18 @@ int reftable_writer_add_log(struct reftable_writer *w,\n>>  \tif (log->value_type == REFTABLE_LOG_DELETION)\n>>  \t\treturn reftable_writer_add_log_verbatim(w, log);\n>>\n>> +\t/*\n>> +\t * Verify only the upper limit of the update_index. Each reflog entry\n>> +\t * is tied to a specific update_index. Entries in the reflog can be\n>> +\t * replaced by adding a new entry with the same update_index,\n>> +\t * effectively canceling the old one.\n>> +\t *\n>> +\t * Consequently, reflog updates may include update_index values lower\n>> +\t * than the writer's min_update_index.\n>> +\t */\n>> +\tif (log->update_index > w->max_update_index)\n>> +\t\treturn REFTABLE_API_ERROR;\n>\n> Yup, looks sensible.\n>\n>>  \tif (!log->refname)\n>>  \t\treturn REFTABLE_API_ERROR;\n>>\n>> diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\n>> index d279b86df0aeda11b3fb4d2c15803760ae394941..5ad1c72f6901abcfe7fdc6c3e69e26b58d0013a6 100644\n>> --- a/t/unit-tests/t-reftable-readwrite.c\n>> +++ b/t/unit-tests/t-reftable-readwrite.c\n>> @@ -151,6 +151,45 @@ static void t_log_overflow(void)\n>>  \treftable_buf_release(&buf);\n>>  }\n>>\n>> +static void t_log_write_limits(void)\n>> +{\n>> +\tstruct reftable_write_options opts = { 0 };\n>> +\tstruct reftable_buf buf = REFTABLE_BUF_INIT;\n>> +\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n>> +\tstruct reftable_log_record log = {\n>> +\t\t.refname = (char *)\"refs/head/master\",\n>> +\t\t.update_index = 1,\n>> +\t\t.value_type = REFTABLE_LOG_UPDATE,\n>> +\t\t.value = {\n>> +\t\t\t.update = {\n>> +\t\t\t\t.old_hash = { 1 },\n>> +\t\t\t\t.new_hash = { 2 },\n>> +\t\t\t\t.name = (char *)\"Han-Wen Nienhuys\",\n>> +\t\t\t\t.email = (char *)\"hanwen@google.com\",\n>> +\t\t\t\t.tz_offset = 100,\n>> +\t\t\t\t.time = 0x5e430672,\n>> +\t\t\t},\n>> +\t\t},\n>> +\t};\n>> +\tint err;\n>> +\n>> +\treftable_writer_set_limits(w, 1, 2);\n>> +\n>> +\terr = reftable_writer_add_log(w, &log);\n>> +\tcheck_int(err, ==, 0);\n>> +\n>> +\tlog.update_index = 2;\n>> +\terr = reftable_writer_add_log(w, &log);\n>> +\tcheck_int(err, ==, 0);\n>> +\n>> +\tlog.update_index = 3;\n>> +\terr = reftable_writer_add_log(w, &log);\n>> +\tcheck_int(err, ==, REFTABLE_API_ERROR);\n>> +\n>> +\treftable_writer_free(w);\n>> +\treftable_buf_release(&buf);\n>> +}\n>\n> Makes sense, as well. We could trivially extend this test to also assert\n> that we can successfully write a log record with update index 0, which\n> would be smaller than the lower bound.\n>\n> Patrick\n\nYeah, that does make sense, will add and send in a new version. Thanks\nfor the quick review!\n\nKarthik\n"},{"id":"508763","messageId":"20241206-424-reftable-writer-add-check-for-limits-v2-1-82ca350b10be@gmail.com","threadId":"62602","inReplyTo":"20241205-424-reftable-writer-add-check-for-limits-v1-1-b287b055204e@gmail.com","subject":"[PATCH v2] reftable/writer: ensure valid range for log's update_index","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-06T13:13:19Z","receivedAt":"2024-12-06T13:13:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Each reftable addition has an associated update_index. While writing\nrefs, the update_index is verified to be within the range of the\nreftable writer, i.e. `writer.min_update_index <= ref.update_index` and\n`writer.max_update_index => ref.update_index`.\n\nThe corresponding check for reflogs in `reftable_writer_add_log` is\nhowever missing. Add a similar check, but only check for the upper\nlimit. This is because reflogs are treated a bit differently than refs.\nEach reflog entry in reftable has an associated update_index and we also\nallow expiring entries in the middle, which is done by simply writing a\nnew reflog entry with the same update_index. This means, writing reflog\nentries with update_index lesser than the writer's update_index is an\nexpected scenario.\n\nAdd a new unit test to check for the limits and fix some of the existing\ntests, which were setting arbitrary values for the update_index by\nensuring they stay within the now checked limits.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\nChanges in v2:\n- Update the commit message to have an inclusive rannge.\n- Update the unit test to also include an update with index below range.\n- Link to v1: https://lore.kernel.org/r/20241205-424-reftable-writer-add-check-for-limits-v1-1-b287b055204e@gmail.com\n---\n reftable/writer.c                   | 12 ++++++++++\n t/unit-tests/t-reftable-readwrite.c | 47 +++++++++++++++++++++++++++++++++++--\n t/unit-tests/t-reftable-stack.c     |  8 +++++--\n 3 files changed, 63 insertions(+), 4 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex fd136794d5a27b33b5017f36fbd6b095ab8dac5b..f87086777cd20a9890a63f10c5d6932310dd5610 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -412,6 +412,18 @@ int reftable_writer_add_log(struct reftable_writer *w,\n \tif (log->value_type == REFTABLE_LOG_DELETION)\n \t\treturn reftable_writer_add_log_verbatim(w, log);\n \n+\t/*\n+\t * Verify only the upper limit of the update_index. Each reflog entry\n+\t * is tied to a specific update_index. Entries in the reflog can be\n+\t * replaced by adding a new entry with the same update_index,\n+\t * effectively canceling the old one.\n+\t *\n+\t * Consequently, reflog updates may include update_index values lower\n+\t * than the writer's min_update_index.\n+\t */\n+\tif (log->update_index > w->max_update_index)\n+\t\treturn REFTABLE_API_ERROR;\n+\n \tif (!log->refname)\n \t\treturn REFTABLE_API_ERROR;\n \ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex d279b86df0aeda11b3fb4d2c15803760ae394941..7ffbd78c6759fc85ab9d5a2909d4d20662fc56c7 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -90,7 +90,7 @@ static void t_log_buffer_size(void)\n \tint i;\n \tstruct reftable_log_record\n \t\tlog = { .refname = (char *) \"refs/heads/master\",\n-\t\t\t.update_index = 0xa,\n+\t\t\t.update_index = update_index,\n \t\t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t\t.value = { .update = {\n \t\t\t\t\t   .name = (char *) \"Han-Wen Nienhuys\",\n@@ -127,7 +127,7 @@ static void t_log_overflow(void)\n \tint err;\n \tstruct reftable_log_record log = {\n \t\t.refname = (char *) \"refs/heads/master\",\n-\t\t.update_index = 0xa,\n+\t\t.update_index = update_index,\n \t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t.value = {\n \t\t\t.update = {\n@@ -151,6 +151,48 @@ static void t_log_overflow(void)\n \treftable_buf_release(&buf);\n }\n \n+static void t_log_write_limits(void)\n+{\n+\tstruct reftable_write_options opts = { 0 };\n+\tstruct reftable_buf buf = REFTABLE_BUF_INIT;\n+\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n+\tstruct reftable_log_record log = {\n+\t\t.refname = (char *)\"refs/head/master\",\n+\t\t.update_index = 0,\n+\t\t.value_type = REFTABLE_LOG_UPDATE,\n+\t\t.value = {\n+\t\t\t.update = {\n+\t\t\t\t.old_hash = { 1 },\n+\t\t\t\t.new_hash = { 2 },\n+\t\t\t\t.name = (char *)\"Han-Wen Nienhuys\",\n+\t\t\t\t.email = (char *)\"hanwen@google.com\",\n+\t\t\t\t.tz_offset = 100,\n+\t\t\t\t.time = 0x5e430672,\n+\t\t\t},\n+\t\t},\n+\t};\n+\tint err;\n+\n+\treftable_writer_set_limits(w, 1, 1);\n+\n+\t/* write with update_index (0) below set limits (1, 1) */\n+\terr = reftable_writer_add_log(w, &log);\n+\tcheck_int(err, ==, 0);\n+\n+\t/* write with update_index (1) in the set limits (1, 1) */\n+\tlog.update_index = 1;\n+\terr = reftable_writer_add_log(w, &log);\n+\tcheck_int(err, ==, 0);\n+\n+\t/* write with update_index (3) above set limits (1, 1) */\n+\tlog.update_index = 3;\n+\terr = reftable_writer_add_log(w, &log);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n+\n+\treftable_writer_free(w);\n+\treftable_buf_release(&buf);\n+}\n+\n static void t_log_write_read(void)\n {\n \tstruct reftable_write_options opts = {\n@@ -917,6 +959,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \tTEST(t_corrupt_table_empty(), \"read-write on an empty table\");\n \tTEST(t_log_buffer_size(), \"buffer extension for log compression\");\n \tTEST(t_log_overflow(), \"log overflow returns expected error\");\n+\tTEST(t_log_write_limits(), \"writer limits for writing log records\");\n \tTEST(t_log_write_read(), \"read-write on log records\");\n \tTEST(t_log_zlib_corruption(), \"reading corrupted log record returns expected error\");\n \tTEST(t_table_read_api(), \"read on a table\");\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex 72f6747064f62106e87c9a90e5fe315139d46e60..52b81475c36aa94ff09f3bf77a7d23992957deb4 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -770,8 +770,12 @@ static void t_reftable_stack_tombstone(void)\n \t\t}\n \n \t\tlogs[i].refname = xstrdup(buf);\n-\t\t/* update_index is part of the key. */\n-\t\tlogs[i].update_index = 42;\n+\t\t/*\n+\t\t * update_index is part of the key so should be constant.\n+\t\t * The value itself should be less than the writer's upper\n+\t\t * limit.\n+\t\t */\n+\t\tlogs[i].update_index = 1;\n \t\tif (i % 2 == 0) {\n \t\t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n \t\t\tt_reftable_set_hash(logs[i].value.update.new_hash, i,\n\n---\n\nRange-diff versus v1:\n\n1:  e30bdaeeee ! 1:  1c3cb5e92b reftable/writer: ensure valid range for log's update_index\n    @@ Commit message\n     \n         Each reftable addition has an associated update_index. While writing\n         refs, the update_index is verified to be within the range of the\n    -    reftable writer, i.e. `writer.min_update_index < ref.update_index` and\n    -    `writer.max_update_index > ref.update_index`.\n    +    reftable writer, i.e. `writer.min_update_index <= ref.update_index` and\n    +    `writer.max_update_index => ref.update_index`.\n     \n         The corresponding check for reflogs in `reftable_writer_add_log` is\n         however missing. Add a similar check, but only check for the upper\n    @@ t/unit-tests/t-reftable-readwrite.c: static void t_log_overflow(void)\n     +\tstruct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);\n     +\tstruct reftable_log_record log = {\n     +\t\t.refname = (char *)\"refs/head/master\",\n    -+\t\t.update_index = 1,\n    ++\t\t.update_index = 0,\n     +\t\t.value_type = REFTABLE_LOG_UPDATE,\n     +\t\t.value = {\n     +\t\t\t.update = {\n    @@ t/unit-tests/t-reftable-readwrite.c: static void t_log_overflow(void)\n     +\t};\n     +\tint err;\n     +\n    -+\treftable_writer_set_limits(w, 1, 2);\n    ++\treftable_writer_set_limits(w, 1, 1);\n     +\n    ++\t/* write with update_index (0) below set limits (1, 1) */\n     +\terr = reftable_writer_add_log(w, &log);\n     +\tcheck_int(err, ==, 0);\n     +\n    -+\tlog.update_index = 2;\n    ++\t/* write with update_index (1) in the set limits (1, 1) */\n    ++\tlog.update_index = 1;\n     +\terr = reftable_writer_add_log(w, &log);\n     +\tcheck_int(err, ==, 0);\n     +\n    ++\t/* write with update_index (3) above set limits (1, 1) */\n     +\tlog.update_index = 3;\n     +\terr = reftable_writer_add_log(w, &log);\n     +\tcheck_int(err, ==, REFTABLE_API_ERROR);\n\n\n--- \n\nbase-commit: cc01bad4a9f566cf4453c7edd6b433851b0835e2\nchange-id: 20241120-424-reftable-writer-add-check-for-limits-01b1fb703528\n\nThanks\n- Karthik\n\n"},{"id":"508811","messageId":"xmqqwmgcv23b.fsf@gitster.g","threadId":"62602","inReplyTo":"20241206-424-reftable-writer-add-check-for-limits-v2-1-82ca350b10be@gmail.com","subject":"Re: [PATCH v2] reftable/writer: ensure valid range for log's update_index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-06T23:11:36Z","receivedAt":"2024-12-06T23:11:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> The corresponding check for reflogs in `reftable_writer_add_log` is\n> however missing. Add a similar check, but only check for the upper\n> limit. This is because reflogs are treated a bit differently than refs.\n> Each reflog entry in reftable has an associated update_index and we also\n> allow expiring entries in the middle, which is done by simply writing a\n> new reflog entry with the same update_index. This means, writing reflog\n> entries with update_index lesser than the writer's update_index is an\n> expected scenario.\n>\n> Add a new unit test to check for the limits and fix some of the existing\n> tests, which were setting arbitrary values for the update_index by\n> ensuring they stay within the now checked limits.\n\nInteresting.  I am a little curious how this was found.  As long as\nthe other parts of the system (i.e. the callers) are not buggy, the\nupdate pointer will stay within the range, and that is why I do not\nthink I can write an end-to-end test to trigger an existing bug that\nwould be caught by this \"fix\".  Fixing the existing unit tests that\nfeed a wrong input and expect some right outcome is good, but would\nit be a good to also have a new unit test that validates that such\nan incorrect precondition for calling is caught and the caller gets\nthe expected REFTABLE_API_ERROR as a result, I wonder?  Being able\nto trigger a condition that is harder to do with the end-to-end test\nis one good thing about having unit test framework, no?\n\nWill queue.  Thanks.\n\n"},{"id":"508818","messageId":"CAOLa=ZRYDqhAPMiuAS-HGaC1huiUGgLW3hL_dDNwrFA5c8ewEQ@mail.gmail.com","threadId":"62602","inReplyTo":"xmqqwmgcv23b.fsf@gitster.g","subject":"Re: [PATCH v2] reftable/writer: ensure valid range for log's update_index","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-07T09:54:30Z","receivedAt":"2024-12-07T09:54:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> The corresponding check for reflogs in `reftable_writer_add_log` is\n>> however missing. Add a similar check, but only check for the upper\n>> limit. This is because reflogs are treated a bit differently than refs.\n>> Each reflog entry in reftable has an associated update_index and we also\n>> allow expiring entries in the middle, which is done by simply writing a\n>> new reflog entry with the same update_index. This means, writing reflog\n>> entries with update_index lesser than the writer's update_index is an\n>> expected scenario.\n>>\n>> Add a new unit test to check for the limits and fix some of the existing\n>> tests, which were setting arbitrary values for the update_index by\n>> ensuring they stay within the now checked limits.\n>\n> Interesting.  I am a little curious how this was found.  As long as\n> the other parts of the system (i.e. the callers) are not buggy, the\n> update pointer will stay within the range, and that is why I do not\n> think I can write an end-to-end test to trigger an existing bug that\n> would be caught by this \"fix\".\n\nI was reading the reftable code and noticed it. So mostly luck. Agreed\nwith what you're saying, I'd say this is mostly a 'safeguard'.\n\n> Fixing the existing unit tests that\n> feed a wrong input and expect some right outcome is good, but would\n> it be a good to also have a new unit test that validates that such\n> an incorrect precondition for calling is caught and the caller gets\n> the expected REFTABLE_API_ERROR as a result, I wonder?\n\nI'm confused, I did add a _new_ test in this patch to do exactly what\nyou're suggesting.\n\n> Being able\n> to trigger a condition that is harder to do with the end-to-end test\n> is one good thing about having unit test framework, no?\n>\n\nTotally, these kind of specific changes are perfect for unit-tests. Plus\nit was very simple to add one too.\n\n> Will queue.  Thanks.\n\nThanks!\n\nKarthik\n"},{"id":"508984","messageId":"871pyejrfi.fsf@iotcl.com","threadId":"62602","inReplyTo":"CAOLa=ZRYDqhAPMiuAS-HGaC1huiUGgLW3hL_dDNwrFA5c8ewEQ@mail.gmail.com","subject":"Re: [PATCH v2] reftable/writer: ensure valid range for log's update_index","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-12-11T13:10:57Z","receivedAt":"2024-12-11T13:11:22Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"karthik nayak <karthik.188@gmail.com> writes:\n\n> I was reading the reftable code and noticed it. So mostly luck. Agreed\n> with what you're saying, I'd say this is mostly a 'safeguard'.\n\nI was wondering as well, thanks for that info.\n\n> Totally, these kind of specific changes are perfect for unit-tests. Plus\n> it was very simple to add one too.\n\nI appreciate you're doing this, that makes it easier to reason about\nthese changes.\n\n>> Will queue.  Thanks.\n\nJunio, thank you for the quick turnarond on this patch. For what it's\nworth, I've given this a review and I approve these changes.\n\n-- \nToon\n\n"}]}