Merge branch '2018-09-13-radix-tree-bug'

base/Makefile

@@ -12,7 +12,7 @@
 
 # Uncomment this to build the simple radix tree.  You'll need to make clean too.
 # Comment to build the advanced radix tree.
-base/data-struct/radix-tree.o: CFLAGS += -DSIMPLE_RADIX_TREE
+#base/data-struct/radix-tree.o: CFLAGS += -DSIMPLE_RADIX_TREE
 
 BASE_SOURCE=\
 	base/data-struct/radix-tree.c \

base/data-struct/radix-tree-adaptive.c

@@ -581,12 +581,11 @@ static void _degrade_to_n16(struct node48 *n48, struct value *result)
         for (i = 0; i < 256; i++) {
 	        if (n48->keys[i] < 48) {
 		        n16->keys[count] = i;
+		        n16->values[count] = n48->values[n48->keys[i]];
 		        count++;
 	        }
         }
 
-        memcpy(n16->values, n48->values, n48->nr_entries * sizeof(*n16->values));
-
         free(n48);
 
 	result->type = NODE16;
@@ -598,16 +597,16 @@ static void _degrade_to_n48(struct node256 *n256, struct value *result)
         unsigned i, count = 0;
         struct node48 *n48 = zalloc(sizeof(*n48));
 
-	memset(n48->keys, 48, sizeof(n48->keys));
-
         n48->nr_entries = n256->nr_entries;
         for (i = 0; i < 256; i++) {
 		if (n256->values[i].type == UNSET)
-        		continue;
+			n48->keys[i] = 48;
 
-		n48->keys[i] = count;
+		else {
-		n48->values[count] = n256->values[i];
+			n48->keys[i] = count;
-		count++;
+			n48->values[count] = n256->values[i];
+			count++;
+		}
         }
 
         free(n256);
