Keep codec option updates atomic on allocation failure - #3350
Conversation
y-guyon
left a comment
y-guyon
left a comment
There was a problem hiding this comment.
Thank you for the change to internal.h and avif.c, it looks good to me.
Could the test use GoogleTest? Or does it need to be written in C?
We should also use __wrap_malloc in a fuzzer target to cover most OOM checks in libavif.
| @@ -0,0 +1,190 @@ | |||
| // SPDX-License-Identifier: BSD-2-Clause | |||
There was a problem hiding this comment.
Yannis: We should not accept this test. It uses excessive (platform-specific) machinery to test only how a single function handles memory allocation failures.
There was a problem hiding this comment.
I thought that was better than no test at all, and that could be later transformed into a fuzz target. I am fine with removing this test entirely.
There was a problem hiding this comment.
Since this test comes from an anonymous contributor, the burden of cleaning the test up is likely to fall on us and we may never get around to it. That's why I prefer not adding the test because it is quite different in quality and style from the tests we wrote. In any case I have gone over the test and will try to clean it up in my spare time.
And thank you for reviewing the pull request!
|
|
||
| if (value) { | ||
| // Add a new key | ||
| // Prepare both strings before publishing a new entry. |
There was a problem hiding this comment.
Please restore the original comment "Add a new key" before this line.
Optional: We can remove this comment. This comment suggests it is necessary to perform these operations in the new order, but that's not true. We can also handle allocation failures properly in the original order by calling avifArrayPop(csOptions) to free entry. (The avifArrayPop() function was added exactly for this purpose.)
| CHECK(avifCodecSpecificOptionsSet(options, "key", "old") == AVIF_RESULT_OK); | ||
| const optionState state = saveOptionState(options); | ||
|
|
||
| mallocFailuresAfter = 0; |
There was a problem hiding this comment.
When we set mallocFailuresAfter to a nonnegative value, we should add a comment to note which allocation in avifCodecSpecificOptionsSet() we are targeting.
This will help us decide if we need to update this test when we modify the memory allocations in avifCodecSpecificOptionsSet().
| CHECK(!strcmp(options->entries[index].value, value)); | ||
| } | ||
|
|
||
| static void testNormalOperations(void) |
There was a problem hiding this comment.
This test checks implementation details such as avifCodecSpecificOptions is created with an initial capacity of 4 and the capacity of an avif array is grown by doubling. These details are not part of the API contract, so we don't need to check them.
| @@ -0,0 +1,190 @@ | |||
| // SPDX-License-Identifier: BSD-2-Clause | |||
There was a problem hiding this comment.
Since this test comes from an anonymous contributor, the burden of cleaning the test up is likely to fall on us and we may never get around to it. That's why I prefer not adding the test because it is quite different in quality and style from the tests we wrote. In any case I have gone over the test and will try to clean it up in my spare time.
And thank you for reviewing the pull request!
3 participants
Summary
Keep codec-specific option updates transactional when an allocation fails.
strings if value duplication or array growth fails.
existing option state unchanged.
failures. The test follows the existing C++ linker selection for libgav1/libyuv.
Validation
Built the patched library and ran
avifoptionstestwith GCC 15.2 and warningstreated as errors in Debug, Release, and AddressSanitizer/UndefinedBehaviorSanitizer
builds; all passed. Leak detection was enabled for the sanitized run.
The test covers replacement, key-copy, value-copy, and array-growth allocation
failures using tiny strings. Fault injection uses
--wrap=mallocon Linux withGNU/Clang; other platforms retain the normal-operation checks. Release checks
remain active under
NDEBUG.These were codec-disabled builds. The full codec-enabled suite and non-Linux
configurations were not run locally. Changed C/header files pass clang-format 19.
The tests were run on the patched tree only; no unpatched comparison was run.
AI-assisted implementation with local build/test verification.