Keep codec option updates atomic on allocation failure

include/avif/internal.h

@@ -525,7 +525,8 @@ AVIF_ARRAY_DECLARE(avifCodecSpecificOptions, avifCodecSpecificOption, entries);
 AVIF_NODISCARD avifCodecSpecificOptions * avifCodecSpecificOptionsCreate(void);
 void avifCodecSpecificOptionsClear(avifCodecSpecificOptions * csOptions);
 void avifCodecSpecificOptionsDestroy(avifCodecSpecificOptions * csOptions);
-avifResult avifCodecSpecificOptionsSet(avifCodecSpecificOptions * csOptions, const char * key, const char * value); // if(value==NULL), key is deleted
+// If value is NULL, key is deleted. On allocation failure, csOptions is left unchanged.
+avifResult avifCodecSpecificOptionsSet(avifCodecSpecificOptions * csOptions, const char * key, const char * value);
 
 // ---------------------------------------------------------------------------
 // avifCodecType (underlying video format)

src/avif.c

@@ -1128,9 +1128,10 @@ avifResult avifCodecSpecificOptionsSet(avifCodecSpecificOptions * csOptions, con
         if (!strcmp(entry->key, key)) {
             if (value) {
                 // Update the value
+                char * newValue = avifStrdup(value);
+                AVIF_CHECKERR(newValue, AVIF_RESULT_OUT_OF_MEMORY);
                 avifFree(entry->value);
-                entry->value = avifStrdup(value);
-                AVIF_CHECKERR(entry->value, AVIF_RESULT_OUT_OF_MEMORY);
+                entry->value = newValue;
             } else {
                 // Delete the value
                 avifFree(entry->key);
@@ -1146,12 +1147,21 @@ avifResult avifCodecSpecificOptionsSet(avifCodecSpecificOptions * csOptions, con
 
     if (value) {
         // Add a new key
+        char * newKey = avifStrdup(key);
+        AVIF_CHECKERR(newKey, AVIF_RESULT_OUT_OF_MEMORY);
+        char * newValue = avifStrdup(value);
+        if (!newValue) {
+            avifFree(newKey);
+            return AVIF_RESULT_OUT_OF_MEMORY;
+        }
         avifCodecSpecificOption * entry = (avifCodecSpecificOption *)avifArrayPush(csOptions);
-        AVIF_CHECKERR(entry, AVIF_RESULT_OUT_OF_MEMORY);
-        entry->key = avifStrdup(key);
-        AVIF_CHECKERR(entry->key, AVIF_RESULT_OUT_OF_MEMORY);
-        entry->value = avifStrdup(value);
-        AVIF_CHECKERR(entry->value, AVIF_RESULT_OUT_OF_MEMORY);
+        if (!entry) {
+            avifFree(newKey);
+            avifFree(newValue);
+            return AVIF_RESULT_OUT_OF_MEMORY;
+        }
+        entry->key = newKey;
+        entry->value = newValue;
     }
     return AVIF_RESULT_OK;
 }

tests/CMakeLists.txt

@@ -43,6 +43,18 @@ foreach(AVIFYUV_MODE limited rgb) # Modes drift and premultiply take more than 2
     add_test(NAME avifyuv_${AVIFYUV_MODE} COMMAND avifyuv -m ${AVIFYUV_MODE})
 endforeach()
 
+add_executable(avifoptionstest avifoptionstest.c)
+if(AVIF_CODEC_LIBGAV1_ENABLED OR AVIF_LIBYUV_ENABLED)
+    set_target_properties(avifoptionstest PROPERTIES LINKER_LANGUAGE "CXX")
+endif()
+target_link_libraries(avifoptionstest PRIVATE avif_internal avif_enable_warnings)
+if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
+    target_compile_definitions(avifoptionstest PRIVATE AVIF_OPTIONSTEST_WRAP_MALLOC)
+    target_link_options(avifoptionstest PRIVATE "LINKER:--wrap=malloc")
+endif()
+add_test(NAME avifoptionstest COMMAND avifoptionstest)
+register_test_for_coverage(avifoptionstest)
+
 if(AVIF_FUZZTEST OR AVIF_GTEST OR AVIF_BUILD_APPS)
     add_library(aviftest_helpers OBJECT gtest/aviftest_helpers.cc)
     target_link_libraries(aviftest_helpers PUBLIC avif_apps_internal avif_internal)

tests/avifoptionstest.c

@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: BSD-2-Clause
+
+#include "avif/internal.h"
+
+#include <stdio.h>
+#include <string.h>
+
+static int failures;
+
+#define CHECK(condition)                                                    \
+    do {                                                                    \
+        if (!(condition)) {                                                 \
+            fprintf(stderr, "%s:%d: %s\n", __FILE__, __LINE__, #condition); \
+            ++failures;                                                     \
+            return;                                                         \
+        }                                                                   \
+    } while (0)
+
+static void checkOption(const avifCodecSpecificOptions * options, uint32_t index, const char * key, const char * value)
+{
+    CHECK(index < options->count);
+    CHECK(!strcmp(options->entries[index].key, key));
+    CHECK(!strcmp(options->entries[index].value, value));
+}
+
+static void testNormalOperations(void)
+{
+    avifCodecSpecificOptions * options = avifCodecSpecificOptionsCreate();
+    CHECK(options != NULL);
+    const uint32_t initialCapacity = options->capacity;
+    CHECK(initialCapacity == 4);
+
+    CHECK(avifCodecSpecificOptionsSet(options, "a", "one") == AVIF_RESULT_OK);
+    CHECK(avifCodecSpecificOptionsSet(options, "b", "two") == AVIF_RESULT_OK);
+    CHECK(options->count == 2);
+    checkOption(options, 0, "a", "one");
+    checkOption(options, 1, "b", "two");
+
+    CHECK(avifCodecSpecificOptionsSet(options, "a", "updated") == AVIF_RESULT_OK);
+    CHECK(options->count == 2);
+    checkOption(options, 0, "a", "updated");
+
+    CHECK(avifCodecSpecificOptionsSet(options, "b", NULL) == AVIF_RESULT_OK);
+    CHECK(options->count == 1);
+    checkOption(options, 0, "a", "updated");
+
+    CHECK(avifCodecSpecificOptionsSet(options, "c", "three") == AVIF_RESULT_OK);
+    CHECK(avifCodecSpecificOptionsSet(options, "d", "four") == AVIF_RESULT_OK);
+    CHECK(avifCodecSpecificOptionsSet(options, "e", "five") == AVIF_RESULT_OK);
+    CHECK(options->count == initialCapacity);
+    CHECK(avifCodecSpecificOptionsSet(options, "f", "six") == AVIF_RESULT_OK);
+    CHECK(options->count == initialCapacity + 1);
+    CHECK(options->capacity == initialCapacity * 2);
+    checkOption(options, 4, "f", "six");
+
+    avifCodecSpecificOptionsClear(options);
+    CHECK(options->count == 0);
+    CHECK(options->capacity == initialCapacity * 2);
+    avifCodecSpecificOptionsDestroy(options);
+}
+
+#if defined(AVIF_OPTIONSTEST_WRAP_MALLOC)
+
+extern void * __real_malloc(size_t size);
+
+// A nonnegative value permits that many allocations before the next one fails.
+static int mallocFailuresAfter = -1;
+
+void * __wrap_malloc(size_t size)
+{
+    if (mallocFailuresAfter == 0) {
+        return NULL;
+    }
+    if (mallocFailuresAfter > 0) {
+        --mallocFailuresAfter;
+    }
+    return __real_malloc(size);
+}
+
+typedef struct optionState
+{
+    avifCodecSpecificOption * entries;
+    uint32_t count;
+    uint32_t capacity;
+    char * keys[4];
+    char * values[4];
+} optionState;
+
+static optionState saveOptionState(const avifCodecSpecificOptions * options)
+{
+    optionState state;
+    memset(&state, 0, sizeof(state));
+    state.entries = options->entries;
+    state.count = options->count;
+    state.capacity = options->capacity;
+    if (state.count > 4) {
+        fprintf(stderr, "%s:%d: state.count <= 4\n", __FILE__, __LINE__);
+        ++failures;
+        state.count = 0;
+        return state;
+    }
+    for (uint32_t i = 0; i < state.count; ++i) {
+        state.keys[i] = options->entries[i].key;
+        state.values[i] = options->entries[i].value;
+    }
+    return state;
+}
+
+static void checkOptionState(const avifCodecSpecificOptions * options, const optionState * state)
+{
+    CHECK(options->entries == state->entries);
+    CHECK(options->count == state->count);
+    CHECK(options->capacity == state->capacity);
+    for (uint32_t i = 0; i < state->count; ++i) {
+        CHECK(options->entries[i].key == state->keys[i]);
+        CHECK(options->entries[i].value == state->values[i]);
+    }
+}
+
+static void testReplaceAllocationFailure(void)
+{
+    avifCodecSpecificOptions * options = avifCodecSpecificOptionsCreate();
+    CHECK(options != NULL);
+    CHECK(avifCodecSpecificOptionsSet(options, "key", "old") == AVIF_RESULT_OK);
+    const optionState state = saveOptionState(options);
+
+    mallocFailuresAfter = 0;
+    const avifResult result = avifCodecSpecificOptionsSet(options, "key", "new");
+    mallocFailuresAfter = -1;
+    CHECK(result == AVIF_RESULT_OUT_OF_MEMORY);
+    checkOptionState(options, &state);
+    checkOption(options, 0, "key", "old");
+    avifCodecSpecificOptionsDestroy(options);
+}
+
+static void testAddKeyAndValueAllocationFailures(void)
+{
+    avifCodecSpecificOptions * options = avifCodecSpecificOptionsCreate();
+    CHECK(options != NULL);
+    CHECK(avifCodecSpecificOptionsSet(options, "base", "value") == AVIF_RESULT_OK);
+    const optionState state = saveOptionState(options);
+
+    mallocFailuresAfter = 0;
+    avifResult result = avifCodecSpecificOptionsSet(options, "new", "value");
+    mallocFailuresAfter = -1;
+    CHECK(result == AVIF_RESULT_OUT_OF_MEMORY);
+    checkOptionState(options, &state);
+
+    mallocFailuresAfter = 1;
+    result = avifCodecSpecificOptionsSet(options, "new", "value");
+    mallocFailuresAfter = -1;
+    CHECK(result == AVIF_RESULT_OUT_OF_MEMORY);
+    checkOptionState(options, &state);
+    checkOption(options, 0, "base", "value");
+    avifCodecSpecificOptionsDestroy(options);
+}
+
+static void testGrowthAllocationFailure(void)
+{
+    avifCodecSpecificOptions * options = avifCodecSpecificOptionsCreate();
+    CHECK(options != NULL);
+    CHECK(options->capacity == 4);
+    CHECK(avifCodecSpecificOptionsSet(options, "a", "1") == AVIF_RESULT_OK);
+    CHECK(avifCodecSpecificOptionsSet(options, "b", "2") == AVIF_RESULT_OK);
+    CHECK(avifCodecSpecificOptionsSet(options, "c", "3") == AVIF_RESULT_OK);
+    CHECK(avifCodecSpecificOptionsSet(options, "d", "4") == AVIF_RESULT_OK);
+    const optionState state = saveOptionState(options);
+
+    mallocFailuresAfter = 2;
+    const avifResult result = avifCodecSpecificOptionsSet(options, "e", "5");
+    mallocFailuresAfter = -1;
+    CHECK(result == AVIF_RESULT_OUT_OF_MEMORY);
+    checkOptionState(options, &state);
+    checkOption(options, 0, "a", "1");
+    checkOption(options, 3, "d", "4");
+    avifCodecSpecificOptionsDestroy(options);
+}
+
+#endif
+
+int main(void)
+{
+    testNormalOperations();
+#if defined(AVIF_OPTIONSTEST_WRAP_MALLOC)
+    testReplaceAllocationFailure();
+    testAddKeyAndValueAllocationFailures();
+    testGrowthAllocationFailure();
+#endif
+    return failures != 0;
+}