Fix transaction safety and reader regressions in Go and C

api.go

@@ -37,14 +37,17 @@ var SHA1ID = HashID([4]byte{'s', 'h', 'a', '1'})
 var SHA256ID = HashID([4]byte{'s', '2', '5', '6'})
 var NullHashID = HashID([4]byte{0, 0, 0, 0})
 
+// Size returns the hash size in bytes, or 0 if the ID is not one this
+// package knows. Callers must treat 0 as an error: the hash ID can come
+// straight out of a file header, so this must not panic.
 func (i HashID) Size() int {
 	switch i {
 	case NullHashID, SHA1ID:
 		return 20
 	case SHA256ID:
 		return 32
 	}
-	panic("unknown hash")
+	return 0
 }
 
 // Table is a read interface for reftables, either file reftables or merged reftables.

block.go

@@ -175,38 +175,61 @@ func (br *blockReader) getType() byte {
 	return br.block[br.headerOff]
 }
 
-// newBlockWriter prepares for reading a block.
+// newBlockReader prepares for reading a block.
 func newBlockReader(block []byte, headerOff uint32, tableBlockSize uint32, hashSize int) (*blockReader, error) {
 
+	if uint64(headerOff)+4 > uint64(len(block)) {
+		return nil, fmtError
+	}
 	fullBlockSize := tableBlockSize
 	typ := block[headerOff]
 	if !isBlockType(typ) {
 		return nil, fmt.Errorf("reftable: unknown block type %c", typ)
 	}
 
 	sz := getU24(block[headerOff+1:])
+	if uint64(sz) < uint64(headerOff)+6 {
+		return nil, fmtError
+	}
+	if typ != blockTypeLog && uint64(sz) > uint64(len(block)) {
+		return nil, fmtError
+	}
 
 	if typ == blockTypeLog {
+		// sz is a 3-byte field read straight from the block header, and is
+		// deliberately not bounded by len(block) above because a log block
+		// declares its *decompressed* size. Bound it by what this input
+		// could possibly produce: DEFLATE cannot expand by more than
+		// 1032:1, so anything larger is malformed. Without this a ~40 byte
+		// table can reserve 16MiB up front, before a byte is decompressed.
+		if uint64(sz) > uint64(len(block))*maxDeflateRatio {
+			return nil, fmtError
+		}
 		decompress := make([]byte, 0, sz)
 		buf := bytes.NewBuffer(block)
 		out := bytes.NewBuffer(decompress)
 
 		before := buf.Len()
 
 		// Consume header
-		io.CopyN(out, buf, int64(headerOff+4))
+		if _, err := io.CopyN(out, buf, int64(headerOff)+4); err != nil {
+			return nil, err
+		}
 		r, err := zlib.NewReader(buf)
 		if err != nil {
 			return nil, err
 		}
-		// Have to use io.Copy. zlib stream has a terminator,
-		// which we must consume, so go until EOF.
-		if _, err := io.Copy(out, r); err != nil {
+		defer r.Close()
+		// Read one byte beyond the declared payload size so an oversized
+		// stream is detected by the out.Len() != sz check below rather than
+		// being decompressed in full. Valid streams reach EOF within the
+		// limit and consume the zlib trailer, so the compressed-block
+		// accounting below (before - buf.Len()) stays correct.
+		limit := int64(sz) - int64(headerOff) - 4 + 1
+		if _, err := io.Copy(out, io.LimitReader(r, limit)); err != nil {
 			return nil, err
 		}
 
-		r.Close()
-
 		if out.Len() != int(sz) {
 			return nil, fmtError
 		}
@@ -228,7 +251,18 @@ func newBlockReader(block []byte, headerOff uint32, tableBlockSize uint32, hashS
 
 	restartCount := binary.BigEndian.Uint16(block[len(block)-2:])
 	restartStart := len(block) - 2 - 3*int(restartCount)
+	if restartStart < int(headerOff)+4 {
+		return nil, fmtError
+	}
 	restartBytes := block[restartStart:]
+	var previous uint32
+	for i := 0; i < int(restartCount); i++ {
+		off := getU24(restartBytes[3*i:])
+		if off < headerOff+4 || off >= uint32(restartStart) || (i > 0 && off <= previous) {
+			return nil, fmtError
+		}
+		previous = off
+	}
 	block = block[:restartStart]
 
 	br := &blockReader{

block_validation_test.go

@@ -0,0 +1,43 @@
+package reftable
+
+import (
+	"bytes"
+	"compress/zlib"
+	"testing"
+)
+
+func TestBlockReaderRejectsInvalidLayout(t *testing.T) {
+	for _, tc := range []struct {
+		name      string
+		data      []byte
+		headerOff uint32
+	}{
+		{"missing_header", nil, 0},
+		{"short_header", []byte{'r', 0, 0}, 0},
+		{"header_offset", []byte{'r', 0, 0, 6, 0, 0}, 24},
+		{"oversized_restart_count", []byte{'r', 0, 0, 6, 255, 255}, 0},
+		{"restart_in_header", []byte{'r', 0, 0, 10, 0, 0, 0, 0, 0, 1}, 0},
+		{"restart_in_trailer", []byte{'r', 0, 0, 10, 0, 0, 0, 5, 0, 1}, 0},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			if _, err := newBlockReader(tc.data, tc.headerOff, 0, 20); err == nil {
+				t.Fatal("invalid block layout accepted")
+			}
+		})
+	}
+}
+
+func TestBlockReaderRejectsOversizedLogStream(t *testing.T) {
+	var compressed bytes.Buffer
+	compressed.Write([]byte{'g', 0, 0, 6}) // Declares only two payload bytes.
+	zw := zlib.NewWriter(&compressed)
+	if _, err := zw.Write(bytes.Repeat([]byte{0}, 1<<20)); err != nil {
+		t.Fatal(err)
+	}
+	if err := zw.Close(); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := newBlockReader(compressed.Bytes(), 0, 0, 20); err == nil {
+		t.Fatal("oversized decompressed payload accepted")
+	}
+}

c/block.c

@@ -13,6 +13,7 @@ license that can be found in the LICENSE file or at
 #include "record.h"
 #include "reftable-error.h"
 #include "system.h"
+#include <limits.h>
 #include <zlib.h>
 
 int header_size(int version)
@@ -186,13 +187,22 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
 		      int hash_size)
 {
 	uint32_t full_block_size = table_block_size;
-	uint8_t typ = block->data[header_off];
-	uint32_t sz = get_be24(block->data + header_off + 1);
+	uint8_t typ;
+	uint32_t sz;
 
 	uint16_t restart_count = 0;
 	uint32_t restart_start = 0;
 	uint8_t *restart_bytes = NULL;
+	uint32_t previous = 0;
+	int i;
 
+	if ((uint64_t)header_off + 4 > block->len)
+		return REFTABLE_FORMAT_ERROR;
+	typ = block->data[header_off];
+	sz = get_be24(block->data + header_off + 1);
+	if ((uint64_t)sz < (uint64_t)header_off + 6 ||
+	    (typ != BLOCK_TYPE_LOG && sz > block->len))
+		return REFTABLE_FORMAT_ERROR;
 	if (!reftable_is_block_type(typ))
 		return REFTABLE_FORMAT_ERROR;
 
@@ -216,8 +226,10 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
 			return REFTABLE_ZLIB_ERROR;
 		}
 
-		if (dst_len + block_header_skip != sz)
+		if (dst_len + block_header_skip != sz) {
+			reftable_free(uncompressed);
 			return REFTABLE_FORMAT_ERROR;
+		}
 
 		/* We're done with the input data. */
 		reftable_block_done(block);
@@ -236,8 +248,17 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
 	}
 
 	restart_count = get_be16(block->data + sz - 2);
+	if (2 + 3 * (uint32_t)restart_count > sz - header_off - 4)
+		return REFTABLE_FORMAT_ERROR;
 	restart_start = sz - 2 - 3 * restart_count;
 	restart_bytes = block->data + restart_start;
+	for (i = 0; i < restart_count; i++) {
+		uint32_t off = get_be24(restart_bytes + 3 * i);
+		if (off < header_off + 4 || off >= restart_start ||
+		    (i > 0 && off <= previous))
+			return REFTABLE_FORMAT_ERROR;
+		previous = off;
+	}
 
 	/* transfer ownership. */
 	br->block = *block;

c/block_test.c

@@ -113,8 +113,38 @@ static void test_block_read_write(void)
 	}
 }
 