@@ -1036,6 +1035,7 @@ void radix_tree_iterate(struct radix_tree *rt, uint8_t *kb, uint8_t *ke,
 
 static bool _check_nodes(struct value *v, unsigned *count)
 {
+	uint64_t bits;
 	unsigned i, ncount;
 	struct value_chain *vc;
 	struct prefix_chain *pc;
@@ -1090,22 +1090,47 @@ static bool _check_nodes(struct value *v, unsigned *count)
 		return true;
 
 	case NODE48:
+		bits = 0;
 		n48 = v->value.ptr;
 		ncount = 0;
 		for (i = 0; i < 256; i++) {
 			if (n48->keys[i] < 48) {
+				if (n48->keys[i] >= n48->nr_entries) {
+					fprintf(stderr, "referencing value past nr_entries (n48)\n");
+					return false;
+				}
+
+				if (bits & (1ull << n48->keys[i])) {
+					fprintf(stderr, "duplicate entry (n48) %u\n", (unsigned) n48->keys[i]);
+					return false;
+				}
+				bits = bits | (1ull << n48->keys[i]);
 				ncount++;
+
 				if (!_check_nodes(n48->values + n48->keys[i], count))
 					return false;
 			}
 		}
 
+		for (i = 0; i < n48->nr_entries; i++) {
+			if (!(bits & (1ull << i))) {
+				fprintf(stderr, "not all values are referenced (n48)\n");
+				return false;
+			}
+		}
+
 		if (ncount != n48->nr_entries) {
 			fprintf(stderr, "incorrect number of entries in n48, n48->nr_entries = %u, actual = %u\n",
                                 n48->nr_entries, ncount);
 			return false;
 		}
 
+		for (i = 0; i < n48->nr_entries; i++)
+			if (n48->values[i].type == UNSET) {
+				fprintf(stderr, "value in UNSET (n48)\n");
+				return false;
+			}
+
 		for (i = n48->nr_entries; i < 48; i++)
 			if (n48->values[i].type != UNSET) {
 				fprintf(stderr, "unused value is not UNSET (n48)\n");
@@ -1234,9 +1259,10 @@ static void _dump(FILE *out, struct value v, unsigned indent)
 				fprintf(out, "%x ", i);
 		fprintf(out, ">\n");
 
-		for (i = 0; i < 256; i++)
+		for (i = 0; i < n48->nr_entries; i++) {
-			if (n48->keys[i] < 48)
+			assert(n48->values[i].type != UNSET);
-				_dump(out, n48->values[i], indent + 1);
+			_dump(out, n48->values[i], indent + 1);
+		}
 		break;
 
 	case NODE256:

test/unit/Makefile

@@ -29,6 +29,8 @@ UNIT_SOURCE=\
 	test/unit/string_t.c \
 	test/unit/vdo_t.c
 
+test/unit/radix_tree_t.o: test/unit/rt_case1.c
+
 UNIT_DEPENDS=$(subst .c,.d,$(UNIT_SOURCE))
 UNIT_OBJECTS=$(UNIT_SOURCE:%.c=%.o)
 CLEAN_TARGETS+=$(UNIT_DEPENDS) $(UNIT_OBJECTS)

test/unit/radix_tree_t.c

@@ -750,6 +750,65 @@ static void test_bcache_scenario2(void *fixture)
 
 //----------------------------------------------------------------
 
+struct key_parts {
+	uint32_t fd;
+	uint64_t b;
+} __attribute__ ((packed));
+
+union key {
+	struct key_parts parts;
+        uint8_t bytes[12];
+};
+
+static void __lookup_matches(struct radix_tree *rt, int fd, uint64_t b, uint64_t expected)
+{
+	union key k;
+	union radix_value v;
+
+	k.parts.fd = fd;
+	k.parts.b = b;
+	T_ASSERT(radix_tree_lookup(rt, k.bytes, k.bytes + sizeof(k.bytes), &v));
+	T_ASSERT(v.n == expected);
+}
+
+static void __lookup_fails(struct radix_tree *rt, int fd, uint64_t b)
+{
+	union key k;
+	union radix_value v;
+
+	k.parts.fd = fd;
+	k.parts.b = b;
+	T_ASSERT(!radix_tree_lookup(rt, k.bytes, k.bytes + sizeof(k.bytes), &v));
+}
+
+static void __insert(struct radix_tree *rt, int fd, uint64_t b, uint64_t n)
+{
+	union key k;
+	union radix_value v;
+
+	k.parts.fd = fd;
+	k.parts.b = b;
+	v.n = n;
+	T_ASSERT(radix_tree_insert(rt, k.bytes, k.bytes + sizeof(k.bytes), v));
+}
+
+static void __invalidate(struct radix_tree *rt, int fd)
+{
+	union key k;
+
+	k.parts.fd = fd;
+	radix_tree_remove_prefix(rt, k.bytes, k.bytes + sizeof(k.parts.fd));
+	radix_tree_is_well_formed(rt);
+}
+
+static void test_bcache_scenario3(void *fixture)
+{
+	struct radix_tree *rt = fixture;
+
+	#include "test/unit/rt_case1.c"
+}
+
+//----------------------------------------------------------------
 #define T(path, desc, fn) register_test(ts, "/base/data-struct/radix-tree/" path, desc, fn)
 
 void radix_tree_tests(struct dm_list *all_tests)
@@ -784,6 +843,7 @@ void radix_tree_tests(struct dm_list *all_tests)
 	T("destroy-calls-dtr", "destroy should call the dtr for all values", test_destroy_calls_dtr);
 	T("bcache-scenario", "A specific series of keys from a bcache scenario", test_bcache_scenario);
 	T("bcache-scenario-2", "A second series of keys from a bcache scenario", test_bcache_scenario2);
+	T("bcache-scenario-3", "A third series of keys from a bcache scenario", test_bcache_scenario3);
 
 	dm_list_add(all_tests, &ts->list);
 }