From 968e43df636964600a83cc45244e95da89f571b1 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Thu, 25 Jan 2024 04:49:13 +0000 Subject: [PATCH 01/12] utils: add map iteration iterator --- tests/unit/s2n_map_test.c | 328 ++++++++++++++++++++++++++------------ utils/s2n_map.c | 88 ++++++++++ utils/s2n_map.h | 7 + utils/s2n_map_internal.h | 8 +- 4 files changed, 325 insertions(+), 106 deletions(-) diff --git a/tests/unit/s2n_map_test.c b/tests/unit/s2n_map_test.c index 8f1d8bb8437..a09adae6771 100644 --- a/tests/unit/s2n_map_test.c +++ b/tests/unit/s2n_map_test.c @@ -19,70 +19,114 @@ #include "api/s2n.h" #include "s2n_test.h" +#include "utils/s2n_map_internal.h" + +DEFINE_POINTER_CLEANUP_FUNC(struct s2n_map_iterator *, s2n_map_iterator_free); int main(int argc, char **argv) { - char keystr[sizeof("ffff")]; - char valstr[sizeof("16384")]; - struct s2n_map *empty, *map; - struct s2n_blob key = { 0 }; - struct s2n_blob val = { 0 }; - bool key_found; + /* s2n_map test */ + { + char keystr[sizeof("ffff")]; + char valstr[sizeof("16384")]; + uint32_t size; + struct s2n_map *empty, *map; + struct s2n_blob key = { 0 }; + struct s2n_blob val = { 0 }; + bool key_found; + + BEGIN_TEST(); + EXPECT_SUCCESS(s2n_disable_tls13_in_test()); + + EXPECT_NOT_NULL(empty = s2n_map_new()); + EXPECT_OK(s2n_map_size(empty, &size)); + EXPECT_EQUAL(size, 0); + + /* Try a lookup on an empty map. Expect an error because the map is still mutable. */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 1234)); + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; + EXPECT_ERROR(s2n_map_lookup(empty, &key, &val, &key_found)); - BEGIN_TEST(); - EXPECT_SUCCESS(s2n_disable_tls13_in_test()); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 1234)); + val.data = (void *) valstr; + val.size = strlen(valstr) + 1; - EXPECT_NOT_NULL(empty = s2n_map_new()); + /* Try to add/put key with zero-size data. Expect failures */ + key.size = 0; + EXPECT_ERROR(s2n_map_add(empty, &key, &val)); + EXPECT_ERROR(s2n_map_put(empty, &key, &val)); + key.size = strlen(keystr) + 1; - /* Try a lookup on an empty map. Expect an error because the map is still mutable. */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 1234)); - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; - EXPECT_ERROR(s2n_map_lookup(empty, &key, &val, &key_found)); + /* Make the empty map complete */ + EXPECT_OK(s2n_map_complete(empty)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 1234)); - val.data = (void *) valstr; - val.size = strlen(valstr) + 1; + /* Lookup and expect no result */ + EXPECT_OK(s2n_map_lookup(empty, &key, &val, &key_found)); + EXPECT_EQUAL(key_found, false); - /* Try to add/put key with zero-size data. Expect failures */ - key.size = 0; - EXPECT_ERROR(s2n_map_add(empty, &key, &val)); - EXPECT_ERROR(s2n_map_put(empty, &key, &val)); - key.size = strlen(keystr) + 1; + /* Done with the empty map */ + EXPECT_OK(s2n_map_free(empty)); - /* Make the empty map complete */ - EXPECT_OK(s2n_map_complete(empty)); + /* Expect failure since initial map size is zero */ + EXPECT_NULL(map = s2n_map_new_with_initial_capacity(0)); - /* Lookup and expect no result */ - EXPECT_OK(s2n_map_lookup(empty, &key, &val, &key_found)); - EXPECT_EQUAL(key_found, false); + /* Create map with the smallest initial size */ + EXPECT_NOT_NULL(map = s2n_map_new_with_initial_capacity(1)); - /* Done with the empty map */ - EXPECT_OK(s2n_map_free(empty)); + /* Insert 8k key value pairs of the form hex(i) -> dec(i) */ + for (int i = 0; i < 8192; i++) { + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); - /* Expect failure since initial map size is zero */ - EXPECT_NULL(map = s2n_map_new_with_initial_capacity(0)); + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; + val.data = (void *) valstr; + val.size = strlen(valstr) + 1; - /* Create map with the smallest initial size */ - EXPECT_NOT_NULL(map = s2n_map_new_with_initial_capacity(1)); + EXPECT_OK(s2n_map_add(map, &key, &val)); + } + EXPECT_OK(s2n_map_size(map, &size)); + EXPECT_EQUAL(size, 8192); - /* Insert 8k key value pairs of the form hex(i) -> dec(i) */ - for (int i = 0; i < 8192; i++) { - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); + /* Try adding some duplicates */ + for (int i = 0; i < 10; i++) { + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; - val.data = (void *) valstr; - val.size = strlen(valstr) + 1; + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; + val.data = (void *) valstr; + val.size = strlen(valstr) + 1; - EXPECT_OK(s2n_map_add(map, &key, &val)); - } + EXPECT_ERROR(s2n_map_add(map, &key, &val)); + } + EXPECT_OK(s2n_map_size(map, &size)); + EXPECT_EQUAL(size, 8192); + + /* Try replacing some entries */ + for (int i = 0; i < 10; i++) { + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i + 1)); - /* Try adding some duplicates */ - for (int i = 0; i < 10; i++) { - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; + val.data = (void *) valstr; + val.size = strlen(valstr) + 1; + + EXPECT_OK(s2n_map_put(map, &key, &val)); + } + + /* Try a lookup before the map is complete: should fail */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 1)); + EXPECT_ERROR(s2n_map_lookup(map, &key, &val, &key_found)); + + /* Make the map complete */ + EXPECT_OK(s2n_map_complete(map)); + + /* Make sure that add-after-complete fails */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 8193)); key.data = (void *) keystr; key.size = strlen(keystr) + 1; @@ -90,88 +134,162 @@ int main(int argc, char **argv) val.size = strlen(valstr) + 1; EXPECT_ERROR(s2n_map_add(map, &key, &val)); - } - /* Try replacing some entries */ - for (int i = 0; i < 10; i++) { - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i + 1)); + /* Check for equivalence */ + for (int i = 0; i < 8192; i++) { + if (i >= 10) { + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); + } else { + /* The first 10 entries were overwritten with i+1 */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i + 1)); + } + + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; + + EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); + EXPECT_EQUAL(key_found, true); + + EXPECT_SUCCESS(memcmp(val.data, valstr, strlen(valstr) + 1)); + } + + /* Check for a key that shouldn't be there */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; + EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); + EXPECT_EQUAL(key_found, false); + + /* Make the map mutable */ + EXPECT_OK(s2n_map_unlock(map)); + /* Make sure that add-after-unlock succeeds */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 8193)); key.data = (void *) keystr; key.size = strlen(keystr) + 1; val.data = (void *) valstr; val.size = strlen(valstr) + 1; - EXPECT_OK(s2n_map_put(map, &key, &val)); - } + EXPECT_OK(s2n_map_add(map, &key, &val)); + + /* Complete the map again */ + EXPECT_OK(s2n_map_complete(map)); + + /* Check the element added after map unlock */ + EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); + EXPECT_EQUAL(key_found, true); + EXPECT_SUCCESS(memcmp(val.data, valstr, strlen(valstr) + 1)); + + EXPECT_OK(s2n_map_free(map)); + }; + + /* s2n_map_iterator test */ + { + struct s2n_map *map = s2n_map_new(); + EXPECT_NOT_NULL(map); + /* fail to initialize an iterator on a mutable map */ + { + DEFER_CLEANUP(struct s2n_map_iterator *iter = NULL, s2n_map_iterator_free_pointer); + EXPECT_ERROR(s2n_map_iterator_new(map, &iter)); + }; + + EXPECT_OK(s2n_map_complete(map)); + + /* has next is false on an empty map, and next returns an error */ + { + DEFER_CLEANUP(struct s2n_map_iterator *iter = NULL, s2n_map_iterator_free_pointer); + EXPECT_OK(s2n_map_iterator_new(map, &iter)); + + bool has_next = false; + EXPECT_OK(s2n_map_iterator_has_next(iter, &has_next)); + EXPECT_FALSE(has_next); + + struct s2n_blob value = { 0 }; + EXPECT_ERROR(s2n_map_iterator_next(iter, &value)); + }; + + EXPECT_OK(s2n_map_unlock(map)); + for (uint8_t i = 0; i < 10; i++) { + struct s2n_blob key = { .size = 1, .data = &i }; + struct s2n_blob val = { .size = 1, .data = &i }; + EXPECT_OK(s2n_map_put(map, &key, &val)); + } + EXPECT_OK(s2n_map_complete(map)); - /* Try a lookup before the map is complete: should fail */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 1)); - EXPECT_ERROR(s2n_map_lookup(map, &key, &val, &key_found)); + /* iterator goes over all elements */ + { + bool seen[10] = { 0 }; - /* Make the map complete */ - EXPECT_OK(s2n_map_complete(map)); + DEFER_CLEANUP(struct s2n_map_iterator *iter = NULL, s2n_map_iterator_free_pointer); + EXPECT_OK(s2n_map_iterator_new(map, &iter)); - /* Make sure that add-after-complete fails */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 8193)); + bool has_next = false; + struct s2n_blob value = { 0 }; + for (size_t i = 0; i < 10; i++) { + EXPECT_OK(s2n_map_iterator_has_next(iter, &has_next)); + EXPECT_TRUE(has_next); - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; - val.data = (void *) valstr; - val.size = strlen(valstr) + 1; + EXPECT_OK(s2n_map_iterator_next(iter, &value)); + seen[*value.data] = true; + } - EXPECT_ERROR(s2n_map_add(map, &key, &val)); + /* all elements have been iterated over */ + EXPECT_OK(s2n_map_iterator_has_next(iter, &has_next)); + EXPECT_FALSE(has_next); - /* Check for equivalence */ - for (int i = 0; i < 8192; i++) { - if (i >= 10) { - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); - } else { - /* The first 10 entries were overwritten with i+1 */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i + 1)); + EXPECT_ERROR(s2n_map_iterator_next(iter, &value)); + + /* all elements were seen */ + for (size_t i = 0; i < 10; i++) { + EXPECT_TRUE(seen[i]); + } } - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; + EXPECT_OK(s2n_map_free(map)); - EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); - EXPECT_EQUAL(key_found, true); + /* test first and last slots in table */ + { + struct s2n_blob blobs[4] = { 0 }; + for (uint8_t i = 0; i < 4; i++) { + s2n_alloc(&blobs[i], 1); + *blobs[i].data = i; + } - EXPECT_SUCCESS(memcmp(val.data, valstr, strlen(valstr) + 1)); - } + struct s2n_map *map = s2n_map_new(); + EXPECT_NOT_NULL(map); - /* Check for a key that shouldn't be there */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; - EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); - EXPECT_EQUAL(key_found, false); + /* set values in map to 0 and 1 */ + map->table[0].value = blobs[0]; + map->table[0].key = blobs[2]; + map->table[map->capacity - 1].value = blobs[1]; + map->table[map->capacity - 1].key = blobs[3]; - /* Make the map mutable */ - EXPECT_OK(s2n_map_unlock(map)); - /* Make sure that add-after-unlock succeeds */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 8193)); + map->size = 2; + EXPECT_OK(s2n_map_complete(map)); - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; - val.data = (void *) valstr; - val.size = strlen(valstr) + 1; + DEFER_CLEANUP(struct s2n_map_iterator *iter = NULL, s2n_map_iterator_free_pointer); + EXPECT_OK(s2n_map_iterator_new(map, &iter)); + bool seen[2] = { 0 }; - EXPECT_OK(s2n_map_add(map, &key, &val)); + bool has_next = false; + struct s2n_blob value = { 0 }; + for (size_t i = 0; i < 2; i++) { + EXPECT_OK(s2n_map_iterator_has_next(iter, &has_next)); + EXPECT_TRUE(has_next); - /* Complete the map again */ - EXPECT_OK(s2n_map_complete(map)); + EXPECT_OK(s2n_map_iterator_next(iter, &value)); + seen[*value.data] = true; + } - /* Check the element added after map unlock */ - EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); - EXPECT_EQUAL(key_found, true); - EXPECT_SUCCESS(memcmp(val.data, valstr, strlen(valstr) + 1)); + /* assert that 0 and 1 were both seen */ + EXPECT_TRUE(seen[0] && seen[1]); - EXPECT_OK(s2n_map_free(map)); + EXPECT_OK(s2n_map_free(map)); + }; + }; END_TEST(); } diff --git a/utils/s2n_map.c b/utils/s2n_map.c index e95679553e0..b5171d94681 100644 --- a/utils/s2n_map.c +++ b/utils/s2n_map.c @@ -246,3 +246,91 @@ S2N_RESULT s2n_map_free(struct s2n_map *map) return S2N_RESULT_OK; } + +S2N_RESULT s2n_map_size(struct s2n_map *map, uint32_t *size) +{ + RESULT_ENSURE_REF(map); + *size = map->size; + return S2N_RESULT_OK; +} + +/* Update the internal state so that `current_index` will point to the next value + * in the table or will equal `map->capacity` if there are no more elements in + * the map. + */ +S2N_RESULT s2n_map_iterator_advance(struct s2n_map_iterator *iter) +{ + RESULT_ENSURE_REF(iter); + RESULT_ENSURE_REF(iter->map); + + while (++iter->current_index < iter->map->capacity) { + /* a value was found in the map */ + if (iter->map->table[iter->current_index].key.size) { + return S2N_RESULT_OK; + } + } + /* no more values were found in the map */ + return S2N_RESULT_OK; +} + +S2N_RESULT s2n_map_iterator_new(struct s2n_map *map, struct s2n_map_iterator **iter_out) +{ + RESULT_ENSURE_REF(map); + RESULT_ENSURE(map->immutable, S2N_ERR_MAP_MUTABLE); + + struct s2n_blob mem = { 0 }; + struct s2n_map_iterator *iter; + + RESULT_GUARD_POSIX(s2n_alloc(&mem, sizeof(struct s2n_map_iterator))); + + iter = (void *) mem.data; + iter->map = map; + iter->current_index = 0; + + /* Advance the index to point to the first value in the map. This isn't + * necessary if the slot at index 0 already contains a value. + */ + if (iter->map->table[0].key.size == 0) { + RESULT_GUARD(s2n_map_iterator_advance(iter)); + } + + *iter_out = iter; + + return S2N_RESULT_OK; +} + +S2N_RESULT s2n_map_iterator_next(struct s2n_map_iterator *iter, struct s2n_blob *value) +{ + RESULT_ENSURE_REF(iter); + RESULT_ENSURE(iter->map->immutable, S2N_ERR_MAP_MUTABLE); + + bool has_next = false; + RESULT_GUARD(s2n_map_iterator_has_next(iter, &has_next)); + RESULT_ENSURE(has_next, S2N_ERR_SAFETY); + + value->data = iter->map->table[iter->current_index].value.data; + value->size = iter->map->table[iter->current_index].value.size; + + RESULT_GUARD(s2n_map_iterator_advance(iter)); + + return S2N_RESULT_OK; +} + +S2N_RESULT s2n_map_iterator_has_next(const struct s2n_map_iterator *iter, bool *has_next) +{ + RESULT_ENSURE_REF(iter); + RESULT_ENSURE_REF(iter->map); + *has_next = iter->current_index < iter->map->capacity; + + return S2N_RESULT_OK; +} + +int s2n_map_iterator_free(struct s2n_map_iterator *iter) +{ + if (iter == NULL) { + return S2N_SUCCESS; + } + POSIX_GUARD(s2n_free_object((uint8_t **) &iter, sizeof(struct s2n_map_iterator))); + + return S2N_SUCCESS; +} diff --git a/utils/s2n_map.h b/utils/s2n_map.h index 259082936cf..c49b95a3676 100644 --- a/utils/s2n_map.h +++ b/utils/s2n_map.h @@ -21,6 +21,7 @@ #include "utils/s2n_result.h" struct s2n_map; +struct s2n_map_iterator; struct s2n_map *s2n_map_new(); struct s2n_map *s2n_map_new_with_initial_capacity(uint32_t capacity); @@ -30,3 +31,9 @@ S2N_RESULT s2n_map_complete(struct s2n_map *map); S2N_RESULT s2n_map_unlock(struct s2n_map *map); S2N_RESULT s2n_map_lookup(const struct s2n_map *map, struct s2n_blob *key, struct s2n_blob *value, bool *key_found); S2N_RESULT s2n_map_free(struct s2n_map *map); +S2N_RESULT s2n_map_size(struct s2n_map *map, uint32_t *size); + +S2N_RESULT s2n_map_iterator_new(struct s2n_map *map, struct s2n_map_iterator **iter); +S2N_RESULT s2n_map_iterator_next(struct s2n_map_iterator *iter, struct s2n_blob *value); +S2N_RESULT s2n_map_iterator_has_next(const struct s2n_map_iterator *iter, bool *has_next); +int s2n_map_iterator_free(struct s2n_map_iterator *iter); diff --git a/utils/s2n_map_internal.h b/utils/s2n_map_internal.h index 01d629fb16f..97ba1d7253b 100644 --- a/utils/s2n_map_internal.h +++ b/utils/s2n_map_internal.h @@ -32,5 +32,11 @@ struct s2n_map { int immutable; /* Pointer to the hash-table, should be capacity * sizeof(struct s2n_map_entry) */ - struct s2n_map_entry *table; + struct s2n_map_entry* table; +}; + +struct s2n_map_iterator { + struct s2n_map* map; + /* Index of the entry to be returned on the next `s2n_map_iterator_next()` call. */ + uint32_t current_index; }; From 09bd422b68bd3544976b8ec51304712a149882e1 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Thu, 25 Jan 2024 08:22:51 +0000 Subject: [PATCH 02/12] address ci errors * shadow variables in test * fix pointer alignment, because clang won't? --- tests/unit/s2n_map_test.c | 20 ++++++++++---------- utils/s2n_map_internal.h | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/unit/s2n_map_test.c b/tests/unit/s2n_map_test.c index a09adae6771..2c83a59c875 100644 --- a/tests/unit/s2n_map_test.c +++ b/tests/unit/s2n_map_test.c @@ -258,20 +258,20 @@ int main(int argc, char **argv) *blobs[i].data = i; } - struct s2n_map *map = s2n_map_new(); - EXPECT_NOT_NULL(map); + struct s2n_map *test_map = s2n_map_new(); + EXPECT_NOT_NULL(test_map); /* set values in map to 0 and 1 */ - map->table[0].value = blobs[0]; - map->table[0].key = blobs[2]; - map->table[map->capacity - 1].value = blobs[1]; - map->table[map->capacity - 1].key = blobs[3]; + test_map->table[0].value = blobs[0]; + test_map->table[0].key = blobs[2]; + test_map->table[test_map->capacity - 1].value = blobs[1]; + test_map->table[test_map->capacity - 1].key = blobs[3]; - map->size = 2; - EXPECT_OK(s2n_map_complete(map)); + test_map->size = 2; + EXPECT_OK(s2n_map_complete(test_map)); DEFER_CLEANUP(struct s2n_map_iterator *iter = NULL, s2n_map_iterator_free_pointer); - EXPECT_OK(s2n_map_iterator_new(map, &iter)); + EXPECT_OK(s2n_map_iterator_new(test_map, &iter)); bool seen[2] = { 0 }; bool has_next = false; @@ -287,7 +287,7 @@ int main(int argc, char **argv) /* assert that 0 and 1 were both seen */ EXPECT_TRUE(seen[0] && seen[1]); - EXPECT_OK(s2n_map_free(map)); + EXPECT_OK(s2n_map_free(test_map)); }; }; diff --git a/utils/s2n_map_internal.h b/utils/s2n_map_internal.h index 97ba1d7253b..fb6c6130657 100644 --- a/utils/s2n_map_internal.h +++ b/utils/s2n_map_internal.h @@ -32,11 +32,11 @@ struct s2n_map { int immutable; /* Pointer to the hash-table, should be capacity * sizeof(struct s2n_map_entry) */ - struct s2n_map_entry* table; + struct s2n_map_entry *table; }; struct s2n_map_iterator { - struct s2n_map* map; + struct s2n_map *map; /* Index of the entry to be returned on the next `s2n_map_iterator_next()` call. */ uint32_t current_index; }; From 4d8866511186d72a3b6420251d9414cfd5ba34dc Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Thu, 25 Jan 2024 23:22:45 +0000 Subject: [PATCH 03/12] address pr feedback - use S2N_CLEANUP_RESULT - use init rather than new --- tests/unit/s2n_map_test.c | 32 ++++++++++++++++---------------- utils/s2n_map.c | 20 +------------------- utils/s2n_map.h | 4 ++-- utils/s2n_map_internal.h | 2 +- 4 files changed, 20 insertions(+), 38 deletions(-) diff --git a/tests/unit/s2n_map_test.c b/tests/unit/s2n_map_test.c index 2c83a59c875..d7834455d1b 100644 --- a/tests/unit/s2n_map_test.c +++ b/tests/unit/s2n_map_test.c @@ -192,23 +192,23 @@ int main(int argc, char **argv) EXPECT_NOT_NULL(map); /* fail to initialize an iterator on a mutable map */ { - DEFER_CLEANUP(struct s2n_map_iterator *iter = NULL, s2n_map_iterator_free_pointer); - EXPECT_ERROR(s2n_map_iterator_new(map, &iter)); + struct s2n_map_iterator iter = { 0 }; + EXPECT_ERROR(s2n_map_iterator_init(&iter, map)); }; EXPECT_OK(s2n_map_complete(map)); /* has next is false on an empty map, and next returns an error */ { - DEFER_CLEANUP(struct s2n_map_iterator *iter = NULL, s2n_map_iterator_free_pointer); - EXPECT_OK(s2n_map_iterator_new(map, &iter)); + struct s2n_map_iterator iter = { 0 }; + EXPECT_OK(s2n_map_iterator_init(&iter, map)); bool has_next = false; - EXPECT_OK(s2n_map_iterator_has_next(iter, &has_next)); + EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); EXPECT_FALSE(has_next); struct s2n_blob value = { 0 }; - EXPECT_ERROR(s2n_map_iterator_next(iter, &value)); + EXPECT_ERROR(s2n_map_iterator_next(&iter, &value)); }; EXPECT_OK(s2n_map_unlock(map)); @@ -223,24 +223,24 @@ int main(int argc, char **argv) { bool seen[10] = { 0 }; - DEFER_CLEANUP(struct s2n_map_iterator *iter = NULL, s2n_map_iterator_free_pointer); - EXPECT_OK(s2n_map_iterator_new(map, &iter)); + struct s2n_map_iterator iter = { 0 }; + EXPECT_OK(s2n_map_iterator_init(&iter, map)); bool has_next = false; struct s2n_blob value = { 0 }; for (size_t i = 0; i < 10; i++) { - EXPECT_OK(s2n_map_iterator_has_next(iter, &has_next)); + EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); EXPECT_TRUE(has_next); - EXPECT_OK(s2n_map_iterator_next(iter, &value)); + EXPECT_OK(s2n_map_iterator_next(&iter, &value)); seen[*value.data] = true; } /* all elements have been iterated over */ - EXPECT_OK(s2n_map_iterator_has_next(iter, &has_next)); + EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); EXPECT_FALSE(has_next); - EXPECT_ERROR(s2n_map_iterator_next(iter, &value)); + EXPECT_ERROR(s2n_map_iterator_next(&iter, &value)); /* all elements were seen */ for (size_t i = 0; i < 10; i++) { @@ -270,17 +270,17 @@ int main(int argc, char **argv) test_map->size = 2; EXPECT_OK(s2n_map_complete(test_map)); - DEFER_CLEANUP(struct s2n_map_iterator *iter = NULL, s2n_map_iterator_free_pointer); - EXPECT_OK(s2n_map_iterator_new(test_map, &iter)); + struct s2n_map_iterator iter = { 0 }; + EXPECT_OK(s2n_map_iterator_init(&iter, test_map)); bool seen[2] = { 0 }; bool has_next = false; struct s2n_blob value = { 0 }; for (size_t i = 0; i < 2; i++) { - EXPECT_OK(s2n_map_iterator_has_next(iter, &has_next)); + EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); EXPECT_TRUE(has_next); - EXPECT_OK(s2n_map_iterator_next(iter, &value)); + EXPECT_OK(s2n_map_iterator_next(&iter, &value)); seen[*value.data] = true; } diff --git a/utils/s2n_map.c b/utils/s2n_map.c index b5171d94681..a13e6466915 100644 --- a/utils/s2n_map.c +++ b/utils/s2n_map.c @@ -273,17 +273,11 @@ S2N_RESULT s2n_map_iterator_advance(struct s2n_map_iterator *iter) return S2N_RESULT_OK; } -S2N_RESULT s2n_map_iterator_new(struct s2n_map *map, struct s2n_map_iterator **iter_out) +S2N_RESULT s2n_map_iterator_init(struct s2n_map_iterator *iter, const struct s2n_map *map) { RESULT_ENSURE_REF(map); RESULT_ENSURE(map->immutable, S2N_ERR_MAP_MUTABLE); - struct s2n_blob mem = { 0 }; - struct s2n_map_iterator *iter; - - RESULT_GUARD_POSIX(s2n_alloc(&mem, sizeof(struct s2n_map_iterator))); - - iter = (void *) mem.data; iter->map = map; iter->current_index = 0; @@ -294,8 +288,6 @@ S2N_RESULT s2n_map_iterator_new(struct s2n_map *map, struct s2n_map_iterator **i RESULT_GUARD(s2n_map_iterator_advance(iter)); } - *iter_out = iter; - return S2N_RESULT_OK; } @@ -324,13 +316,3 @@ S2N_RESULT s2n_map_iterator_has_next(const struct s2n_map_iterator *iter, bool * return S2N_RESULT_OK; } - -int s2n_map_iterator_free(struct s2n_map_iterator *iter) -{ - if (iter == NULL) { - return S2N_SUCCESS; - } - POSIX_GUARD(s2n_free_object((uint8_t **) &iter, sizeof(struct s2n_map_iterator))); - - return S2N_SUCCESS; -} diff --git a/utils/s2n_map.h b/utils/s2n_map.h index c49b95a3676..25e5a2469b6 100644 --- a/utils/s2n_map.h +++ b/utils/s2n_map.h @@ -33,7 +33,7 @@ S2N_RESULT s2n_map_lookup(const struct s2n_map *map, struct s2n_blob *key, struc S2N_RESULT s2n_map_free(struct s2n_map *map); S2N_RESULT s2n_map_size(struct s2n_map *map, uint32_t *size); -S2N_RESULT s2n_map_iterator_new(struct s2n_map *map, struct s2n_map_iterator **iter); +S2N_RESULT s2n_map_iterator_init(struct s2n_map_iterator *iter, const struct s2n_map *map); S2N_RESULT s2n_map_iterator_next(struct s2n_map_iterator *iter, struct s2n_blob *value); S2N_RESULT s2n_map_iterator_has_next(const struct s2n_map_iterator *iter, bool *has_next); -int s2n_map_iterator_free(struct s2n_map_iterator *iter); +S2N_CLEANUP_RESULT s2n_map_iterator_free(struct s2n_map_iterator *iter); diff --git a/utils/s2n_map_internal.h b/utils/s2n_map_internal.h index fb6c6130657..91c320c2629 100644 --- a/utils/s2n_map_internal.h +++ b/utils/s2n_map_internal.h @@ -36,7 +36,7 @@ struct s2n_map { }; struct s2n_map_iterator { - struct s2n_map *map; + const struct s2n_map *map; /* Index of the entry to be returned on the next `s2n_map_iterator_next()` call. */ uint32_t current_index; }; From 80dcca5e5203ba86667eeadfb176114007145d4b Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Sat, 27 Jan 2024 04:05:12 +0000 Subject: [PATCH 04/12] make s2n_map_iterator visible This is necessary to allow for us to construct an s2n_map_iterator on the stack. --- utils/s2n_map.h | 6 +++++- utils/s2n_map_internal.h | 6 ------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/utils/s2n_map.h b/utils/s2n_map.h index 25e5a2469b6..9d4bc69047b 100644 --- a/utils/s2n_map.h +++ b/utils/s2n_map.h @@ -21,7 +21,11 @@ #include "utils/s2n_result.h" struct s2n_map; -struct s2n_map_iterator; +struct s2n_map_iterator { + const struct s2n_map *map; + /* Index of the entry to be returned on the next `s2n_map_iterator_next()` call. */ + uint32_t current_index; +}; struct s2n_map *s2n_map_new(); struct s2n_map *s2n_map_new_with_initial_capacity(uint32_t capacity); diff --git a/utils/s2n_map_internal.h b/utils/s2n_map_internal.h index 91c320c2629..01d629fb16f 100644 --- a/utils/s2n_map_internal.h +++ b/utils/s2n_map_internal.h @@ -34,9 +34,3 @@ struct s2n_map { /* Pointer to the hash-table, should be capacity * sizeof(struct s2n_map_entry) */ struct s2n_map_entry *table; }; - -struct s2n_map_iterator { - const struct s2n_map *map; - /* Index of the entry to be returned on the next `s2n_map_iterator_next()` call. */ - uint32_t current_index; -}; From 177358a162c16d589b087917fec957daee077162 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 30 Jan 2024 00:55:33 +0000 Subject: [PATCH 05/12] address pr feedback - remove pre-increment usage - add explicit flag to track consumption - remove unused header - minimize diff in test file --- tests/unit/s2n_map_iterator_test.c | 133 +++++++++++ tests/unit/s2n_map_test.c | 349 ++++++++++------------------- utils/s2n_map.c | 28 ++- utils/s2n_map.h | 2 +- 4 files changed, 270 insertions(+), 242 deletions(-) create mode 100644 tests/unit/s2n_map_iterator_test.c diff --git a/tests/unit/s2n_map_iterator_test.c b/tests/unit/s2n_map_iterator_test.c new file mode 100644 index 00000000000..b61bf10407b --- /dev/null +++ b/tests/unit/s2n_map_iterator_test.c @@ -0,0 +1,133 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +#include "api/s2n.h" +#include "s2n_test.h" +#include "utils/s2n_map.h" +#include "utils/s2n_map_internal.h" + +DEFINE_POINTER_CLEANUP_FUNC(struct s2n_map_iterator *, s2n_map_iterator_free); + +int main(int argc, char **argv) +{ + BEGIN_TEST(); + + /* s2n_map_iterator iteration test */ + { + struct s2n_map *map = s2n_map_new(); + EXPECT_NOT_NULL(map); + /* fail to initialize an iterator on a mutable map */ + { + struct s2n_map_iterator iter = { 0 }; + EXPECT_ERROR(s2n_map_iterator_init(&iter, map)); + }; + + EXPECT_OK(s2n_map_complete(map)); + + /* has next is false on an empty map, and next returns an error */ + { + struct s2n_map_iterator iter = { 0 }; + EXPECT_OK(s2n_map_iterator_init(&iter, map)); + + bool has_next = false; + EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); + EXPECT_FALSE(has_next); + + struct s2n_blob value = { 0 }; + EXPECT_ERROR(s2n_map_iterator_next(&iter, &value)); + }; + + EXPECT_OK(s2n_map_unlock(map)); + for (uint8_t i = 0; i < 10; i++) { + struct s2n_blob key = { .size = 1, .data = &i }; + struct s2n_blob val = { .size = 1, .data = &i }; + EXPECT_OK(s2n_map_put(map, &key, &val)); + } + EXPECT_OK(s2n_map_complete(map)); + + /* iterator goes over all elements */ + { + bool seen[10] = { 0 }; + + struct s2n_map_iterator iter = { 0 }; + EXPECT_OK(s2n_map_iterator_init(&iter, map)); + + bool has_next = false; + struct s2n_blob value = { 0 }; + for (size_t i = 0; i < 10; i++) { + EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); + EXPECT_TRUE(has_next); + + EXPECT_OK(s2n_map_iterator_next(&iter, &value)); + seen[*value.data] = true; + } + + /* all elements have been iterated over */ + EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); + EXPECT_FALSE(has_next); + + EXPECT_ERROR(s2n_map_iterator_next(&iter, &value)); + + /* all elements were seen */ + for (size_t i = 0; i < 10; i++) { + EXPECT_TRUE(seen[i]); + } + } + + EXPECT_OK(s2n_map_free(map)); + }; + + /* test first and last slots in table */ + { + struct s2n_blob blobs[4] = { 0 }; + for (uint8_t i = 0; i < 4; i++) { + s2n_alloc(&blobs[i], 1); + *blobs[i].data = i; + } + + struct s2n_map *test_map = s2n_map_new(); + EXPECT_NOT_NULL(test_map); + + /* set values in map to 0 and 1 */ + test_map->table[0].value = blobs[0]; + test_map->table[0].key = blobs[2]; + test_map->table[test_map->capacity - 1].value = blobs[1]; + test_map->table[test_map->capacity - 1].key = blobs[3]; + + test_map->size = 2; + EXPECT_OK(s2n_map_complete(test_map)); + + struct s2n_map_iterator iter = { 0 }; + EXPECT_OK(s2n_map_iterator_init(&iter, test_map)); + bool seen[2] = { 0 }; + + bool has_next = false; + struct s2n_blob value = { 0 }; + for (size_t i = 0; i < 2; i++) { + EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); + EXPECT_TRUE(has_next); + + EXPECT_OK(s2n_map_iterator_next(&iter, &value)); + seen[*value.data] = true; + } + + /* assert that 0 and 1 were both seen */ + EXPECT_TRUE(seen[0] && seen[1]); + + EXPECT_OK(s2n_map_free(test_map)); + }; + + END_TEST(); +} diff --git a/tests/unit/s2n_map_test.c b/tests/unit/s2n_map_test.c index d7834455d1b..73ef694ef4e 100644 --- a/tests/unit/s2n_map_test.c +++ b/tests/unit/s2n_map_test.c @@ -19,114 +19,77 @@ #include "api/s2n.h" #include "s2n_test.h" -#include "utils/s2n_map_internal.h" DEFINE_POINTER_CLEANUP_FUNC(struct s2n_map_iterator *, s2n_map_iterator_free); int main(int argc, char **argv) { - /* s2n_map test */ - { - char keystr[sizeof("ffff")]; - char valstr[sizeof("16384")]; - uint32_t size; - struct s2n_map *empty, *map; - struct s2n_blob key = { 0 }; - struct s2n_blob val = { 0 }; - bool key_found; - - BEGIN_TEST(); - EXPECT_SUCCESS(s2n_disable_tls13_in_test()); - - EXPECT_NOT_NULL(empty = s2n_map_new()); - EXPECT_OK(s2n_map_size(empty, &size)); - EXPECT_EQUAL(size, 0); - - /* Try a lookup on an empty map. Expect an error because the map is still mutable. */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 1234)); + char keystr[sizeof("ffff")]; + char valstr[sizeof("16384")]; + uint32_t size; + struct s2n_map *empty, *map; + struct s2n_blob key = { 0 }; + struct s2n_blob val = { 0 }; + bool key_found; + + BEGIN_TEST(); + EXPECT_SUCCESS(s2n_disable_tls13_in_test()); + + EXPECT_NOT_NULL(empty = s2n_map_new()); + EXPECT_OK(s2n_map_size(empty, &size)); + EXPECT_EQUAL(size, 0); + + /* Try a lookup on an empty map. Expect an error because the map is still mutable. */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 1234)); + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; + EXPECT_ERROR(s2n_map_lookup(empty, &key, &val, &key_found)); + + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 1234)); + val.data = (void *) valstr; + val.size = strlen(valstr) + 1; + + /* Try to add/put key with zero-size data. Expect failures */ + key.size = 0; + EXPECT_ERROR(s2n_map_add(empty, &key, &val)); + EXPECT_ERROR(s2n_map_put(empty, &key, &val)); + key.size = strlen(keystr) + 1; + + /* Make the empty map complete */ + EXPECT_OK(s2n_map_complete(empty)); + + /* Lookup and expect no result */ + EXPECT_OK(s2n_map_lookup(empty, &key, &val, &key_found)); + EXPECT_EQUAL(key_found, false); + + /* Done with the empty map */ + EXPECT_OK(s2n_map_free(empty)); + + /* Expect failure since initial map size is zero */ + EXPECT_NULL(map = s2n_map_new_with_initial_capacity(0)); + + /* Create map with the smallest initial size */ + EXPECT_NOT_NULL(map = s2n_map_new_with_initial_capacity(1)); + + /* Insert 8k key value pairs of the form hex(i) -> dec(i) */ + for (int i = 0; i < 8192; i++) { + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); + key.data = (void *) keystr; key.size = strlen(keystr) + 1; - EXPECT_ERROR(s2n_map_lookup(empty, &key, &val, &key_found)); - - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 1234)); val.data = (void *) valstr; val.size = strlen(valstr) + 1; - /* Try to add/put key with zero-size data. Expect failures */ - key.size = 0; - EXPECT_ERROR(s2n_map_add(empty, &key, &val)); - EXPECT_ERROR(s2n_map_put(empty, &key, &val)); - key.size = strlen(keystr) + 1; - - /* Make the empty map complete */ - EXPECT_OK(s2n_map_complete(empty)); - - /* Lookup and expect no result */ - EXPECT_OK(s2n_map_lookup(empty, &key, &val, &key_found)); - EXPECT_EQUAL(key_found, false); - - /* Done with the empty map */ - EXPECT_OK(s2n_map_free(empty)); - - /* Expect failure since initial map size is zero */ - EXPECT_NULL(map = s2n_map_new_with_initial_capacity(0)); - - /* Create map with the smallest initial size */ - EXPECT_NOT_NULL(map = s2n_map_new_with_initial_capacity(1)); - - /* Insert 8k key value pairs of the form hex(i) -> dec(i) */ - for (int i = 0; i < 8192; i++) { - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); - - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; - val.data = (void *) valstr; - val.size = strlen(valstr) + 1; - - EXPECT_OK(s2n_map_add(map, &key, &val)); - } - EXPECT_OK(s2n_map_size(map, &size)); - EXPECT_EQUAL(size, 8192); - - /* Try adding some duplicates */ - for (int i = 0; i < 10; i++) { - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); - - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; - val.data = (void *) valstr; - val.size = strlen(valstr) + 1; - - EXPECT_ERROR(s2n_map_add(map, &key, &val)); - } - EXPECT_OK(s2n_map_size(map, &size)); - EXPECT_EQUAL(size, 8192); - - /* Try replacing some entries */ - for (int i = 0; i < 10; i++) { - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i + 1)); - - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; - val.data = (void *) valstr; - val.size = strlen(valstr) + 1; - - EXPECT_OK(s2n_map_put(map, &key, &val)); - } - - /* Try a lookup before the map is complete: should fail */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 1)); - EXPECT_ERROR(s2n_map_lookup(map, &key, &val, &key_found)); - - /* Make the map complete */ - EXPECT_OK(s2n_map_complete(map)); + EXPECT_OK(s2n_map_add(map, &key, &val)); + } + EXPECT_OK(s2n_map_size(map, &size)); + EXPECT_EQUAL(size, 8192); - /* Make sure that add-after-complete fails */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 8193)); + /* Try adding some duplicates */ + for (int i = 0; i < 10; i++) { + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); key.data = (void *) keystr; key.size = strlen(keystr) + 1; @@ -134,162 +97,90 @@ int main(int argc, char **argv) val.size = strlen(valstr) + 1; EXPECT_ERROR(s2n_map_add(map, &key, &val)); + } + EXPECT_OK(s2n_map_size(map, &size)); + EXPECT_EQUAL(size, 8192); - /* Check for equivalence */ - for (int i = 0; i < 8192; i++) { - if (i >= 10) { - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); - } else { - /* The first 10 entries were overwritten with i+1 */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i + 1)); - } - - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; - - EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); - EXPECT_EQUAL(key_found, true); - - EXPECT_SUCCESS(memcmp(val.data, valstr, strlen(valstr) + 1)); - } - - /* Check for a key that shouldn't be there */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); - key.data = (void *) keystr; - key.size = strlen(keystr) + 1; - EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); - EXPECT_EQUAL(key_found, false); - - /* Make the map mutable */ - EXPECT_OK(s2n_map_unlock(map)); - /* Make sure that add-after-unlock succeeds */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 8193)); + /* Try replacing some entries */ + for (int i = 0; i < 10; i++) { + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i + 1)); key.data = (void *) keystr; key.size = strlen(keystr) + 1; val.data = (void *) valstr; val.size = strlen(valstr) + 1; - EXPECT_OK(s2n_map_add(map, &key, &val)); - - /* Complete the map again */ - EXPECT_OK(s2n_map_complete(map)); - - /* Check the element added after map unlock */ - EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); - EXPECT_EQUAL(key_found, true); - EXPECT_SUCCESS(memcmp(val.data, valstr, strlen(valstr) + 1)); - - EXPECT_OK(s2n_map_free(map)); - }; - - /* s2n_map_iterator test */ - { - struct s2n_map *map = s2n_map_new(); - EXPECT_NOT_NULL(map); - /* fail to initialize an iterator on a mutable map */ - { - struct s2n_map_iterator iter = { 0 }; - EXPECT_ERROR(s2n_map_iterator_init(&iter, map)); - }; - - EXPECT_OK(s2n_map_complete(map)); - - /* has next is false on an empty map, and next returns an error */ - { - struct s2n_map_iterator iter = { 0 }; - EXPECT_OK(s2n_map_iterator_init(&iter, map)); - - bool has_next = false; - EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); - EXPECT_FALSE(has_next); - - struct s2n_blob value = { 0 }; - EXPECT_ERROR(s2n_map_iterator_next(&iter, &value)); - }; - - EXPECT_OK(s2n_map_unlock(map)); - for (uint8_t i = 0; i < 10; i++) { - struct s2n_blob key = { .size = 1, .data = &i }; - struct s2n_blob val = { .size = 1, .data = &i }; - EXPECT_OK(s2n_map_put(map, &key, &val)); - } - EXPECT_OK(s2n_map_complete(map)); + EXPECT_OK(s2n_map_put(map, &key, &val)); + } - /* iterator goes over all elements */ - { - bool seen[10] = { 0 }; + /* Try a lookup before the map is complete: should fail */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 1)); + EXPECT_ERROR(s2n_map_lookup(map, &key, &val, &key_found)); - struct s2n_map_iterator iter = { 0 }; - EXPECT_OK(s2n_map_iterator_init(&iter, map)); + /* Make the map complete */ + EXPECT_OK(s2n_map_complete(map)); - bool has_next = false; - struct s2n_blob value = { 0 }; - for (size_t i = 0; i < 10; i++) { - EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); - EXPECT_TRUE(has_next); + /* Make sure that add-after-complete fails */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 8193)); - EXPECT_OK(s2n_map_iterator_next(&iter, &value)); - seen[*value.data] = true; - } + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; + val.data = (void *) valstr; + val.size = strlen(valstr) + 1; - /* all elements have been iterated over */ - EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); - EXPECT_FALSE(has_next); + EXPECT_ERROR(s2n_map_add(map, &key, &val)); - EXPECT_ERROR(s2n_map_iterator_next(&iter, &value)); - - /* all elements were seen */ - for (size_t i = 0; i < 10; i++) { - EXPECT_TRUE(seen[i]); - } + /* Check for equivalence */ + for (int i = 0; i < 8192; i++) { + if (i >= 10) { + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); + } else { + /* The first 10 entries were overwritten with i+1 */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i + 1)); } - EXPECT_OK(s2n_map_free(map)); + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; - /* test first and last slots in table */ - { - struct s2n_blob blobs[4] = { 0 }; - for (uint8_t i = 0; i < 4; i++) { - s2n_alloc(&blobs[i], 1); - *blobs[i].data = i; - } + EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); + EXPECT_EQUAL(key_found, true); - struct s2n_map *test_map = s2n_map_new(); - EXPECT_NOT_NULL(test_map); + EXPECT_SUCCESS(memcmp(val.data, valstr, strlen(valstr) + 1)); + } - /* set values in map to 0 and 1 */ - test_map->table[0].value = blobs[0]; - test_map->table[0].key = blobs[2]; - test_map->table[test_map->capacity - 1].value = blobs[1]; - test_map->table[test_map->capacity - 1].key = blobs[3]; + /* Check for a key that shouldn't be there */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; + EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); + EXPECT_EQUAL(key_found, false); - test_map->size = 2; - EXPECT_OK(s2n_map_complete(test_map)); + /* Make the map mutable */ + EXPECT_OK(s2n_map_unlock(map)); + /* Make sure that add-after-unlock succeeds */ + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 8193)); - struct s2n_map_iterator iter = { 0 }; - EXPECT_OK(s2n_map_iterator_init(&iter, test_map)); - bool seen[2] = { 0 }; + key.data = (void *) keystr; + key.size = strlen(keystr) + 1; + val.data = (void *) valstr; + val.size = strlen(valstr) + 1; - bool has_next = false; - struct s2n_blob value = { 0 }; - for (size_t i = 0; i < 2; i++) { - EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); - EXPECT_TRUE(has_next); + EXPECT_OK(s2n_map_add(map, &key, &val)); - EXPECT_OK(s2n_map_iterator_next(&iter, &value)); - seen[*value.data] = true; - } + /* Complete the map again */ + EXPECT_OK(s2n_map_complete(map)); - /* assert that 0 and 1 were both seen */ - EXPECT_TRUE(seen[0] && seen[1]); + /* Check the element added after map unlock */ + EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); + EXPECT_EQUAL(key_found, true); + EXPECT_SUCCESS(memcmp(val.data, valstr, strlen(valstr) + 1)); - EXPECT_OK(s2n_map_free(test_map)); - }; - }; + EXPECT_OK(s2n_map_free(map)); END_TEST(); } diff --git a/utils/s2n_map.c b/utils/s2n_map.c index a13e6466915..2cf8b4c4a2c 100644 --- a/utils/s2n_map.c +++ b/utils/s2n_map.c @@ -209,8 +209,8 @@ S2N_RESULT s2n_map_lookup(const struct s2n_map *map, struct s2n_blob *key, struc } /* We found a match */ - value->data = map->table[slot].value.data; - value->size = map->table[slot].value.size; + struct s2n_blob entry_value = map->table[slot].value; + s2n_blob_init(value, entry_value.data, entry_value.size); *key_found = true; @@ -255,26 +255,31 @@ S2N_RESULT s2n_map_size(struct s2n_map *map, uint32_t *size) } /* Update the internal state so that `current_index` will point to the next value - * in the table or will equal `map->capacity` if there are no more elements in - * the map. + * in the table or set iter->consumed equal to true if there are no more elements + * in the map. */ S2N_RESULT s2n_map_iterator_advance(struct s2n_map_iterator *iter) { RESULT_ENSURE_REF(iter); RESULT_ENSURE_REF(iter->map); + RESULT_ENSURE(!iter->consumed, S2N_ERR_SAFETY); - while (++iter->current_index < iter->map->capacity) { + iter->current_index++; + while (iter->current_index < iter->map->capacity) { /* a value was found in the map */ if (iter->map->table[iter->current_index].key.size) { return S2N_RESULT_OK; } + iter->current_index++; } /* no more values were found in the map */ + iter->consumed = true; return S2N_RESULT_OK; } S2N_RESULT s2n_map_iterator_init(struct s2n_map_iterator *iter, const struct s2n_map *map) { + RESULT_ENSURE_REF(iter); RESULT_ENSURE_REF(map); RESULT_ENSURE(map->immutable, S2N_ERR_MAP_MUTABLE); @@ -295,13 +300,11 @@ S2N_RESULT s2n_map_iterator_next(struct s2n_map_iterator *iter, struct s2n_blob { RESULT_ENSURE_REF(iter); RESULT_ENSURE(iter->map->immutable, S2N_ERR_MAP_MUTABLE); + RESULT_ENSURE(!iter->consumed, S2N_ERR_SAFETY); + RESULT_ENSURE(value->allocated == 0, S2N_ERR_SAFETY); - bool has_next = false; - RESULT_GUARD(s2n_map_iterator_has_next(iter, &has_next)); - RESULT_ENSURE(has_next, S2N_ERR_SAFETY); - - value->data = iter->map->table[iter->current_index].value.data; - value->size = iter->map->table[iter->current_index].value.size; + struct s2n_blob entry_value = iter->map->table[iter->current_index].value; + s2n_blob_init(value, entry_value.data, entry_value.size); RESULT_GUARD(s2n_map_iterator_advance(iter)); @@ -312,7 +315,8 @@ S2N_RESULT s2n_map_iterator_has_next(const struct s2n_map_iterator *iter, bool * { RESULT_ENSURE_REF(iter); RESULT_ENSURE_REF(iter->map); - *has_next = iter->current_index < iter->map->capacity; + + *has_next = !iter->consumed; return S2N_RESULT_OK; } diff --git a/utils/s2n_map.h b/utils/s2n_map.h index 9d4bc69047b..2e4d02e381a 100644 --- a/utils/s2n_map.h +++ b/utils/s2n_map.h @@ -25,6 +25,7 @@ struct s2n_map_iterator { const struct s2n_map *map; /* Index of the entry to be returned on the next `s2n_map_iterator_next()` call. */ uint32_t current_index; + bool consumed; }; struct s2n_map *s2n_map_new(); @@ -40,4 +41,3 @@ S2N_RESULT s2n_map_size(struct s2n_map *map, uint32_t *size); S2N_RESULT s2n_map_iterator_init(struct s2n_map_iterator *iter, const struct s2n_map *map); S2N_RESULT s2n_map_iterator_next(struct s2n_map_iterator *iter, struct s2n_blob *value); S2N_RESULT s2n_map_iterator_has_next(const struct s2n_map_iterator *iter, bool *has_next); -S2N_CLEANUP_RESULT s2n_map_iterator_free(struct s2n_map_iterator *iter); From 6cf6f347901889772e64448279589a8da6e83970 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 30 Jan 2024 21:28:27 +0000 Subject: [PATCH 06/12] address pr feedback - add missed return assertions - switch to expect-error-with-errno - has_next returns bool - define test constants --- tests/unit/s2n_map_iterator_test.c | 46 ++++++++++++++++-------------- tests/unit/s2n_map_test.c | 22 +++++++------- utils/s2n_map.c | 17 ++++++----- utils/s2n_map.h | 2 +- 4 files changed, 46 insertions(+), 41 deletions(-) diff --git a/tests/unit/s2n_map_iterator_test.c b/tests/unit/s2n_map_iterator_test.c index b61bf10407b..b8a4a5b94e6 100644 --- a/tests/unit/s2n_map_iterator_test.c +++ b/tests/unit/s2n_map_iterator_test.c @@ -18,6 +18,8 @@ #include "utils/s2n_map.h" #include "utils/s2n_map_internal.h" +#define TEST_VALUE_COUNT 10 + DEFINE_POINTER_CLEANUP_FUNC(struct s2n_map_iterator *, s2n_map_iterator_free); int main(int argc, char **argv) @@ -31,26 +33,32 @@ int main(int argc, char **argv) /* fail to initialize an iterator on a mutable map */ { struct s2n_map_iterator iter = { 0 }; - EXPECT_ERROR(s2n_map_iterator_init(&iter, map)); + EXPECT_ERROR_WITH_ERRNO(s2n_map_iterator_init(&iter, map), S2N_ERR_MAP_MUTABLE); }; EXPECT_OK(s2n_map_complete(map)); + /* next returns an error when the blob is null */ + { + struct s2n_map_iterator iter = { 0 }; + EXPECT_OK(s2n_map_iterator_init(&iter, map)); + + EXPECT_ERROR_WITH_ERRNO(s2n_map_iterator_next(&iter, NULL), S2N_ERR_SAFETY); + } + /* has next is false on an empty map, and next returns an error */ { struct s2n_map_iterator iter = { 0 }; EXPECT_OK(s2n_map_iterator_init(&iter, map)); - bool has_next = false; - EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); - EXPECT_FALSE(has_next); + EXPECT_FALSE(s2n_map_iterator_has_next(&iter)); struct s2n_blob value = { 0 }; - EXPECT_ERROR(s2n_map_iterator_next(&iter, &value)); + EXPECT_ERROR_WITH_ERRNO(s2n_map_iterator_next(&iter, &value), S2N_ERR_SAFETY); }; EXPECT_OK(s2n_map_unlock(map)); - for (uint8_t i = 0; i < 10; i++) { + for (uint8_t i = 0; i < TEST_VALUE_COUNT; i++) { struct s2n_blob key = { .size = 1, .data = &i }; struct s2n_blob val = { .size = 1, .data = &i }; EXPECT_OK(s2n_map_put(map, &key, &val)); @@ -59,40 +67,37 @@ int main(int argc, char **argv) /* iterator goes over all elements */ { - bool seen[10] = { 0 }; + bool seen[TEST_VALUE_COUNT] = { 0 }; struct s2n_map_iterator iter = { 0 }; EXPECT_OK(s2n_map_iterator_init(&iter, map)); - bool has_next = false; struct s2n_blob value = { 0 }; - for (size_t i = 0; i < 10; i++) { - EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); - EXPECT_TRUE(has_next); + for (size_t i = 0; i < TEST_VALUE_COUNT; i++) { + EXPECT_TRUE(s2n_map_iterator_has_next(&iter)); EXPECT_OK(s2n_map_iterator_next(&iter, &value)); seen[*value.data] = true; } /* all elements have been iterated over */ - EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); - EXPECT_FALSE(has_next); - - EXPECT_ERROR(s2n_map_iterator_next(&iter, &value)); + EXPECT_FALSE(s2n_map_iterator_has_next(&iter)); + EXPECT_ERROR_WITH_ERRNO(s2n_map_iterator_next(&iter, &value), S2N_ERR_SAFETY); /* all elements were seen */ - for (size_t i = 0; i < 10; i++) { + for (size_t i = 0; i < TEST_VALUE_COUNT; i++) { EXPECT_TRUE(seen[i]); } - } + }; EXPECT_OK(s2n_map_free(map)); }; /* test first and last slots in table */ { - struct s2n_blob blobs[4] = { 0 }; - for (uint8_t i = 0; i < 4; i++) { + /* 2 (first and last slot) * 2 (key and value) */ + struct s2n_blob blobs[2 * 2] = { 0 }; + for (uint8_t i = 0; i < (2 * 2); i++) { s2n_alloc(&blobs[i], 1); *blobs[i].data = i; } @@ -116,8 +121,7 @@ int main(int argc, char **argv) bool has_next = false; struct s2n_blob value = { 0 }; for (size_t i = 0; i < 2; i++) { - EXPECT_OK(s2n_map_iterator_has_next(&iter, &has_next)); - EXPECT_TRUE(has_next); + EXPECT_TRUE(s2n_map_iterator_has_next(&iter)); EXPECT_OK(s2n_map_iterator_next(&iter, &value)); seen[*value.data] = true; diff --git a/tests/unit/s2n_map_test.c b/tests/unit/s2n_map_test.c index 73ef694ef4e..e480c4bbd47 100644 --- a/tests/unit/s2n_map_test.c +++ b/tests/unit/s2n_map_test.c @@ -20,13 +20,15 @@ #include "api/s2n.h" #include "s2n_test.h" +#define TEST_VALUE_COUNT 8192 + DEFINE_POINTER_CLEANUP_FUNC(struct s2n_map_iterator *, s2n_map_iterator_free); int main(int argc, char **argv) { char keystr[sizeof("ffff")]; char valstr[sizeof("16384")]; - uint32_t size; + uint32_t size = 0; struct s2n_map *empty, *map; struct s2n_blob key = { 0 }; struct s2n_blob val = { 0 }; @@ -72,7 +74,7 @@ int main(int argc, char **argv) EXPECT_NOT_NULL(map = s2n_map_new_with_initial_capacity(1)); /* Insert 8k key value pairs of the form hex(i) -> dec(i) */ - for (int i = 0; i < 8192; i++) { + for (int i = 0; i < TEST_VALUE_COUNT; i++) { EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); @@ -84,7 +86,7 @@ int main(int argc, char **argv) EXPECT_OK(s2n_map_add(map, &key, &val)); } EXPECT_OK(s2n_map_size(map, &size)); - EXPECT_EQUAL(size, 8192); + EXPECT_EQUAL(size, TEST_VALUE_COUNT); /* Try adding some duplicates */ for (int i = 0; i < 10; i++) { @@ -99,7 +101,7 @@ int main(int argc, char **argv) EXPECT_ERROR(s2n_map_add(map, &key, &val)); } EXPECT_OK(s2n_map_size(map, &size)); - EXPECT_EQUAL(size, 8192); + EXPECT_EQUAL(size, TEST_VALUE_COUNT); /* Try replacing some entries */ for (int i = 0; i < 10; i++) { @@ -122,8 +124,8 @@ int main(int argc, char **argv) EXPECT_OK(s2n_map_complete(map)); /* Make sure that add-after-complete fails */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 8193)); + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", TEST_VALUE_COUNT + 1)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", TEST_VALUE_COUNT + 1)); key.data = (void *) keystr; key.size = strlen(keystr) + 1; @@ -133,7 +135,7 @@ int main(int argc, char **argv) EXPECT_ERROR(s2n_map_add(map, &key, &val)); /* Check for equivalence */ - for (int i = 0; i < 8192; i++) { + for (int i = 0; i < TEST_VALUE_COUNT; i++) { if (i >= 10) { EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", i)); EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", i)); @@ -153,7 +155,7 @@ int main(int argc, char **argv) } /* Check for a key that shouldn't be there */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", TEST_VALUE_COUNT + 1)); key.data = (void *) keystr; key.size = strlen(keystr) + 1; EXPECT_OK(s2n_map_lookup(map, &key, &val, &key_found)); @@ -162,8 +164,8 @@ int main(int argc, char **argv) /* Make the map mutable */ EXPECT_OK(s2n_map_unlock(map)); /* Make sure that add-after-unlock succeeds */ - EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", 8193)); - EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", 8193)); + EXPECT_SUCCESS(snprintf(keystr, sizeof(keystr), "%04x", TEST_VALUE_COUNT + 1)); + EXPECT_SUCCESS(snprintf(valstr, sizeof(valstr), "%05d", TEST_VALUE_COUNT + 1)); key.data = (void *) keystr; key.size = strlen(keystr) + 1; diff --git a/utils/s2n_map.c b/utils/s2n_map.c index 2cf8b4c4a2c..1078b948664 100644 --- a/utils/s2n_map.c +++ b/utils/s2n_map.c @@ -210,7 +210,7 @@ S2N_RESULT s2n_map_lookup(const struct s2n_map *map, struct s2n_blob *key, struc /* We found a match */ struct s2n_blob entry_value = map->table[slot].value; - s2n_blob_init(value, entry_value.data, entry_value.size); + RESULT_GUARD_POSIX(s2n_blob_init(value, entry_value.data, entry_value.size)); *key_found = true; @@ -301,22 +301,21 @@ S2N_RESULT s2n_map_iterator_next(struct s2n_map_iterator *iter, struct s2n_blob RESULT_ENSURE_REF(iter); RESULT_ENSURE(iter->map->immutable, S2N_ERR_MAP_MUTABLE); RESULT_ENSURE(!iter->consumed, S2N_ERR_SAFETY); - RESULT_ENSURE(value->allocated == 0, S2N_ERR_SAFETY); + RESULT_ENSURE(iter->current_index < iter->map->capacity, S2N_ERR_SAFETY); struct s2n_blob entry_value = iter->map->table[iter->current_index].value; - s2n_blob_init(value, entry_value.data, entry_value.size); + RESULT_GUARD_POSIX(s2n_blob_init(value, entry_value.data, entry_value.size)); RESULT_GUARD(s2n_map_iterator_advance(iter)); return S2N_RESULT_OK; } -S2N_RESULT s2n_map_iterator_has_next(const struct s2n_map_iterator *iter, bool *has_next) +bool s2n_map_iterator_has_next(const struct s2n_map_iterator *iter) { - RESULT_ENSURE_REF(iter); - RESULT_ENSURE_REF(iter->map); - - *has_next = !iter->consumed; + if (!iter) { + return false; + } - return S2N_RESULT_OK; + return !iter->consumed; } diff --git a/utils/s2n_map.h b/utils/s2n_map.h index 2e4d02e381a..eaabe4d5825 100644 --- a/utils/s2n_map.h +++ b/utils/s2n_map.h @@ -40,4 +40,4 @@ S2N_RESULT s2n_map_size(struct s2n_map *map, uint32_t *size); S2N_RESULT s2n_map_iterator_init(struct s2n_map_iterator *iter, const struct s2n_map *map); S2N_RESULT s2n_map_iterator_next(struct s2n_map_iterator *iter, struct s2n_blob *value); -S2N_RESULT s2n_map_iterator_has_next(const struct s2n_map_iterator *iter, bool *has_next); +bool s2n_map_iterator_has_next(const struct s2n_map_iterator *iter); From 97f5cab04420c09118db9e5ccc7f131d0d778196 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Wed, 31 Jan 2024 00:21:34 +0000 Subject: [PATCH 07/12] address ci failures - remove unused variables - remove unused functions --- tests/unit/s2n_map_iterator_test.c | 3 --- tests/unit/s2n_map_test.c | 2 -- 2 files changed, 5 deletions(-) diff --git a/tests/unit/s2n_map_iterator_test.c b/tests/unit/s2n_map_iterator_test.c index b8a4a5b94e6..bccfd8cb38f 100644 --- a/tests/unit/s2n_map_iterator_test.c +++ b/tests/unit/s2n_map_iterator_test.c @@ -20,8 +20,6 @@ #define TEST_VALUE_COUNT 10 -DEFINE_POINTER_CLEANUP_FUNC(struct s2n_map_iterator *, s2n_map_iterator_free); - int main(int argc, char **argv) { BEGIN_TEST(); @@ -118,7 +116,6 @@ int main(int argc, char **argv) EXPECT_OK(s2n_map_iterator_init(&iter, test_map)); bool seen[2] = { 0 }; - bool has_next = false; struct s2n_blob value = { 0 }; for (size_t i = 0; i < 2; i++) { EXPECT_TRUE(s2n_map_iterator_has_next(&iter)); diff --git a/tests/unit/s2n_map_test.c b/tests/unit/s2n_map_test.c index e480c4bbd47..870f3ab42ff 100644 --- a/tests/unit/s2n_map_test.c +++ b/tests/unit/s2n_map_test.c @@ -22,8 +22,6 @@ #define TEST_VALUE_COUNT 8192 -DEFINE_POINTER_CLEANUP_FUNC(struct s2n_map_iterator *, s2n_map_iterator_free); - int main(int argc, char **argv) { char keystr[sizeof("ffff")]; From 5c58ae3ce2098a7dc11e4e2c67ec3207e74ae20f Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Thu, 1 Feb 2024 07:29:54 +0000 Subject: [PATCH 08/12] address pr feedback - switch error message for no more next - add additional ensure ref - call has_next rather than checking iterator - fix error in test case --- tests/unit/s2n_map_iterator_test.c | 20 ++++++++++---------- utils/s2n_map.c | 11 ++++------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/tests/unit/s2n_map_iterator_test.c b/tests/unit/s2n_map_iterator_test.c index bccfd8cb38f..ecae228450c 100644 --- a/tests/unit/s2n_map_iterator_test.c +++ b/tests/unit/s2n_map_iterator_test.c @@ -36,14 +36,6 @@ int main(int argc, char **argv) EXPECT_OK(s2n_map_complete(map)); - /* next returns an error when the blob is null */ - { - struct s2n_map_iterator iter = { 0 }; - EXPECT_OK(s2n_map_iterator_init(&iter, map)); - - EXPECT_ERROR_WITH_ERRNO(s2n_map_iterator_next(&iter, NULL), S2N_ERR_SAFETY); - } - /* has next is false on an empty map, and next returns an error */ { struct s2n_map_iterator iter = { 0 }; @@ -52,7 +44,7 @@ int main(int argc, char **argv) EXPECT_FALSE(s2n_map_iterator_has_next(&iter)); struct s2n_blob value = { 0 }; - EXPECT_ERROR_WITH_ERRNO(s2n_map_iterator_next(&iter, &value), S2N_ERR_SAFETY); + EXPECT_ERROR_WITH_ERRNO(s2n_map_iterator_next(&iter, &value), S2N_ERR_ARRAY_INDEX_OOB); }; EXPECT_OK(s2n_map_unlock(map)); @@ -80,7 +72,7 @@ int main(int argc, char **argv) /* all elements have been iterated over */ EXPECT_FALSE(s2n_map_iterator_has_next(&iter)); - EXPECT_ERROR_WITH_ERRNO(s2n_map_iterator_next(&iter, &value), S2N_ERR_SAFETY); + EXPECT_ERROR_WITH_ERRNO(s2n_map_iterator_next(&iter, &value), S2N_ERR_ARRAY_INDEX_OOB); /* all elements were seen */ for (size_t i = 0; i < TEST_VALUE_COUNT; i++) { @@ -88,6 +80,14 @@ int main(int argc, char **argv) } }; + /* next returns an error when the blob is null */ + { + struct s2n_map_iterator iter = { 0 }; + EXPECT_OK(s2n_map_iterator_init(&iter, map)); + + EXPECT_ERROR_WITH_ERRNO(s2n_map_iterator_next(&iter, NULL), S2N_ERR_NULL); + } + EXPECT_OK(s2n_map_free(map)); }; diff --git a/utils/s2n_map.c b/utils/s2n_map.c index 1078b948664..544d22f0cb7 100644 --- a/utils/s2n_map.c +++ b/utils/s2n_map.c @@ -299,10 +299,11 @@ S2N_RESULT s2n_map_iterator_init(struct s2n_map_iterator *iter, const struct s2n S2N_RESULT s2n_map_iterator_next(struct s2n_map_iterator *iter, struct s2n_blob *value) { RESULT_ENSURE_REF(iter); + RESULT_ENSURE_REF(iter->map); RESULT_ENSURE(iter->map->immutable, S2N_ERR_MAP_MUTABLE); - RESULT_ENSURE(!iter->consumed, S2N_ERR_SAFETY); + RESULT_ENSURE(s2n_map_iterator_has_next(iter), S2N_ERR_ARRAY_INDEX_OOB); - RESULT_ENSURE(iter->current_index < iter->map->capacity, S2N_ERR_SAFETY); + RESULT_ENSURE(iter->current_index < iter->map->capacity, S2N_ERR_ARRAY_INDEX_OOB); struct s2n_blob entry_value = iter->map->table[iter->current_index].value; RESULT_GUARD_POSIX(s2n_blob_init(value, entry_value.data, entry_value.size)); @@ -313,9 +314,5 @@ S2N_RESULT s2n_map_iterator_next(struct s2n_map_iterator *iter, struct s2n_blob bool s2n_map_iterator_has_next(const struct s2n_map_iterator *iter) { - if (!iter) { - return false; - } - - return !iter->consumed; + return iter && !iter->consumed; } From effa38210d16ba3a8f9a6cd10618bda398fa33bb Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Thu, 1 Feb 2024 07:37:23 +0000 Subject: [PATCH 09/12] address pr feedback - missed the removal of a direct iterator check --- utils/s2n_map.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/s2n_map.c b/utils/s2n_map.c index 544d22f0cb7..b1ad6721c09 100644 --- a/utils/s2n_map.c +++ b/utils/s2n_map.c @@ -262,7 +262,7 @@ S2N_RESULT s2n_map_iterator_advance(struct s2n_map_iterator *iter) { RESULT_ENSURE_REF(iter); RESULT_ENSURE_REF(iter->map); - RESULT_ENSURE(!iter->consumed, S2N_ERR_SAFETY); + RESULT_ENSURE(s2n_map_iterator_has_next(iter), S2N_ERR_ARRAY_INDEX_OOB); iter->current_index++; while (iter->current_index < iter->map->capacity) { From b285c7ec2cc9100ac6f5f5a3e5fbcb994d2a366b Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Thu, 1 Feb 2024 19:15:27 +0000 Subject: [PATCH 10/12] address ci failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - some incredibly kind and thoughtful person added return-value-checks to our CI ☺️ --- tests/unit/s2n_map_iterator_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/s2n_map_iterator_test.c b/tests/unit/s2n_map_iterator_test.c index ecae228450c..dff3080d2d1 100644 --- a/tests/unit/s2n_map_iterator_test.c +++ b/tests/unit/s2n_map_iterator_test.c @@ -96,7 +96,7 @@ int main(int argc, char **argv) /* 2 (first and last slot) * 2 (key and value) */ struct s2n_blob blobs[2 * 2] = { 0 }; for (uint8_t i = 0; i < (2 * 2); i++) { - s2n_alloc(&blobs[i], 1); + EXPECT_SUCCESS(s2n_alloc(&blobs[i], 1)); *blobs[i].data = i; } From f4c2bdb7509f23042701e9f748dda4f40cfcf325 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Fri, 2 Feb 2024 18:06:43 +0000 Subject: [PATCH 11/12] non-semantic change to force codebuild rebuild --- codebuild/spec/buildspec_asan.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/codebuild/spec/buildspec_asan.yml b/codebuild/spec/buildspec_asan.yml index d6ea0e4cb0e..7bda851268d 100644 --- a/codebuild/spec/buildspec_asan.yml +++ b/codebuild/spec/buildspec_asan.yml @@ -47,6 +47,7 @@ batch: variables: S2N_LIBCRYPTO: openssl-1.0.2 + phases: build: on-failure: ABORT From 14bf39e3e5ebc8ca2eed41ca08a1358371b16758 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Fri, 2 Feb 2024 19:04:01 +0000 Subject: [PATCH 12/12] Revert "non-semantic change to force codebuild rebuild" This reverts commit f4c2bdb7509f23042701e9f748dda4f40cfcf325. --- codebuild/spec/buildspec_asan.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/codebuild/spec/buildspec_asan.yml b/codebuild/spec/buildspec_asan.yml index 7bda851268d..d6ea0e4cb0e 100644 --- a/codebuild/spec/buildspec_asan.yml +++ b/codebuild/spec/buildspec_asan.yml @@ -47,7 +47,6 @@ batch: variables: S2N_LIBCRYPTO: openssl-1.0.2 - phases: build: on-failure: ABORT