git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2] reftable/writer: ensure valid range for log's update_index

From
Karthik Nayak <karthik.188@gmail.com>
Date
Dec 6, 2024, 13:13 UTC
Message-ID
<20241206-424-reftable-writer-add-check-for-limits-v2-1-82ca350b10be@gmail.com>
In-Reply-To
<20241205-424-reftable-writer-add-check-for-limits-v1-1-b287b055204e@gmail.com>

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>
---
Changes in v2:
- Update the commit message to have an inclusive rannge.
- Update the unit test to also include an update with index below range.
- Link to v1: https://lore.kernel.org/r/20241205-424-reftable-writer-add-check-for-limits-v1-1-b287b055204e@gmail.com
---
 reftable/writer.c                   | 12 ++++++++++
 t/unit-tests/t-reftable-readwrite.c | 47 +++++++++++++++++++++++++++++++++++--
 t/unit-tests/t-reftable-stack.c     |  8 +++++--
 3 files changed, 63 insertions(+), 4 deletions(-)
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..7ffbd78c6759fc85ab9d5a2909d4d20662fc56c7 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,48 @@ 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 = 0,
+		.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, 1);
+
+	/* write with update_index (0) below set limits (1, 1) */
+	err = reftable_writer_add_log(w, &log);
+	check_int(err, ==, 0);
+
+	/* write with update_index (1) in the set limits (1, 1) */
+	log.update_index = 1;
+	err = reftable_writer_add_log(w, &log);
+	check_int(err, ==, 0);
+
+	/* write with update_index (3) above set limits (1, 1) */
+	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 +959,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,

---

Range-diff versus v1:

1:  e30bdaeeee ! 1:  1c3cb5e92b reftable/writer: ensure valid range for log's update_index
    @@ Commit message
     
         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`.
    +    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
    @@ t/unit-tests/t-reftable-readwrite.c: static void t_log_overflow(void)
     +	struct reftable_writer *w = t_reftable_strbuf_writer(&buf, &opts);
     +	struct reftable_log_record log = {
     +		.refname = (char *)"refs/head/master",
    -+		.update_index = 1,
    ++		.update_index = 0,
     +		.value_type = REFTABLE_LOG_UPDATE,
     +		.value = {
     +			.update = {
    @@ t/unit-tests/t-reftable-readwrite.c: static void t_log_overflow(void)
     +	};
     +	int err;
     +
    -+	reftable_writer_set_limits(w, 1, 2);
    ++	reftable_writer_set_limits(w, 1, 1);
     +
    ++	/* write with update_index (0) below set limits (1, 1) */
     +	err = reftable_writer_add_log(w, &log);
     +	check_int(err, ==, 0);
     +
    -+	log.update_index = 2;
    ++	/* write with update_index (1) in the set limits (1, 1) */
    ++	log.update_index = 1;
     +	err = reftable_writer_add_log(w, &log);
     +	check_int(err, ==, 0);
     +
    ++	/* write with update_index (3) above set limits (1, 1) */
     +	log.update_index = 3;
     +	err = reftable_writer_add_log(w, &log);
     +	check_int(err, ==, REFTABLE_API_ERROR);


--- 

base-commit: cc01bad4a9f566cf4453c7edd6b433851b0835e2
change-id: 20241120-424-reftable-writer-add-check-for-limits-01b1fb703528

Thanks
- Karthik
Previous: karthik nayakNext: Junio C Hamano
Message 4 of 7 in “reftable/writer: ensure valid range for log's update_index”
  1. reftable/writer: ensure valid range for log's update_indexKarthik Nayak, Dec 5, 2024
  2. Patrick SteinhardtDec 6, 2024
  3. karthik nayakDec 6, 2024
  4. reftable/writer: ensure valid range for log's update_indexKarthik Nayak, Dec 6, 2024
  5. Junio C HamanoDec 6, 2024
  6. karthik nayakDec 7, 2024
  7. Toon ClaesDec 11, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.