reftable: avoid writing empty keys at the block layer

The public interface (reftable_writer) already ensures that keys are
written in strictly increasing order, and an empty key by definition
fails this check.

However, by also enforcing this at the block layer, it is easier to
verify that records (which are written into blocks) never have to
consider the possibility of empty keys.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/reftable/block.c b/reftable/block.c
index 2170748c5e9bb413f6a8ed2cef130686f47ecb58..34d4d073692f9e913d84715d987c8e1c35a7fe8a 100644 (file)
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -88,8 +88,9 @@ uint8_t block_writer_type(struct block_writer *bw)
        return bw->buf[bw->header_off];
 }
 
-/* adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
-   success */
+/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
+   success. Returns REFTABLE_API_ERROR if attempting to write a record with
+   empty key. */
 int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 {
        struct strbuf empty = STRBUF_INIT;
@@ -105,8 +106,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
        int is_restart = 0;
        struct strbuf key = STRBUF_INIT;
        int n = 0;
+       int err = -1;
 
        reftable_record_key(rec, &key);
+       if (!key.len) {
+               err = REFTABLE_API_ERROR;
+               goto done;
+       }
+
        n = reftable_encode_key(&is_restart, out, last, key,
                                reftable_record_val_type(rec));
        if (n < 0)
@@ -118,16 +125,11 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
                goto done;
        string_view_consume(&out, n);
 
-       if (block_writer_register_restart(w, start.len - out.len, is_restart,
-                                         &key) < 0)
-               goto done;
-
-       strbuf_release(&key);
-       return 0;
-
+       err = block_writer_register_restart(w, start.len - out.len, is_restart,
+                                           &key);
 done:
        strbuf_release(&key);
-       return -1;
+       return err;
 }
 
 int block_writer_finish(struct block_writer *w)
@@ -332,6 +334,9 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)
        if (n < 0)
                return -1;
 
+       if (!key.len)
+               return REFTABLE_FORMAT_ERROR;
+
        string_view_consume(&in, n);
        n = reftable_record_decode(rec, key, extra, in, it->br->hash_size);
        if (n < 0)
@@ -358,6 +363,8 @@ int block_reader_first_key(struct block_reader *br, struct strbuf *key)
        int n = reftable_decode_key(key, &extra, empty, in);
        if (n < 0)
                return n;
+       if (!key->len)
+               return REFTABLE_FORMAT_ERROR;
 
        return 0;
 }

diff --git a/reftable/block_test.c b/reftable/block_test.c
index fa2ee092ec0bf52546cfbf5f4d3158656e553629..cb88af4a5639258945f569ede758e31809b62ead 100644 (file)
--- a/reftable/block_test.c
+++ b/reftable/block_test.c
@@ -42,6 +42,11 @@ static void test_block_read_write(void)
        block_writer_init(&bw, BLOCK_TYPE_REF, block.data, block_size,
                          header_off, hash_size(GIT_SHA1_FORMAT_ID));
 
+       rec.u.ref.refname = "";
+       rec.u.ref.value_type = REFTABLE_REF_DELETION;
+       n = block_writer_add(&bw, &rec);
+       EXPECT(n == REFTABLE_API_ERROR);
+
        for (i = 0; i < N; i++) {
                char name[100];
                uint8_t hash[GIT_SHA1_RAWSZ];

diff --git a/reftable/writer.c b/reftable/writer.c
index 944c2329ab568aa17cb1243507cc7a352faaf8ad..d54215a50dc2fec6168bf3e93478e5453f2be25f 100644 (file)
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -240,14 +240,13 @@ static int writer_add_record(struct reftable_writer *w,
 
        writer_reinit_block_writer(w, reftable_record_type(rec));
        err = block_writer_add(w->block_writer, rec);
-       if (err < 0) {
+       if (err == -1) {
                /* we are writing into memory, so an error can only mean it
                 * doesn't fit. */
                err = REFTABLE_ENTRY_TOO_BIG_ERROR;
                goto done;
        }
 
-       err = 0;
 done:
        strbuf_release(&key);
        return err;