+static void test_block_rejects_invalid_layout(void)
+{
+	uint8_t data[][10] = {
+		{ 'r', 0, 0, 0 },
+		{ 'r', 0, 0, 1 },
+		{ 'r', 255, 255, 255 },
+		{ 'r', 0, 0, 6, 255, 255 },
+		{ 'r', 0, 0, 10, 0, 0, 0, 0, 0, 1 },
+		{ 'r', 0, 0, 10, 0, 0, 0, 5, 0, 1 },
+		{ 'g', 0, 0, 1 },
+	};
+	int i;
+	for (i = 0; i < ARRAY_SIZE(data); i++) {
+		struct block_reader br = { 0 };
+		struct reftable_block block = {
+			.data = data[i],
+			.len = sizeof(data[i]),
+		};
+		int err = block_reader_init(&br, &block, 0, 0, GIT_SHA1_RAWSZ);
+		EXPECT(err == REFTABLE_FORMAT_ERROR);
+	}
+	for (i = 0; i < 4; i++) {
+		struct block_reader br = { 0 };
+		struct reftable_block block = { .data = data[0], .len = i };
+		int err = block_reader_init(&br, &block, 0, 0, GIT_SHA1_RAWSZ);
+		EXPECT(err == REFTABLE_FORMAT_ERROR);
+	}
+}
+
 int block_test_main(int argc, const char *argv[])
 {
 	RUN_TEST(test_block_read_write);
+	RUN_TEST(test_block_rejects_invalid_layout);
 	return 0;
 }

