AVRO-4305: [c] Make st_insert() OOM-safe

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];

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));

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)

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 <stdio.h>
+#include <stdlib.h>
+
+#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;
+}