Fix multiple memory safety issues detected by ASan/UBSan/LeakSan#1171
Fix multiple memory safety issues detected by ASan/UBSan/LeakSan#1171ShadowRider09 wants to merge 1 commit intomltframework:masterfrom
Conversation
Fix null pointer dereference in filter_avcolour_space.c when profile is NULL Fix Pango/Fontconfig memory leak in producer_pango.c by adding instance counting Fix null pointer dereferences in producer_colour.c (two locations) Fix null pointer dereference in mlt_producer_set_in_and_out Fix null pointer dereference in filter_movit_convert.cpp Fix double-free in producer_xml.c by properly handling stack state All fixes have been verified with fuzzing tests. See FIXES_REPORT.md for details, and all of the crash is in the mlt/fuzz/fixed_crashes
420ae39 to
ef9790a
Compare
ddennedy
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I only request some minor easy things to change.
| if (profile) { | ||
| if (*width <= 0) | ||
| *width = profile->width; | ||
| if (*height <= 0) | ||
| *height = profile->height; | ||
| } else { | ||
| if (*width <= 0) | ||
| *width = 16; | ||
| if (*height <= 0) | ||
| *height = 16; | ||
| } |
There was a problem hiding this comment.
| if (profile) { | |
| if (*width <= 0) | |
| *width = profile->width; | |
| if (*height <= 0) | |
| *height = profile->height; | |
| } else { | |
| if (*width <= 0) | |
| *width = 16; | |
| if (*height <= 0) | |
| *height = 16; | |
| } | |
| if (*width <= 0) | |
| *width = profile ? profile->width : 16; | |
| if (*height <= 0) | |
| *height = profile ? profile->height : 16; |
| mlt_properties_set_double(properties, "aspect_ratio", mlt_profile_sar(profile)); | ||
| mlt_properties_set_int(properties, "meta.media.width", profile->width); | ||
| mlt_properties_set_int(properties, "meta.media.height", profile->height); | ||
| double sar = profile ? mlt_profile_sar(profile) : 1.0; |
There was a problem hiding this comment.
| double sar = profile ? mlt_profile_sar(profile) : 1.0; | |
| double sar = profile ? mlt_profile_sar(profile) : 1.0; |
| pthread_mutex_lock(&pango_mutex); | ||
| if (fontmap == NULL) | ||
| fontmap = (PangoFT2FontMap *) pango_ft2_font_map_new(); | ||
| pango_instance_count++; |
There was a problem hiding this comment.
| pango_instance_count++; | |
| pango_instance_count++; |
Please use clang-format v14 to prevent formatting issues. We have a build target for that.
|
Some of these checks are good. But the profile checks add a lot of noise to the code. If a service was not initialized with a profile, then the developer is not using the API correctly and we should let them know right away - not mask it and make it more difficult to discover later. How would you feel about adding assert checks for profiles before using them? That would add less conditional checks to the code and give the developer the earliest warning that something is wrong. |
|
@ShadowRider09 could you provide the commands you used to discover these issues? We might consider to put them to CI to prevent regressions... |
#include <stdint.h> #include "mlt.h" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { } |
fixed_crashes.zip