Fix OSS-Fuzz found issues and update to latest cJSON version #1614
Conversation
These test cases all crash with their corresponding tests, allowing easy reproduction and eventual fixing.
Several functions have been added to the cJSON.h/.c files. While these functions are important, they do not belong in the cJSON source code files, as it makes it difficult to update cJSON without stomping all over the added code. This does require re-adding cJSON_Parse to the cJSON.h header file, which was previously removed as it was only used internally to cJSON.c. Now that it isn't the case, it is needed again.
Sets CJSON_HIDE_SYMBOLS since that is the default we want.
Remove cJSON functions that aren't used
This is a modification the loader does to reduce the number of string copies that are required. Since the loader doesn't care about the quotes, they are simply omitted, making usage of the strings much easier. Note that this change was done to the previously checked in version of cJSON, so these changes are the same conceptual changes made to the newer version of the cJSON library.
This brings cJSON back into its pre-update form, as well as documenting that it is an intentional change.
Also prepend loader_ to cJSON functions to reduce symbol collision. This was done before but removed while updating cJSON just to ensure that the code would still work (kinda, the tests failed but no catastrophic incompatibilities were introduced into cJSON).
Passing the error string directly to loader_log() as the format argument can cause crashes since the string comes from JSON files on the system and may contain format specifiers which vsnprintf will try to use and subsequently fail.
By adding a per-compiler __attribute, we can tell compilers to check the variable arguments passed into loader_log using the format argument. This caught several instances of more parameters being passed in than were in the format string.
The implementation of cJSON_GetArrayItem is very simple - it finds the node at an index by iterating the linked-list of child nodes index number of times. This is problematic because it means using cJSON_GetArrayItem(node, i) to iterate all elements becomes a O(n^2). The fix is to use the helper function cJSON_ArrayForEach which directly iterates on the linked list. This change takes time to refactor the get_loader_settings() logic that handles whether the file contains just the settings object or an array of settings objects, making the logic easier to follow.
If meta-layer A has meta-layer B in its component layer list, and meta-layer B has meta-layer A in its component layer list, this would cause a stack overflow in verify_meta_layer_component_layers() since it would recursively check the meta layer of the other layer. Fixing it means checking that the a component layer is a meta layer, and if so, checking the component layer's component layers for the meta layer being verified. Granted, this check is only 1 deep so more complex recursive checks may be needed.
loader_parse_json_string_to_existing_str() doesn't need to allocate any temporary memory, as cJSON_PrintPreallocated allows dumping directly to the char array that is the final destination.
The cJSON_GetStringValue function allows us to read the contents of the cJSON string without printing, which requires a memory allocation. This simplifies the logic by not needing to free anything.
By using cJSON_GetStringValue, many allocations can be avoided, which simplifies the logic by removing potential error paths.
Makes sure we are only moving valid elements around, and not iterating past the end of the allocation.
If "library_path" contained nothing, loader_parse_json_string returns the string "", which still needs to be freed.
Because these functions are requied on windows for paths to behave correct, it is good to put them in common code functions (like EnvVarWrapper) instead of requiring all code to remember to call them. Having JsonWriter handle the path fixup means less changes of double application occuring.
|
CI Vulkan-Loader build queued with queue ID 310833. |
|
CI Vulkan-Loader build # 2809 running. |
|
CI Vulkan-Loader build # 2809 passed. |
|
@DavidKorczynski here is a PR which fixes all of the known OSS-Fuzz issues. Would greatly appreciate confirmation that they fix the issues found on your end as well. I added the repro's to the Vulkan-Loader source since they are handy blobs of 'bad json' to test against. I also plan on merging the changes relatively soon since the SDK release cycle is close at hand. |
There was a problem hiding this comment.
This looks great @charles-lunarg
OSS-Fuzz will automatically report the issues as fixed once the PR lands. So all the issues with "New" (which means open) should change to "Verified" once OSS-Fuzz verifies they are fixed. This should happen within 24 hours of the PR landing. Note you have to be logged in on the URL with the email from the project.yaml to see the open issues. You will also get email notifications for each issue fixed, confirmed that it is fixed.
I agree updating cjson is good step and we shouldn't be concerned with double fuzzing IMO. Once this lands, there is a chance the fuzzers will continue to find more bugs as they can progress from the bugs they are blocked at the moment. I will also take a look at adding new fuzzers once we have stabilized with the existing bugs.
|
There are always more bugs... but it's very good to know the process of these things! The double fuzzing technically isn't true due to the number of changes that were made to the cJSON copy in the loader, but then again json being the main entry point makes it doubly important to fuzz well. I'm curious what new bugs are found by the fuzzer, the current slate had some interesting corner cases I hadn't considered (recursive meta layers was the most sophisticated). |
The OSS-Fuzz project found many case which caused the loader to crash, perform UB, leak, etc. Since this PR is massive, I've detailed the development process below.
The first step was to include the manifest files that cause issues, which is doubly good because it serves as great "weird inputs" to use in the loader test suite.
The second step was to update the cJSON library because many of the issues were in the implementation of OSS-Fuzz. This is essential because updating the cJSON codebase immediately fixed about half of the issues. I believe cJSON got the OSS-Fuzz treatment not too long ago, so in some ways cJSON is doubly fuzzed now (because it was baked into the loader source).
The third step was to re-implement the changes made to cJSON to use the loader's allocator & OOM output parameter, as the loader distinguishes OOM errors from failure to parse errors.
The fourth step was to re-implement the change to cJSON to not surround strings with "", which allows many fewer allocations to take place.
Now that the existing tests work, new tests (one for each of the OSS-Fuzz found issues) were created and fixes for each issue was written. There were many fixed leaks, null checks, and general "good stuff", but the most interesting issue found was "recursive meta layers", where 2 meta layers listed eachother in their component layers causing a stack overflow as the "verify_meta_layers" function recursed without end. I'm honestly impressed OSS-Fuzz found that.
Lastly, because cJSON now has a few new features, I cleaned up the manifest parsing and printing functions to make use of it. cJSON_GetStringValue is very handy for reading strings that have expected values (like file format version) preventing an allocation. cJSON_ArrayForEach allows for linear iteration of arrays instead of the quadratic iteration that was occuring before. And cJSON_PrintPreAllocated pairs very well with
VkLayerProperties&VkExtensionPropertiesto prevent 2 more allocations.