From 69ca0345ecd8ebd16d3cc7a2b6db30478704db2f Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Fri, 9 Jan 2026 15:40:19 -0800 Subject: [PATCH 1/2] Add a builtin buffer to writable buffers Signed-off-by: Rob Johnson --- include/splinterdb/splinterdb.h | 2 +- src/data_internal.h | 4 +- src/platform_linux/platform_heap.h | 1 + src/platform_linux/platform_log.c | 12 +++ src/platform_linux/platform_log.h | 12 +-- src/util.c | 23 +++--- src/util.h | 112 ++++++++++++++------------- tests/functional/filter_test.c | 4 +- tests/unit/misc_test.c | 1 + tests/unit/writable_buffer_test.c | 117 ++++++++--------------------- 10 files changed, 125 insertions(+), 163 deletions(-) diff --git a/include/splinterdb/splinterdb.h b/include/splinterdb/splinterdb.h index 4060bae9..f8206591 100644 --- a/include/splinterdb/splinterdb.h +++ b/include/splinterdb/splinterdb.h @@ -197,7 +197,7 @@ splinterdb_update(const splinterdb *kvsb, slice key, slice delta); // Lookups // Size of opaque data required to hold a lookup result -#define SPLINTERDB_LOOKUP_BUFSIZE (6 * sizeof(void *)) +#define SPLINTERDB_LOOKUP_BUFSIZE (10 * sizeof(void *)) // A lookup result is stored and parsed from here // diff --git a/src/data_internal.h b/src/data_internal.h index 740e744f..4e73ba9e 100644 --- a/src/data_internal.h +++ b/src/data_internal.h @@ -128,7 +128,6 @@ key_copy_contents(void *dst, key k) typedef struct { key_type kind; writable_buffer wb; - char default_buffer[DEFAULT_KEY_BUFFER_SIZE]; } key_buffer; /* @@ -151,8 +150,7 @@ static inline void key_buffer_init(key_buffer *kb, platform_heap_id hid) { kb->kind = USER_KEY; - writable_buffer_init_with_buffer( - &kb->wb, hid, sizeof(kb->default_buffer), kb->default_buffer, 0); + writable_buffer_init(&kb->wb, hid); } static inline platform_status diff --git a/src/platform_linux/platform_heap.h b/src/platform_linux/platform_heap.h index 3c39256b..061be9d2 100644 --- a/src/platform_linux/platform_heap.h +++ b/src/platform_linux/platform_heap.h @@ -7,6 +7,7 @@ #include "platform_status.h" #include "platform_util.h" #include "platform_machine.h" +#include "platform_log.h" #include "shmem.h" #include #include diff --git a/src/platform_linux/platform_log.c b/src/platform_linux/platform_log.c index 7df73609..0337afb1 100644 --- a/src/platform_linux/platform_log.c +++ b/src/platform_linux/platform_log.c @@ -3,6 +3,7 @@ #include "platform_log.h" #include "platform_assert.h" +#include "platform_heap.h" // By default, platform_default_log() messages are sent to /dev/null // and platform_error_log() messages go to stderr (see below). @@ -39,3 +40,14 @@ platform_get_stdout_stream(void) { return Platform_default_log_handle; } + +void +platform_close_log_stream(platform_stream_handle *stream, + platform_log_handle *log_handle) +{ + fclose(stream->stream); + fputs(stream->str, log_handle); + fflush(log_handle); + platform_free_from_heap( + NULL, stream->str, "stream", __func__, __FILE__, __LINE__); +} diff --git a/src/platform_linux/platform_log.h b/src/platform_linux/platform_log.h index 92433756..daa6a474 100644 --- a/src/platform_linux/platform_log.h +++ b/src/platform_linux/platform_log.h @@ -6,7 +6,6 @@ #include "splinterdb/platform_linux/public_platform.h" #include "platform_status.h" #include "platform_util.h" -#include "platform_heap.h" /* Default output file handles for different logging interfaces */ #define PLATFORM_CR "\r" @@ -47,16 +46,9 @@ platform_flush_log_stream(platform_stream_handle *stream) fflush(stream->stream); } -static inline void +void platform_close_log_stream(platform_stream_handle *stream, - platform_log_handle *log_handle) -{ - fclose(stream->stream); - fputs(stream->str, log_handle); - fflush(log_handle); - platform_free_from_heap( - NULL, stream->str, "stream", __func__, __FILE__, __LINE__); -} + platform_log_handle *log_handle); static inline platform_log_handle * platform_log_stream_to_log_handle(platform_stream_handle *stream) diff --git a/src/util.c b/src/util.c index 69bf4c56..0f046c37 100644 --- a/src/util.c +++ b/src/util.c @@ -16,22 +16,25 @@ platform_status writable_buffer_ensure_space(writable_buffer *wb, uint64 minspace) { - if (minspace <= wb->buffer_capacity) { + uint64 old_capacity = writable_buffer_capacity(wb); + if (minspace <= old_capacity) { return STATUS_OK; } - if (minspace < 2 * wb->buffer_capacity) { - minspace = 2 * wb->buffer_capacity; + if (minspace < 2 * old_capacity) { + minspace = 2 * old_capacity; } void *newdata = NULL; - if (wb->can_free) { - newdata = platform_realloc( - wb->heap_id, wb->buffer_capacity, wb->buffer, minspace); + if (wb->mode == WRITABLE_BUFFER_ALLOCED) { + newdata = platform_realloc(wb->heap_id, + wb->u.external.buffer_capacity, + wb->u.external.buffer, + minspace); } else { char *newbuf = TYPED_MANUAL_MALLOC(wb->heap_id, newbuf, minspace); if (newbuf && writable_buffer_data(wb)) { - memcpy(newbuf, wb->buffer, wb->length); + memcpy(newbuf, writable_buffer_data(wb), wb->length); } newdata = (void *)newbuf; } @@ -39,9 +42,9 @@ writable_buffer_ensure_space(writable_buffer *wb, uint64 minspace) return STATUS_NO_MEMORY; } - wb->buffer_capacity = minspace; - wb->buffer = newdata; - wb->can_free = TRUE; + wb->mode = WRITABLE_BUFFER_ALLOCED; + wb->u.external.buffer_capacity = minspace; + wb->u.external.buffer = newdata; return STATUS_OK; } diff --git a/src/util.h b/src/util.h index 4fb30344..5aa5e477 100644 --- a/src/util.h +++ b/src/util.h @@ -99,15 +99,37 @@ slice_lex_cmp(const slice a, const slice b) * buffer you give it during initialization. * ---------------------------------------------------------------------- */ +typedef enum writable_buffer_mode { + WRITABLE_BUFFER_BUILTIN = 0, + WRITABLE_BUFFER_PROVIDED = 1, + WRITABLE_BUFFER_ALLOCED = 2 +} writable_buffer_mode; + typedef struct writable_buffer { + uint64 length : 62; // Logical length + uint64 mode : 2; platform_heap_id heap_id; - void *buffer; - uint64 buffer_capacity; - uint64 length; - bool32 can_free; + union { + struct { + uint64 buffer_capacity; + void *buffer; + } external; + uint8 builtin_buffer[48]; + } u; } writable_buffer; -#define WRITABLE_BUFFER_NULL_LENGTH UINT64_MAX +_Static_assert(sizeof(writable_buffer) == PLATFORM_CACHELINE_SIZE, + "writable_buffer's builtin_buffer should be sized to make the " + "whole structure 1 cacheline in size."); + +#define WRITABLE_BUFFER_NULL_LENGTH (UINT64_MAX >> 2) + +#define NULL_WRITABLE_BUFFER(hid) \ + (writable_buffer) \ + { \ + .length = WRITABLE_BUFFER_NULL_LENGTH, .mode = WRITABLE_BUFFER_BUILTIN, \ + .heap_id = hid, \ + } /* Returns 0 if wb is in the null state */ static inline uint64 @@ -122,7 +144,11 @@ writable_buffer_length(const writable_buffer *wb) static inline uint64 writable_buffer_capacity(const writable_buffer *wb) { - return wb->buffer_capacity; + if (wb->mode == WRITABLE_BUFFER_BUILTIN) + return sizeof(wb->u.builtin_buffer); + else { + return wb->u.external.buffer_capacity; + } } /* May allocate memory */ @@ -137,8 +163,13 @@ writable_buffer_data(const writable_buffer *wb) { if (wb->length == WRITABLE_BUFFER_NULL_LENGTH) { return NULL; + } else if (wb->mode == WRITABLE_BUFFER_BUILTIN) { + // We need to cast away const -- this function won't change the buffer, + // but is intended to give away a pointer that could be used to modify the + // data in the buffer. + return (void *)wb->u.builtin_buffer; } else { - return wb->buffer; + return wb->u.external.buffer; } } @@ -155,40 +186,22 @@ writable_buffer_init_with_buffer(writable_buffer *wb, void *data, uint64 logical_size) { - wb->heap_id = heap_id; - wb->buffer = data; - wb->buffer_capacity = allocation_size; - wb->length = logical_size; - wb->can_free = FALSE; + wb->length = logical_size; + wb->mode = data == NULL ? WRITABLE_BUFFER_BUILTIN : WRITABLE_BUFFER_PROVIDED; + wb->heap_id = heap_id; + + if (data) { + wb->u.external.buffer = data; + wb->u.external.buffer_capacity = allocation_size; + } } static inline void writable_buffer_init(writable_buffer *wb, platform_heap_id heap_id) { - writable_buffer_init_with_buffer( - wb, heap_id, 0, NULL, WRITABLE_BUFFER_NULL_LENGTH); + *wb = NULL_WRITABLE_BUFFER(heap_id); } -/* - * Convenience macro for declaring and initializing an automatically - * destroyed writable buffer with a stack allocated array. Usage: - * - * DECLARE_AUTO_WRITABLE_BUFFER_N(wb, heapid, 128); - * // wb is now initialized and ready for use, e.g. - * writable_buffer_copy_slice(&wb, some_slice); - * ... - * //writable_buffer_deinit(&wb); // DO NOT CALL writable_buffer_deinit! - */ -#define DECLARE_AUTO_WRITABLE_BUFFER_N(wb, hid, n) \ - char wb##_tmp[n]; \ - writable_buffer wb __attribute__((cleanup(writable_buffer_deinit))); \ - writable_buffer_init_with_buffer(&wb, hid, n, wb##_tmp, 0) - -#define WRITABLE_BUFFER_DEFAULT_AUTO_BUFFER_SIZE (128) -#define DECLARE_AUTO_WRITABLE_BUFFER(wb, hid) \ - DECLARE_AUTO_WRITABLE_BUFFER_N( \ - wb, hid, WRITABLE_BUFFER_DEFAULT_AUTO_BUFFER_SIZE) - static inline void writable_buffer_set_to_null(writable_buffer *wb) { @@ -198,20 +211,23 @@ writable_buffer_set_to_null(writable_buffer *wb) static inline void writable_buffer_deinit(writable_buffer *wb) { - if (wb->can_free) { - platform_free(wb->heap_id, wb->buffer); + if (wb->mode == WRITABLE_BUFFER_ALLOCED) { + platform_free(wb->heap_id, wb->u.external.buffer); } - wb->buffer = NULL; - wb->can_free = FALSE; + writable_buffer_init(wb, wb->heap_id); } +#define auto_writable_buffer \ + writable_buffer __attribute__((cleanup(writable_buffer_deinit))) + + static inline void writable_buffer_memset(writable_buffer *wb, int c) { - if (wb->length == WRITABLE_BUFFER_NULL_LENGTH) { - return; + void *buffer = writable_buffer_data(wb); + if (buffer) { + memset(buffer, 0, wb->length); } - memset(wb->buffer, 0, wb->length); } static inline platform_status @@ -221,7 +237,8 @@ writable_buffer_copy_slice(writable_buffer *wb, slice src) if (!SUCCESS(rc)) { return rc; } - memcpy(wb->buffer, slice_data(src), slice_length(src)); + void *buffer = writable_buffer_data(wb); + memcpy(buffer, slice_data(src), slice_length(src)); return rc; } @@ -240,7 +257,7 @@ writable_buffer_to_slice(const writable_buffer *wb) if (wb->length == WRITABLE_BUFFER_NULL_LENGTH) { return NULL_SLICE; } else { - return slice_create(wb->length, wb->buffer); + return slice_create(wb->length, writable_buffer_data(wb)); } } @@ -257,15 +274,6 @@ writable_buffer_append(writable_buffer *wb, uint64 length, const void *newdata) return rc; } -/* - * Creates a copy of src in newly declared slice dst. Everything is - * automatically cleaned up when dst goes out of scope. - */ -#define SLICE_CREATE_LOCAL_COPY(dst, hid, src) \ - WRITABLE_BUFFER_DEFAULT(dst##wb, hid); \ - writable_buffer_copy_slice(&dst##wb, src); \ - slice dst = writable_buffer_to_slice(&dst##wb); - /* * try_string_to_(u)int64 * diff --git a/tests/functional/filter_test.c b/tests/functional/filter_test.c index 48b672fc..d84238ca 100644 --- a/tests/functional/filter_test.c +++ b/tests/functional/filter_test.c @@ -45,7 +45,7 @@ test_filter_basic(cache *cc, uint32 *num_input_keys = TYPED_ARRAY_ZALLOC(hid, num_input_keys, num_values); - DECLARE_AUTO_WRITABLE_BUFFER(keywb, hid); + auto_writable_buffer keywb = NULL_WRITABLE_BUFFER(hid); writable_buffer_resize(&keywb, key_size); writable_buffer_memset(&keywb, 0); uint64 *keybuf = writable_buffer_data(&keywb); @@ -162,7 +162,7 @@ test_filter_perf(cache *cc, if (fp_arr == NULL) { return STATUS_NO_MEMORY; } - DECLARE_AUTO_WRITABLE_BUFFER(keywb, hid); + auto_writable_buffer keywb = NULL_WRITABLE_BUFFER(hid); writable_buffer_resize(&keywb, key_size); writable_buffer_memset(&keywb, 0); uint64 *keybuf = writable_buffer_data(&keywb); diff --git a/tests/unit/misc_test.c b/tests/unit/misc_test.c index 0947b0c5..6f65b26e 100644 --- a/tests/unit/misc_test.c +++ b/tests/unit/misc_test.c @@ -13,6 +13,7 @@ #include "ctest.h" // This is required for all test-case files. #include "platform_log.h" #include "platform_units.h" +#include "platform_assert.h" #define ASSERT_OUTBUF_LEN 200 diff --git a/tests/unit/writable_buffer_test.c b/tests/unit/writable_buffer_test.c index 031379cb..ac2acde6 100644 --- a/tests/unit/writable_buffer_test.c +++ b/tests/unit/writable_buffer_test.c @@ -54,6 +54,7 @@ CTEST2(writable_buffer, test_basic_empty_buffer) writable_buffer *wb = &wb_data; writable_buffer_init(wb, data->hid); + ASSERT_TRUE(writable_buffer_is_null(wb)); ASSERT_EQUAL(0, writable_buffer_length(wb)); ASSERT_NULL(writable_buffer_data(wb)); writable_buffer_deinit(wb); @@ -72,9 +73,6 @@ CTEST2(writable_buffer, test_resize_empty_buffer) writable_buffer_resize(wb, new_length); ASSERT_EQUAL(new_length, writable_buffer_length(wb)); - - // We should have done some memory allocation. - ASSERT_TRUE(wb->can_free); ASSERT_NOT_NULL(writable_buffer_data(wb)); writable_buffer_deinit(wb); } @@ -114,8 +112,6 @@ CTEST2(writable_buffer, test_resize_empty_buffer_to_larger) uint64 new_length = 20; writable_buffer_resize(wb, new_length); - // void * vdatap = writable_buffer_data(wb); - new_length = (new_length + 50); writable_buffer_resize(wb, new_length); @@ -128,7 +124,7 @@ CTEST2(writable_buffer, test_resize_empty_buffer_to_larger) // But we can assert just these two ... ASSERT_EQUAL(new_length, writable_buffer_length(wb)); - ASSERT_TRUE(writable_buffer_length(wb) <= wb->buffer_capacity); + ASSERT_TRUE(writable_buffer_length(wb) <= writable_buffer_capacity(wb)); writable_buffer_deinit(wb); } @@ -148,24 +144,38 @@ CTEST2(writable_buffer, test_basic_user_buffer) uint64 buf_len = writable_buffer_length(wb); ASSERT_EQUAL(0, buf_len, "Unexpected buffer length=%lu", buf_len); ASSERT_EQUAL(sizeof(buf), - wb->buffer_capacity, + writable_buffer_capacity(wb), "Expected size=%lu, received buffer_size=%lu ", sizeof(buf), - wb->buffer_capacity); + writable_buffer_capacity(wb)); + ASSERT_NULL(writable_buffer_data(wb)); - // RESOLVE - This I find very odd. We just init'ed a buffer, and provided - // on-stack buffer. I would naturally want to write stuff to the address - // returned by writable_buffer_data(); like a snprintf(). But that is coming - // out as NULL. Unexpected. - // ASSERT_NOT_NULL(writable_buffer_data(wb)); // ... is failing; RESOLVE + writable_buffer_deinit(wb); +} - // We will be writing to the user-provided on-stack buffer. - ASSERT_NULL(writable_buffer_data(wb)); +/* + * Test basic usage, initializing to a user-provided on-stack buffer. + */ +CTEST2(writable_buffer, test_basic_user_buffer_with_length) +{ + writable_buffer wb_data; + writable_buffer *wb = &wb_data; + + char buf[WB_ONSTACK_BUFSIZE]; + writable_buffer_init_with_buffer( + wb, data->hid, sizeof(buf), (void *)buf, WB_ONSTACK_BUFSIZE); + + // Validate internals + uint64 buf_len = writable_buffer_length(wb); + ASSERT_EQUAL( + WB_ONSTACK_BUFSIZE, buf_len, "Unexpected buffer length=%lu", buf_len); + ASSERT_EQUAL(sizeof(buf), + writable_buffer_capacity(wb), + "Expected size=%lu, received buffer_size=%lu ", + sizeof(buf), + writable_buffer_capacity(wb)); + ASSERT_TRUE(buf == writable_buffer_data(wb)); - // But as no memory allocation has happened, yet, the "length" of - // the 'allocated' portion of the writable buffer should still == 0. - ASSERT_EQUAL(0, writable_buffer_length(wb)); - ASSERT_EQUAL(WB_ONSTACK_BUFSIZE, wb->buffer_capacity); writable_buffer_deinit(wb); } @@ -184,25 +194,10 @@ CTEST2(writable_buffer, test_resize_user_buffer_to_same_length) uint64 new_length = sizeof(buf); writable_buffer_resize(wb, new_length); - // This is fine ... BUT RESOLVE: It's odd that writable_buffer_length() - // immediately after an init() will return 0. But the same interface - // returns a non-zero length, -even- when I just resized to the size of - // the originally provided buffer. In other words, even w/o any new - // memory allocation occurring, writable_buffer_length() now returns a - // non-zero value. ASSERT_EQUAL(new_length, writable_buffer_length(wb)); - - // Nothing has changed, so data handle should still be an un-allocated one. - // RESOLVE: This is failing; It's unexpected that if we resize to the same - // length, return from here is non-NULL. This is not right. - // ASSERT_NULL(writable_buffer_data(wb)); - - // Rework ... to move test along ... - ASSERT_NOT_NULL(writable_buffer_data(wb)); ASSERT_TRUE((void *)buf == writable_buffer_data(wb)); // No allocation was done, so nothing to free - ASSERT_FALSE(wb->can_free); writable_buffer_deinit(wb); } @@ -222,11 +217,8 @@ CTEST2(writable_buffer, test_resize_user_buffer_to_smaller) writable_buffer_resize(wb, new_length); ASSERT_EQUAL(new_length, writable_buffer_length(wb)); - // RESOLVE - Again, odd; just do this to keep test moving along ... ASSERT_TRUE((void *)buf == writable_buffer_data(wb)); - ASSERT_NOT_NULL(writable_buffer_data(wb)); - ASSERT_FALSE(wb->can_free); writable_buffer_deinit(wb); } @@ -249,14 +241,10 @@ CTEST2(writable_buffer, test_resize_user_buffer_to_larger) writable_buffer_init_with_buffer(wb, data->hid, sizeof(buf), (void *)buf, 0); - // Increase buffer beyond its current length. - // RESOLVE: This is odd. I am expecting this to change to 35, but because - // the length() returns 0, this results in 5. Unexpected! uint64 new_length = (writable_buffer_length(wb) + 5); writable_buffer_resize(wb, new_length); ASSERT_EQUAL(new_length, writable_buffer_length(wb)); - // RESOLVE - Again, odd; just do this to keep test moving along ... void *old_datap_before_resize = writable_buffer_data(wb); ASSERT_TRUE((void *)buf == old_datap_before_resize); @@ -269,7 +257,6 @@ CTEST2(writable_buffer, test_resize_user_buffer_to_larger) ASSERT_TRUE(old_datap_before_resize != writable_buffer_data(wb)); ASSERT_NOT_NULL(writable_buffer_data(wb)); - ASSERT_TRUE(wb->can_free); writable_buffer_deinit(wb); } @@ -338,9 +325,6 @@ CTEST2(writable_buffer, test_copy_slice_causing_resize_larger) ASSERT_EQUAL( exp_len, act_len, "exp_len=%lu, act_len=%lu ", exp_len, act_len); - // Memory allocation should have occurred! - ASSERT_TRUE(wb->can_free); - writable_buffer_deinit(wb); } @@ -358,32 +342,6 @@ CTEST2(writable_buffer, test_basic_length_after_deinit) ASSERT_NULL(writable_buffer_data(wb)); } - -CTEST2(writable_buffer, test_resize_empty_buffer_then_check_apis_after_deinit) -{ - writable_buffer wb_data; - writable_buffer *wb = &wb_data; - - writable_buffer_init(wb, data->hid); - uint64 new_length = 20; - writable_buffer_resize(wb, new_length); - - writable_buffer_deinit(wb); - - // RESOLVE - This seems wrong that we are getting a non-zero length, which - // is set upon resize(), but we get this non-zero -after- deinit. - ASSERT_EQUAL(new_length, writable_buffer_length(wb)); - // ASSERT_EQUAL(0, writable_buffer_length(wb)); // is expected assertion. - - // RESOLVE - This seems wrong, too, that datap is non-NULL after deinit! - // ASSERT_NOT_NULL(writable_buffer_data(wb)); // is expected assertion. - - ASSERT_NULL(wb->buffer); - - // RESOLVE - It's wrong that we still think something can be freed! - ASSERT_FALSE(wb->can_free); -} - /* * This test case is interesting as the append interfaces calls realloc * below. For default heap-segment, 'realloc()' is a system-call, so stuff @@ -397,35 +355,23 @@ CTEST2(writable_buffer, test_writable_buffer_append) writable_buffer *wb = &wb_data; writable_buffer_init(wb, data->hid); - const void *old_data = writable_buffer_data(wb); const char *input_str = "Hello World!"; writable_buffer_append(wb, strlen(input_str), (const void *)input_str); + ASSERT_EQUAL(writable_buffer_length(wb), strlen(input_str)); ASSERT_STREQN(writable_buffer_data(wb), input_str, strlen(input_str)); - // append() should have reallocated new buffer space - const void *new_data = writable_buffer_data(wb); - ASSERT_TRUE((old_data != new_data)); - input_str = "Another Hello World!"; writable_buffer_append(wb, strlen(input_str), (const void *)input_str); const char *exp_str = "Hello World!Another Hello World!"; + ASSERT_EQUAL(writable_buffer_length(wb), strlen(exp_str)); ASSERT_STREQN(writable_buffer_data(wb), exp_str, strlen(exp_str), "Unexpected data: '%s'\n", (char *)writable_buffer_data(wb)); - // Currently, reallocation from shared-memory will not reuse the existing - // memory fragment, even if there is some room in it for the append. (That's - // an optimization which needs additional memory fragment-size info which - // is currently not available to the allocator.) - if (data->use_shmem) { - const void *new_data2 = writable_buffer_data(wb); - ASSERT_TRUE((new_data != new_data2)); - } - writable_buffer_deinit(wb); } @@ -459,6 +405,7 @@ CTEST2(writable_buffer, test_writable_buffer_append_beyond_orig_length) const char *exp_str = "Hello World! And, Another Hello World to create long string."; + ASSERT_EQUAL(writable_buffer_length(wb), strlen(exp_str)); ASSERT_STREQN(writable_buffer_data(wb), exp_str, strlen(exp_str), From b3162c1092822857f97452e3f9834b591d7e1c68 Mon Sep 17 00:00:00 2001 From: Rob Johnson Date: Sat, 10 Jan 2026 01:17:28 -0800 Subject: [PATCH 2/2] fix use-after-deinit bugs in core_range_iterator_init Signed-off-by: Rob Johnson --- src/core.c | 94 +++++++++++++++++++++++-------- src/data_internal.h | 1 + src/util.h | 7 ++- tests/unit/writable_buffer_test.c | 14 ----- 4 files changed, 79 insertions(+), 37 deletions(-) diff --git a/src/core.c b/src/core.c index 27b421e7..3dead0ba 100644 --- a/src/core.c +++ b/src/core.c @@ -8,6 +8,7 @@ */ #include "core.h" +#include "data_internal.h" #include "platform_sleep.h" #include "platform_time.h" #include "poison.h" @@ -803,6 +804,8 @@ core_range_iterator_init(core_handle *spl, comparison start_type, uint64 num_tuples) { + platform_status rc; + debug_assert(!key_is_null(min_key)); debug_assert(!key_is_null(max_key)); debug_assert(!key_is_null(start_key)); @@ -815,6 +818,11 @@ core_range_iterator_init(core_handle *spl, range_itor->can_prev = TRUE; range_itor->can_next = TRUE; + key_buffer_init(&range_itor->min_key, spl->heap_id); + key_buffer_init(&range_itor->max_key, spl->heap_id); + key_buffer_init(&range_itor->local_min_key, spl->heap_id); + key_buffer_init(&range_itor->local_max_key, spl->heap_id); + if (core_key_compare(spl, min_key, start_key) > 0) { // in bounds, start at min start_key = min_key; @@ -825,8 +833,17 @@ core_range_iterator_init(core_handle *spl, } // copy over global min and max - key_buffer_init_from_key(&range_itor->min_key, spl->heap_id, min_key); - key_buffer_init_from_key(&range_itor->max_key, spl->heap_id, max_key); + rc = key_buffer_copy_key(&range_itor->min_key, min_key); + if (!SUCCESS(rc)) { + core_range_iterator_deinit(range_itor); + return rc; + } + rc = key_buffer_copy_key(&range_itor->max_key, max_key); + if (!SUCCESS(rc)) { + core_range_iterator_deinit(range_itor); + return rc; + } + ZERO_ARRAY(range_itor->compacted); @@ -868,16 +885,15 @@ core_range_iterator_init(core_handle *spl, } trunk_ondisk_node_handle root_handle; - trunk_init_root_handle(&spl->trunk_context, &root_handle); - + rc = trunk_init_root_handle(&spl->trunk_context, &root_handle); memtable_end_lookup(spl->mt_ctxt); + if (!SUCCESS(rc)) { + core_range_iterator_deinit(range_itor); + return rc; + } - key_buffer_init(&range_itor->local_min_key, spl->heap_id); - key_buffer_init(&range_itor->local_max_key, spl->heap_id); - - platform_status rc; - uint64 old_num_branches = range_itor->num_branches; - rc = trunk_collect_branches(&spl->trunk_context, + uint64 old_num_branches = range_itor->num_branches; + rc = trunk_collect_branches(&spl->trunk_context, &root_handle, start_key, start_type, @@ -887,7 +903,10 @@ core_range_iterator_init(core_handle *spl, &range_itor->local_min_key, &range_itor->local_max_key); trunk_ondisk_node_handle_deinit(&root_handle); - platform_assert_status_ok(rc); + if (!SUCCESS(rc)) { + core_range_iterator_deinit(range_itor); + return rc; + } for (uint64 i = old_num_branches; i < range_itor->num_branches; i++) { range_itor->compacted[i] = TRUE; @@ -899,14 +918,20 @@ core_range_iterator_init(core_handle *spl, <= 0) { rc = key_buffer_copy_key(&range_itor->local_min_key, min_key); - platform_assert_status_ok(rc); + if (!SUCCESS(rc)) { + core_range_iterator_deinit(range_itor); + return rc; + } } if (core_key_compare( spl, key_buffer_key(&range_itor->local_max_key), max_key) >= 0) { rc = key_buffer_copy_key(&range_itor->local_max_key, max_key); - platform_assert_status_ok(rc); + if (!SUCCESS(rc)) { + core_range_iterator_deinit(range_itor); + return rc; + } } for (uint64 i = 0; i < range_itor->num_branches; i++) { @@ -949,7 +974,10 @@ core_range_iterator_init(core_handle *spl, MERGE_FULL, greater_than <= start_type, &range_itor->merge_itor); - platform_assert_status_ok(rc); + if (!SUCCESS(rc)) { + core_range_iterator_deinit(range_itor); + return rc; + } bool32 in_range = iterator_can_curr(&range_itor->merge_itor->super); @@ -960,15 +988,26 @@ core_range_iterator_init(core_handle *spl, if (!in_range && start_type >= greater_than) { key local_max = key_buffer_key(&range_itor->local_max_key); if (core_key_compare(spl, local_max, max_key) < 0) { + key_buffer local_max_buffer; + rc = key_buffer_init_from_key( + &local_max_buffer, spl->heap_id, local_max); core_range_iterator_deinit(range_itor); - rc = core_range_iterator_init(spl, + if (!SUCCESS(rc)) { + return rc; + } + local_max = key_buffer_key(&local_max_buffer); + rc = core_range_iterator_init(spl, range_itor, min_key, max_key, local_max, start_type, range_itor->num_tuples); - platform_assert_status_ok(rc); + key_buffer_deinit(&local_max_buffer); + if (!SUCCESS(rc)) { + return rc; + } + } else { range_itor->can_next = FALSE; range_itor->can_prev = @@ -978,15 +1017,26 @@ core_range_iterator_init(core_handle *spl, if (!in_range && start_type <= less_than_or_equal) { key local_min = key_buffer_key(&range_itor->local_min_key); if (core_key_compare(spl, local_min, min_key) > 0) { + key_buffer local_min_buffer; + rc = key_buffer_init_from_key( + &local_min_buffer, spl->heap_id, local_min); core_range_iterator_deinit(range_itor); - rc = core_range_iterator_init(spl, + if (!SUCCESS(rc)) { + return rc; + } + local_min = key_buffer_key(&local_min_buffer); + rc = core_range_iterator_init(spl, range_itor, min_key, max_key, local_min, start_type, range_itor->num_tuples); - platform_assert_status_ok(rc); + key_buffer_deinit(&local_min_buffer); + if (!SUCCESS(rc)) { + return rc; + } + } else { range_itor->can_prev = FALSE; range_itor->can_next = @@ -1158,11 +1208,11 @@ core_range_iterator_deinit(core_range_iterator *range_itor) core_memtable_dec_ref(spl, mt_gen); } } - key_buffer_deinit(&range_itor->min_key); - key_buffer_deinit(&range_itor->max_key); - key_buffer_deinit(&range_itor->local_min_key); - key_buffer_deinit(&range_itor->local_max_key); } + key_buffer_deinit(&range_itor->min_key); + key_buffer_deinit(&range_itor->max_key); + key_buffer_deinit(&range_itor->local_min_key); + key_buffer_deinit(&range_itor->local_max_key); } /* diff --git a/src/data_internal.h b/src/data_internal.h index 4e73ba9e..88b6325b 100644 --- a/src/data_internal.h +++ b/src/data_internal.h @@ -151,6 +151,7 @@ key_buffer_init(key_buffer *kb, platform_heap_id hid) { kb->kind = USER_KEY; writable_buffer_init(&kb->wb, hid); + writable_buffer_resize(&kb->wb, 0); } static inline platform_status diff --git a/src/util.h b/src/util.h index 5aa5e477..60108dc3 100644 --- a/src/util.h +++ b/src/util.h @@ -214,7 +214,12 @@ writable_buffer_deinit(writable_buffer *wb) if (wb->mode == WRITABLE_BUFFER_ALLOCED) { platform_free(wb->heap_id, wb->u.external.buffer); } - writable_buffer_init(wb, wb->heap_id); + // Put the buffer in a state that should cause a crash if anyone tries to use + // it. + wb->mode = WRITABLE_BUFFER_ALLOCED; + wb->length = 1; + wb->u.external.buffer = NULL; + wb->u.external.buffer_capacity = 1; } #define auto_writable_buffer \ diff --git a/tests/unit/writable_buffer_test.c b/tests/unit/writable_buffer_test.c index ac2acde6..675b463c 100644 --- a/tests/unit/writable_buffer_test.c +++ b/tests/unit/writable_buffer_test.c @@ -328,20 +328,6 @@ CTEST2(writable_buffer, test_copy_slice_causing_resize_larger) writable_buffer_deinit(wb); } -/* - * Test APIs after deinit is done. - */ -CTEST2(writable_buffer, test_basic_length_after_deinit) -{ - writable_buffer wb_data; - writable_buffer *wb = &wb_data; - - writable_buffer_init(wb, data->hid); - writable_buffer_deinit(wb); - ASSERT_EQUAL(0, writable_buffer_length(wb)); - ASSERT_NULL(writable_buffer_data(wb)); -} - /* * This test case is interesting as the append interfaces calls realloc * below. For default heap-segment, 'realloc()' is a system-call, so stuff