From 321284dec6f4316c18ec643ecdf3a3c791714d0d Mon Sep 17 00:00:00 2001 From: DcruBro Date: Sat, 22 Aug 2026 17:28:04 +0200 Subject: [PATCH] set.h: memory safety fixes, added set_take()/set_take_at() --- set.h | 254 +++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 200 insertions(+), 54 deletions(-) diff --git a/set.h b/set.h index 3fcbcfb..72c2499 100644 --- a/set.h +++ b/set.h @@ -3,9 +3,9 @@ set.h - Sets for C Version: 1.0.0 Description: -Header-only library for sets in C. Generally memory safe (when you -use it properly) but not thread-safe (you must implement your own -synchronization mechanisms) - but your own usage may not be. +Header-only library for sets in C. Memory safe against the mistakes it can +detect, but not thread-safe (you must implement your own synchronization +mechanisms). Designed for C23 onwards, but should be compatible with older standards. A set is backed by the same kind of contiguous array as a vector, but it holds @@ -15,20 +15,21 @@ described further down. Guarantees: - Every element in the set is unique with respect to the set's equality rule. set_insert() reports 1 instead of adding a second copy. - - Shallow copies are safe, as long as you stop using the original set after - copying it. A shallow copy still points to the same data, so if you free the - original set, the copy will point to freed memory. If you want to make a - deep copy, use set_deep_copy(). Moves are preferred here with set_move() - which will transfer ownership of the data to the new set. - - Deep copies may be made with set_deep_copy(), which will copy the data as - well as the set itself. This is slower than a shallow copy, but safer. - Deep copies are only available for sets without an element destructor, as - a byte-wise copy of owning elements would result in a double free. + - A set_t must never be duplicated by copying the struct. Two set_t values + sharing one data pointer both believe they own it, and whichever is destroyed + or moved from first leaves the other holding freed memory. Copying the pointer + (set_t* b = a;) is fine - that is one set with two names, and it must be + destroyed exactly once. + - To hand a set's data to another set, use set_move(). The source is emptied, + freed and its pointer set to NULL, so there is only ever one owner. + - To get a second, independent set with the same contents, use set_deep_copy(), + which copies the data as well as the set itself. Deep copies are only + available for sets without an element destructor, as a byte-wise copy of + owning elements would result in a double free. - No gaps in the raw data array. - - When a set is resized, the data is reallocated to a new memory location. - This means that any pointers to the old data will be invalidated. If you want - to keep using the old data, you should make a deep copy of the set before - resizing it. + - When a set is resized, the data is reallocated to a new memory location, so + any pointers into the old data are invalidated. There is no way to preserve + them; re-fetch with set_get_const() after the resize. - Order of elements is NOT preserved. A set is unordered, and removing an element moves the last element into the slot that was freed, so an element's index is only stable until the next removal. Indices exist to let you walk @@ -40,6 +41,9 @@ Guarantees: set_insert() rejects such a pointer with -1 instead, as a byte-wise copy would leave two slots owning the same memory. To duplicate an owning element, copy what it owns yourself and insert that. + - set_take() and set_take_at() transfer ownership of the element to the caller, + and do not call the destructor on it. It is the caller's responsibility to + free whatever the element owns. Element equality: A set decides whether two elements are the same with its comparator, set through @@ -61,7 +65,15 @@ and other plain scalar types, but it is wrong more often than it looks: Set the comparator before adding any elements. Setting it on a set that already holds elements is allowed, but a comparator that is looser than the one in effect before it may leave elements behind that it now considers equal. Call -set_dedupe() afterwards to restore uniqueness. +set_dedupe() afterwards to restore uniqueness. Until you do, the set is not +unique under its own rule, and set_is_equal() may report two sets as equal when +they are not. + +Two sets may only be combined or compared when they use the same comparator, +which is decided by comparing the function pointers themselves. Give your +comparator external linkage. A comparator declared static in a shared header +gets a distinct address in every translation unit, so sets built in different +translation units will silently refuse to combine. Element destructors: A set may be given an element destructor with set_set_destructor(). It is @@ -69,6 +81,11 @@ called for every element that leaves the set, i.e. by set_remove(), set_remove_at(), set_dedupe() (on the duplicates it drops), set_clear() and set_destroy(). +The destructor is NOT called by set_take() or set_take_at(), as these functions +transfer ownership of the element to the caller. It is the caller's +responsibility to free whatever the element owns. They are the only way to get +an owning element back out of a set, as the set has no mutable element access. + The destructor receives a pointer to the element's slot inside the set's data array - NOT a pointer that was returned by malloc(). It must free whatever the element owns, and must never free the pointer it was handed, as that memory @@ -83,6 +100,8 @@ belongs to the set. For a set of char* this means: set_t* set = set_create(sizeof(char*)); set_set_comparator(set, cmp_str); // Compare the strings, not the pointers set_set_destructor(set, free_str); + // ... use the set ... + set_destroy(&set); // Takes the address of your pointer, and NULLs it Differences from vector.h: The functions that only make sense on an ordered, duplicate-tolerant container @@ -90,7 +109,8 @@ are deliberately absent: - There is no mutable element access (no set_get(), no mutable array view). Writing to an element in place could turn it into a copy of another element, which the set has no way to notice. Change an element by removing it and - inserting the new value. + inserting the new value. To get an owning element back out of the set rather + than destroying it, use set_take() or set_take_at(). - There is no front(), back() or pop_back(), as an unordered container has no meaningful ends. - There is no insertion or assignment by index, as a position in the array is @@ -146,6 +166,13 @@ SOFTWARE. typedef void (*set_destructor_t)(void* element); // Function pointer type for element destructor typedef int (*set_comparator_t)(const void* a, const void* b); // Function pointer type for element comparator, returns 0 when equal +/* + A set. Do not construct, copy or modify this struct directly - the fields are + visible only because the functions below are inline. Create sets with + set_create() and use the accessors. A set_t that did not come from set_create() + is not a valid set, and the functions will reject it where they can and + misbehave where they cannot. +*/ typedef struct { size_t size; // Number of elements in the set size_t capacity; // Allocated capacity of the set @@ -156,18 +183,23 @@ typedef struct { } set_t; /* Forward declarations */ +static inline int set_is_aliased(const set_t* set, const void* ptr); +static inline int set_elements_equal(const set_t* set, const void* a, const void* b); static inline set_t* set_create(size_t element_size); static inline int set_set_destructor(set_t* set, set_destructor_t destructor); static inline set_destructor_t set_get_destructor(const set_t* set); static inline int set_set_comparator(set_t* set, set_comparator_t comparator); static inline set_comparator_t set_get_comparator(const set_t* set); static inline int set_reserve(set_t* set, size_t new_capacity); +static inline int set_grow(set_t* set); static inline int set_prune(set_t* set); static inline int set_insert(set_t* set, const void* element); static inline size_t set_find(const set_t* set, const void* element); static inline int set_contains(const set_t* set, const void* element); static inline int set_remove(set_t* set, const void* element); static inline int set_remove_at(set_t* set, size_t index); +static inline int set_take(set_t* set, const void* element, void* out); +static inline int set_take_at(set_t* set, size_t index, void* out); static inline int set_dedupe(set_t* set); static inline int set_clear(set_t* set); static inline int set_is_empty(const set_t* set); @@ -178,12 +210,13 @@ static inline size_t set_element_size(const set_t* set); static inline const void* set_as_c_array(const set_t* set); static inline int set_move(set_t* dest, set_t** src); static inline set_t* set_deep_copy(const set_t* set); +static inline int set_is_compatible(const set_t* a, const set_t* b); static inline set_t* set_union(const set_t* a, const set_t* b); static inline set_t* set_intersection(const set_t* a, const set_t* b); static inline set_t* set_difference(const set_t* a, const set_t* b); static inline int set_is_subset(const set_t* a, const set_t* b); static inline int set_is_equal(const set_t* a, const set_t* b); -static inline int set_destroy(set_t* set); +static inline int set_destroy(set_t** set); /* @brief Checks whether a pointer points inside the set's own data array. Used internally to make the write functions safe against self-referential input. @@ -191,6 +224,7 @@ static inline int set_destroy(set_t* set); @param ptr The pointer to check. @return 1 if the pointer lies within the set's allocated buffer, 0 otherwise. @attention This is an internal helper. You are not expected to call it directly, but it is harmless if you do. + @attention This is a best-effort check, not a portable guarantee. Comparing pointers into different objects is not defined by the standard, and going through uintptr_t is the usual practical workaround rather than a strictly correct one - the conversion is implementation-defined and uintptr_t is an optional type. It does the right thing on every mainstream platform. */ static inline int set_is_aliased(const set_t* set, const void* ptr) { if (!set || !ptr || !set->data) { @@ -198,7 +232,7 @@ static inline int set_is_aliased(const set_t* set, const void* ptr) { } // Compared as integers rather than pointers, as comparing pointers into - // different objects is not well defined. + // different objects is not well defined. See the note above. uintptr_t base = (uintptr_t)set->data; uintptr_t end = base + (set->capacity * set->element_size); uintptr_t target = (uintptr_t)ptr; @@ -339,19 +373,24 @@ static inline set_comparator_t set_get_comparator(const set_t* set) { } /* - @brief Reserves space for the specified capacity in the set. + @brief Ensures the set has room for at least the specified number of elements. @param set A pointer to the set for which to reserve space. - @param new_capacity The new capacity of the set. - @return 0 on success, -1 if the set is NULL or allocation fails. - @attention You cannot reserve space for zero or below the current size (doing so will return -1). + @param new_capacity The minimum capacity the set should have. + @return 0 on success, -1 if the set is NULL, its element size is zero, or allocation fails. + @attention Capacity never decreases. If the set already has room, this is a no-op and reports success. + @attention If the buffer does grow, it is reallocated, so any pointers into the set's data are invalidated. Use set_prune() to release unused memory. */ static inline int set_reserve(set_t* set, size_t new_capacity) { if (!set) { return -1; // Invalid set } - if (new_capacity == 0 || new_capacity < set->size) { - return -1; // Cannot reserve space for zero or below current size + if (set->element_size == 0) { + return -1; // Not a usable set; also guards the division below + } + + if (new_capacity <= set->capacity) { + return 0; // Already have the room. Never shrink } if (new_capacity > SIZE_MAX / set->element_size) { @@ -369,11 +408,30 @@ static inline int set_reserve(set_t* set, size_t new_capacity) { return 0; // Success } +/* + @brief Grows the set's capacity to make room for at least one more element. Used internally by the functions that add elements. + @param set A pointer to the set to grow. + @return 0 on success, -1 if the set is NULL or allocation fails. + @attention This is an internal helper. You are not expected to call it directly. +*/ +static inline int set_grow(set_t* set) { + if (!set) { + return -1; // Invalid set + } + + if (set->capacity > SIZE_MAX / 2) { + return -1; // Prevent overflow + } + + size_t new_capacity = set->capacity > 0 ? set->capacity * 2 : 1; // Double the capacity, or set to 1 if it was 0 + return set_reserve(set, new_capacity); +} + /* @brief Prunes the set to free unused memory. If the set's size is less than its capacity, this function will reallocate the set's data array to match its size, freeing any unused memory. @param set A pointer to the set to be pruned. @return 0 on success, -1 if the set is NULL or allocation fails. - @attention After calling this function, the set's capacity will be equal to its size. Any pointers to the old data will be invalidated. If you want to keep using the old data, you should make a deep copy of the set before pruning it. + @attention After calling this function, the set's capacity will be equal to its size. Any pointers into the set's data are invalidated; re-fetch them with set_get_const() afterwards. @attention Capacity may never drop below 1, even if the set is empty. */ static inline int set_prune(set_t* set) { @@ -381,6 +439,10 @@ static inline int set_prune(set_t* set) { return -1; // Invalid set } + if (set->element_size == 0) { + return -1; // Not a usable set; also guards the division below + } + if (set->size < set->capacity) { size_t new_capacity = set->size > 0 ? set->size : 1; // Ensure capacity is at least 1 @@ -441,7 +503,7 @@ static inline int set_contains(const set_t* set, const void* element) { @return 0 if the element was added, 1 if an equal element was already in the set and nothing changed, -1 if the set is NULL, the element is NULL, if the element is an owning element that is aliased in the set and a destructor is set, or if reservation fails. @attention The element must be a pointer to a valid memory location containing data of the same type as the set's element type. The set will make a copy of the data, so the original element can be modified or freed after this function returns. @attention The element is placed at an unspecified position. Do not assume it lands at the end, and do not assume it stays where it lands. - @attention The element may point into the set's own data array on a set without a destructor, in which case it is by definition already in the set and this returns 1 without touching anything. + @attention The element may point into the set's own data array on a set without a destructor. An element-aligned pointer, i.e. one that set_get_const() returned, is by definition already in the set, so a consistent comparator makes this return 1 without touching anything. A pointer partway into an element, or a comparator that does not report an element as equal to itself, reaches the copy instead; the value is followed through any reallocation and copied with memmove(), so it survives either way. @attention Take note of the return value. A return of 1 means your value was NOT stored, and if it owns memory, you are still responsible for freeing it. */ static inline int set_insert(set_t* set, const void* element) { @@ -468,9 +530,8 @@ static inline int set_insert(set_t* set, const void* element) { offset = (size_t)((const char*)element - (const char*)set->data); } - // Reserve space for the set to double its current capacity - if (set_reserve(set, set->capacity * 2) != 0) { - return -1; // Reserve failed + if (set_grow(set) != 0) { + return -1; // Failed to grow the set } if (aliased) { @@ -478,8 +539,10 @@ static inline int set_insert(set_t* set, const void* element) { } } - // Copy the new element into the set's data array - memcpy((char*)set->data + (set->size * set->element_size), element, set->element_size); + // Copy the new element into the set's data array. memmove, as an interior + // pointer into our own data slips past the duplicate check above and may + // overlap the destination slot + memmove((char*)set->data + (set->size * set->element_size), element, set->element_size); set->size++; return 0; // Success @@ -490,7 +553,7 @@ static inline int set_insert(set_t* set, const void* element) { @param set A pointer to the set from which the element will be removed. @param index The index of the element to be removed. @return 0 on success, -1 if the set is NULL or the index is out of bounds. - @attention After calling this function, the set's size will be reduced by one. The memory occupied by the removed element will not be freed automatically unless a destructor is set. + @attention After calling this function, the set's size will be reduced by one. The memory occupied by the removed element will not be freed automatically unless a destructor is set. Use set_take_at() to remove the element at the specified index and NOT call the destructor on it (hands ownership to the caller). @attention The last element is moved into the freed slot, so it is O(1), but the index of that last element changes. If you are removing elements while walking the set by index, do not advance the index after a removal, or you will skip the element that was moved in. */ static inline int set_remove_at(set_t* set, size_t index) { @@ -519,7 +582,7 @@ static inline int set_remove_at(set_t* set, size_t index) { @param set A pointer to the set from which the element will be removed. @param element A pointer to the element to be removed. It is compared against the set's elements with the set's equality rule. @return 0 if the element was removed, 1 if it was not in the set and nothing changed, -1 if the set is NULL or the element is NULL. - @attention After a successful call, the set's size will be reduced by one. The memory occupied by the removed element will not be freed automatically unless a destructor is set. + @attention After a successful call, the set's size will be reduced by one. The memory occupied by the removed element will not be freed automatically unless a destructor is set. Use set_take() to remove the element and NOT call the destructor on it (hands ownership to the caller). @attention The last element is moved into the freed slot, so any index you were holding onto may now refer to a different element. @attention The element may point into the set's own data array, i.e. one returned by set_get_const(). Its index is resolved before anything is destroyed, so the pointer cannot be left dangling underneath this function. */ @@ -538,6 +601,69 @@ static inline int set_remove(set_t* set, const void* element) { return set_remove_at(set, index); } +/* + @brief Removes the element at the specified index and transfers ownership of it to the caller. + @param set A pointer to the set from which the element will be taken. + @param index The index of the element to take. + @param out A pointer to a buffer of at least set_element_size(set) bytes, which the element is copied into. + @return 0 on success, -1 if the set is NULL, out is NULL, out points into the set's own data, or the index is out of bounds. + @attention The element destructor is deliberately NOT called. Whatever the element owns becomes the caller's responsibility to free. + @attention out must not point into the set's own data array. Doing so would leave two slots owning the same memory and is rejected with -1. + @attention As with set_remove_at(), the last element is moved into the freed slot, so the index of that last element changes. + @attention This and set_take() are the only way to get an owning element out of a set with its contents intact, as the set has no mutable element access. +*/ +static inline int set_take_at(set_t* set, size_t index, void* out) { + if (!set || index >= set->size || !out) { + return -1; // Invalid set, index out of bounds, or output pointer is NULL + } + + if (set_is_aliased(set, out)) { + return -1; // Refuse to take an element into a pointer that lives inside the set + } + + void* slot = (char*)set->data + (index * set->element_size); + memcpy(out, slot, set->element_size); // Hand the element over, destructor deliberately not called + + // Move the last element into the freed slot, as the order is not guaranteed + size_t last = set->size - 1; + if (index != last) { + // memcpy, as the two slots cannot overlap when they are not the same slot + memcpy(slot, (const char*)set->data + (last * set->element_size), set->element_size); + } + + set->size--; + return 0; // Success +} + +/* + @brief Removes an element from the set and transfers ownership of it to the caller. + @param set A pointer to the set from which the element will be taken. + @param element A pointer to the element to look for. It is compared against the set's elements with the set's equality rule. + @param out A pointer to a buffer of at least set_element_size(set) bytes, which the element is copied into. + @return 0 if the element was taken, 1 if it was not in the set and nothing changed, -1 if the set is NULL, the element is NULL, out is NULL, or out points into the set's own data. + @attention The element destructor is deliberately NOT called. Whatever the element owns becomes the caller's responsibility to free. + @attention The element may point into the set's own data array, i.e. one returned by set_get_const(). Its index is resolved before anything is copied, so the pointer cannot be left dangling underneath this function. + @attention out must not point into the set's own data array. Doing so would leave two slots owning the same memory and is rejected with -1. +*/ +static inline int set_take(set_t* set, const void* element, void* out) { + if (!set || !element || !out) { + return -1; // Invalid set, element, or output pointer + } + + if (set_is_aliased(set, out)) { + return -1; // Refuse to take an element into a pointer that lives inside the set + } + + // Resolve the index first, exactly as set_remove() does, so that a pointer + // into our own data cannot be invalidated before we are done with it + size_t index = set_find(set, element); + if (index == SET_NPOS) { + return 1; // Not in the set, nothing to do + } + + return set_take_at(set, index, out); +} + /* @brief Removes duplicate elements from the set, restoring uniqueness under the set's current equality rule. @param set A pointer to the set to be deduplicated. @@ -684,10 +810,11 @@ static inline const void* set_as_c_array(const set_t* set) { @brief Moves a set to another set, transferring ownership of the data. The destination set will take ownership of the source set's data, its element size, its destructor and its comparator. @param dest A pointer to the destination set. @param src A pointer to the pointer holding the source set. It will be set to NULL. - @return 0 on success, -1 if the destination set is NULL, the source pointer is NULL, or the source set is NULL. + @return 0 on success, -1 if the destination set is NULL, the source pointer is NULL, the source set is NULL, or both sets share the same data pointer. @attention After calling this function, the source set will be freed (excluding data) and the source pointer will be set to NULL, so it cannot be used again. @attention The destination set's existing elements will be destroyed (using the destination's own destructor, if it has one) and its data freed. Ensure that you do not need the existing data before calling this function. @attention Moving a set onto itself is a no-op and reports success, leaving the set untouched. + @attention Two sets can only share a data pointer if a set_t was duplicated by copying the struct, which is never valid. The move is refused rather than freeing a buffer the source still points at. */ static inline int set_move(set_t* dest, set_t** src) { if (!dest || !src || !*src) { @@ -698,6 +825,12 @@ static inline int set_move(set_t* dest, set_t** src) { return 0; // Moving to itself, no action needed } + if (dest->data == (*src)->data) { + return -1; // Refuse to move a set onto another set that shares its data + // Realistically, this is unreachable via legal use, but... safety. + // Or something. You WILL trigger this if you do shallow copies. DO NOT! + } + // Destroy the destination set's existing elements with its own destructor, // then free its data, as it is about to be replaced set_clear(dest); @@ -751,7 +884,7 @@ static inline set_t* set_deep_copy(const set_t* set) { int r = set_reserve(new_set, set->capacity); if (r != 0) { - set_destroy(new_set); + set_destroy(&new_set); return NULL; // Allocation failed } @@ -815,7 +948,7 @@ static inline set_t* set_union(const set_t* a, const set_t* b) { for (size_t i = 0; i < a->size; ++i) { if (set_insert(result, (const char*)a->data + (i * a->element_size)) < 0) { - set_destroy(result); + set_destroy(&result); return NULL; // Insertion failed } } @@ -823,7 +956,7 @@ static inline set_t* set_union(const set_t* a, const set_t* b) { for (size_t i = 0; i < b->size; ++i) { // Elements of b that are already in a report 1, which is not a failure if (set_insert(result, (const char*)b->data + (i * b->element_size)) < 0) { - set_destroy(result); + set_destroy(&result); return NULL; // Insertion failed } } @@ -859,7 +992,7 @@ static inline set_t* set_intersection(const set_t* a, const set_t* b) { } if (set_insert(result, element) < 0) { - set_destroy(result); + set_destroy(&result); return NULL; // Insertion failed } } @@ -896,7 +1029,7 @@ static inline set_t* set_difference(const set_t* a, const set_t* b) { } if (set_insert(result, element) < 0) { - set_destroy(result); + set_destroy(&result); return NULL; // Insertion failed } } @@ -949,31 +1082,44 @@ static inline int set_is_equal(const set_t* a, const set_t* b) { return 0; // Different number of elements, so they cannot hold the same ones } - // Both sets are unique and of equal size, so containment one way is enough - return set_is_subset(a, b); + // Containment is checked both ways. One way plus equal sizes would be enough + // for two sets that are each unique under the shared comparator, but a + // comparator loosened on a populated set leaves duplicates behind until + // set_dedupe() is called, and {"A", "a"} would then compare equal to + // {"A", "B"} under a case-insensitive rule + return set_is_subset(a, b) && set_is_subset(b, a); } /* @brief Destroys the set and frees its memory. - @param set A pointer to the set to be destroyed. - @return 0 on success, -1 if the set is NULL. - @attention After calling this function, the set pointer should not be used again. Accessing it after destruction will lead to undefined behavior. + @param set A pointer to the pointer holding the set to be destroyed. It will be set to NULL. + @return 0 on success, or if the set was already NULL. -1 only if the pointer itself is NULL. + @attention After calling this function, the set pointer should not be used again (It will be set to NULL). Accessing it after destruction will lead to undefined behavior. + @attention Only the pointer you pass in is set to NULL. Copies of that pointer held elsewhere are left dangling and must not be used. @attention If stored elements own memory of their own, set a destructor with set_set_destructor() to have it cleaned up here. Otherwise, the set will only free the memory allocated for the data array and the set structure itself, but not any dynamically allocated memory within the elements. */ -static inline int set_destroy(set_t* set) { +static inline int set_destroy(set_t** set) { if (!set) { - return -1; // Invalid set + return -1; // Invalid pointer } - if (set->destructor) { - for (size_t i = 0; i < set->size; ++i) { - void* element = (char*)set->data + (i * set->element_size); - set->destructor(element); // Call the destructor for each element + if (!(*set)) { + return 0; // Already NULL, nothing to destroy + } + + set_t* target = *set; + + if (target->destructor) { + for (size_t i = 0; i < target->size; ++i) { + void* element = (char*)target->data + (i * target->element_size); + target->destructor(element); // Call the destructor for each element } } - free(set->data); - free(set); + free(target->data); + free(target); + *set = NULL; // Set the pointer to NULL to avoid dangling references + return 0; // Success }