From bb54c2b419f69038719dd5d6a1b44659cd418df9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 24 Aug 2026 16:14:15 +0200 Subject: [PATCH] AVRO-4305: [c] Make st_insert() OOM-safe The vendored st hash table allocated entries via avro_new() inside the ADD_DIRECT macro (used by st_insert and st_add_direct) without checking for a NULL return. On allocation failure this dereferenced NULL and crashed rather than reporting an error, so map decoding and every other st user was not OOM-safe on that path. The rehash() helper had the same problem with its Calloc() of the new bin array. - ADD_DIRECT now receives a pre-allocated entry; st_insert allocates it, returns -1 on failure and leaves the table unchanged (0 = inserted, 1 = updated, as before). st_add_direct (void) becomes a no-op on failure. - rehash() skips the rehash and keeps the existing bins when the new bin array cannot be allocated (the table stays correct, only denser). - The only caller that inspects the return value (save_named_schemas in schema.c) already treats any non-zero as an error, so -1 propagates correctly. Adds test_avro_4305, which installs a failing allocator and verifies st_insert returns -1 without crashing and leaves the table unchanged, and that a subsequent insert still works. The test segfaults without the fix. --- lang/c/src/st.c | 26 +++++++--- lang/c/src/st.h | 2 + lang/c/tests/CMakeLists.txt | 1 + lang/c/tests/test_avro_4305.c | 93 +++++++++++++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 6 deletions(-) create mode 100644 lang/c/tests/test_avro_4305.c diff --git a/lang/c/src/st.c b/lang/c/src/st.c index 8437777cb92..4cc145b6cfe 100644 --- a/lang/c/src/st.c +++ b/lang/c/src/st.c @@ -249,16 +249,13 @@ int st_lookup(st_table *table, register st_data_t key, st_data_t *value) } } -#define ADD_DIRECT(table, key, value, hash_val, bin_pos)\ +#define ADD_DIRECT(table, key, value, hash_val, bin_pos, entry)\ do {\ - st_table_entry *entry;\ if (table->num_entries/(table->num_bins) > ST_DEFAULT_MAX_DENSITY) {\ rehash(table);\ bin_pos = hash_val % table->num_bins;\ }\ \ - entry = (st_table_entry *) avro_new(st_table_entry);\ - \ entry->hash = hash_val;\ entry->key = key;\ entry->record = value;\ @@ -276,7 +273,13 @@ int st_insert(register st_table *table, register st_data_t key, st_data_t value) FIND_ENTRY(table, ptr, hash_val, bin_pos); if (ptr == 0) { - ADD_DIRECT(table, key, value, hash_val, bin_pos); + st_table_entry *entry = (st_table_entry *) avro_new(st_table_entry); + if (entry == NULL) { + /* Out of memory: leave the table unchanged rather than + * dereferencing a NULL entry. */ + return -1; + } + ADD_DIRECT(table, key, value, hash_val, bin_pos, entry); return 0; } else { ptr->record = value; @@ -287,10 +290,16 @@ int st_insert(register st_table *table, register st_data_t key, st_data_t value) void st_add_direct(st_table *table,st_data_t key,st_data_t value) { unsigned int hash_val, bin_pos; + st_table_entry *entry; hash_val = do_hash((void*) key, table); bin_pos = hash_val % table->num_bins; - ADD_DIRECT(table, key, value, hash_val, bin_pos); + entry = (st_table_entry *) avro_new(st_table_entry); + if (entry == NULL) { + /* Out of memory: leave the table unchanged. */ + return; + } + ADD_DIRECT(table, key, value, hash_val, bin_pos, entry); } static void rehash(register st_table *table) @@ -302,6 +311,11 @@ static void rehash(register st_table *table) new_num_bins = new_size(old_num_bins + 1); new_bins = (st_table_entry **) Calloc(new_num_bins, sizeof(st_table_entry *)); + if (new_bins == NULL) { + /* Out of memory: skip the rehash and keep the existing bins. + * The table stays correct, only denser. */ + return; + } for (i = 0; i < old_num_bins; i++) { ptr = table->bins[i]; diff --git a/lang/c/src/st.h b/lang/c/src/st.h index 93da018bd9b..420aef88d1e 100644 --- a/lang/c/src/st.h +++ b/lang/c/src/st.h @@ -61,6 +61,8 @@ st_table *st_init_strtable _((void)); st_table *st_init_strtable_with_size _((int)); int st_delete _((st_table *, st_data_t *, st_data_t *)); int st_delete_safe _((st_table *, st_data_t *, st_data_t *, st_data_t)); +/* Returns 0 if a new entry was added, 1 if an existing key was updated, or -1 + * if a new entry could not be allocated (the table is left unchanged). */ int st_insert _((st_table *, st_data_t, st_data_t)); int st_lookup _((st_table *, st_data_t, st_data_t *)); int st_foreach _((st_table *, hash_function_foreach, st_data_t)); diff --git a/lang/c/tests/CMakeLists.txt b/lang/c/tests/CMakeLists.txt index 6b4164fa740..776ee4d2652 100644 --- a/lang/c/tests/CMakeLists.txt +++ b/lang/c/tests/CMakeLists.txt @@ -88,3 +88,4 @@ add_avro_test_checkmem(test_avro_1691) add_avro_test_checkmem(test_avro_1906) add_avro_test_checkmem(test_avro_1904) add_avro_test_checkmem(test_avro_4246) +add_avro_test_checkmem(test_avro_4305) diff --git a/lang/c/tests/test_avro_4305.c b/lang/c/tests/test_avro_4305.c new file mode 100644 index 00000000000..7c6d7cff3a5 --- /dev/null +++ b/lang/c/tests/test_avro_4305.c @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may not use + * this file except in compliance with the License. You may obtain + * a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License 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. + */ + +/* + * Regression test for AVRO-4305: st_insert() must be OOM-safe. On allocation + * failure it should return -1 and leave the table unchanged, rather than + * dereferencing the NULL returned by avro_new() in ADD_DIRECT. + */ + +#include +#include + +#include "avro.h" +#include "avro_private.h" +#include "st.h" + +/* When armed, this allocator fails every allocation to simulate OOM. */ +static int fail_allocations = 0; + +static void * +oom_allocator(void *user_data, void *ptr, size_t osize, size_t nsize) +{ + (void) user_data; + (void) osize; + if (nsize == 0) { + free(ptr); + return NULL; + } + if (fail_allocations) { + return NULL; + } + return realloc(ptr, nsize); +} + +int main(void) +{ + avro_set_allocator(oom_allocator, NULL); + + st_table *table = st_init_strtable(); + if (table == NULL) { + fprintf(stderr, "could not create table\n"); + return EXIT_FAILURE; + } + + /* Arm OOM and try to insert a new key: st_insert must fail gracefully + * with -1 and must not modify the table (previously this dereferenced + * a NULL entry and crashed). */ + fail_allocations = 1; + int rval = st_insert(table, (st_data_t) "key", (st_data_t) 42); + fail_allocations = 0; + + if (rval != -1) { + fprintf(stderr, "expected st_insert to return -1 on OOM, got %d\n", rval); + return EXIT_FAILURE; + } + if (table->num_entries != 0) { + fprintf(stderr, "table must be unchanged after OOM, num_entries=%d\n", + table->num_entries); + return EXIT_FAILURE; + } + + /* After memory is available again the same insert must succeed. */ + rval = st_insert(table, (st_data_t) "key", (st_data_t) 42); + if (rval != 0) { + fprintf(stderr, "expected successful insert to return 0, got %d\n", rval); + return EXIT_FAILURE; + } + + st_data_t value = 0; + if (!st_lookup(table, (st_data_t) "key", &value) || value != 42) { + fprintf(stderr, "lookup after successful insert failed\n"); + return EXIT_FAILURE; + } + + st_free_table(table); + fprintf(stderr, "test_avro_4305 passed\n"); + return EXIT_SUCCESS; +}