c/iter.c

@@ -146,12 +146,15 @@ static int indexed_table_ref_iter_next(void *p, struct reftable_record *rec)
 			}
 			continue;
 		}
-		/* BUG */
-		if (!memcmp(it->oid.buf, ref->value.val2.target_value,
-			    it->oid.len) ||
-		    !memcmp(it->oid.buf, ref->value.val2.value, it->oid.len)) {
+		ref->update_index += it->r->min_update_index;
+		if (ref->value_type == REFTABLE_REF_VAL2 &&
+		    (!memcmp(it->oid.buf, ref->value.val2.target_value,
+			     it->oid.len) ||
+		     !memcmp(it->oid.buf, ref->value.val2.value, it->oid.len)))
+			return 0;
+		if (ref->value_type == REFTABLE_REF_VAL1 &&
+		    !memcmp(it->oid.buf, ref->value.val1, it->oid.len))
 			return 0;
-		}
 	}
 }
 

c/reader.c

@@ -292,10 +292,16 @@ int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,
 	if (err < 0)
 		return err;
 
+	if ((uint64_t)header_off + 4 > block.len) {
+		reftable_block_done(&block);
+		return REFTABLE_FORMAT_ERROR;
+	}
 	block_size = extract_block_size(block.data, &block_typ, next_off,
 					r->version);
-	if (block_size < 0)
-		return block_size;
+	if (!reftable_is_block_type(block_typ)) {
+		reftable_block_done(&block);
+		return REFTABLE_FORMAT_ERROR;
+	}
 
 	if (want_typ != BLOCK_TYPE_ANY && block_typ != want_typ) {
 		reftable_block_done(&block);
@@ -310,8 +316,11 @@ int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,
 		}
 	}
 
-	return block_reader_init(br, &block, header_off, r->block_size,
+	err = block_reader_init(br, &block, header_off, r->block_size,
 				 hash_size(r->hash_id));
+	if (err < 0)
+		reftable_block_done(&block);
+	return err;
 }
 
 static int table_iter_next_block(struct table_iter *dest,

c/readwrite_test.c

@@ -421,7 +421,7 @@ static void test_table_read_write_seek_index(void)
 	test_table_read_write_seek(1, GIT_SHA1_FORMAT_ID);
 }
 
-static void test_table_refs_for(int indexed)
+static void test_table_refs_for(int indexed, int peeled)
 {
 	int N = 50;
 	char **want_names = reftable_calloc(sizeof(char *) * (N + 1));
@@ -446,6 +446,7 @@ static void test_table_refs_for(int indexed)
 	int j;
 
 	set_test_hash(want_hash, 4);
+	reftable_writer_set_limits(w, 100, 200);
 
 	for (i = 0; i < N; i++) {
 		uint8_t hash[GIT_SHA1_RAWSZ];
@@ -464,9 +465,15 @@ static void test_table_refs_for(int indexed)
 
 		set_test_hash(hash1, i / 4);
 		set_test_hash(hash2, 3 + i / 4);
-		ref.value_type = REFTABLE_REF_VAL2;
-		ref.value.val2.value = hash1;
-		ref.value.val2.target_value = hash2;
+		ref.update_index = 150;
+		if (peeled) {
+			ref.value_type = REFTABLE_REF_VAL2;
+			ref.value.val2.value = hash1;
+			ref.value.val2.target_value = hash2;
+		} else {
+			ref.value_type = REFTABLE_REF_VAL1;
+			ref.value.val1 = hash1;
+		}
 
 		/* 80 bytes / entry, so 3 entries per block. Yields 17
 		 */
@@ -475,7 +482,7 @@ static void test_table_refs_for(int indexed)
 		EXPECT(n == 0);
 
 		if (!memcmp(hash1, want_hash, GIT_SHA1_RAWSZ) ||
-		    !memcmp(hash2, want_hash, GIT_SHA1_RAWSZ)) {
+		    (peeled && !memcmp(hash2, want_hash, GIT_SHA1_RAWSZ))) {
 			want_names[want_names_len++] = xstrdup(name);
 		}
 	}
@@ -511,6 +518,7 @@ static void test_table_refs_for(int indexed)
 
 		EXPECT(j < want_names_len);
 		EXPECT(0 == strcmp(ref.refname, want_names[j]));
+		EXPECT(ref.update_index == 150);
 		j++;
 		reftable_ref_record_release(&ref);
 	}
@@ -524,12 +532,14 @@ static void test_table_refs_for(int indexed)
 
 static void test_table_refs_for_no_index(void)
 {
-	test_table_refs_for(0);
+	test_table_refs_for(0, 0);
+	test_table_refs_for(0, 1);
 }
 
 static void test_table_refs_for_obj_index(void)
 {
-	test_table_refs_for(1);
+	test_table_refs_for(1, 0);
+	test_table_refs_for(1, 1);
 }
 
 static void test_write_empty_table(void)