oss-fuzz reports stack overflow in libheif #1336
Comments
|
I cannot reproduce this. I've tested at several commits. |
|
Git clones of libheif are reporting recent revisions de48209f29be, 2f470644d34e, and 2595e649e943, but I am not sure how these correspond to test reports. The job metadata says this: and it says that the same reproducer creates crashes for four different job types. afl_asan_graphicsmagick - Stack-overflow |
These are no libheif commit IDs. Let's postpone this one a bit. I am currently doing a lot of internal restructuring. Thus, changes are high that it was just an intermediate commit that had this issue. |
|
The most recent oss-fuzz test run with the problematic test case results in totally different behavior. This "recursive_mutex lock failed: Invalid argument" issue has been seen before: |
|
There is only one There is a new Are you sure that this mutex is in libheif? |
|
GraphicsMagick may (optionally) also use recursive mutexes. But the crash suggests it may be in the C++ library implementation or perhaps in the oss-fuzz run time libraries.
Bob
…On Oct 16, 2024, 9:33 AM, at 9:33 AM, Dirk Farin ***@***.***> wrote:
There is only one `recursive_mutex` in libheif. That is used in the
library initialization to make them thread-safe. However, nothing
changed in that code part during the past months.
There is a new `mutex` (not recursive) introduced in 009c34e for the
decoding progress reporting. Maybe it is related to that? But just from
looking at the source, I do not see any issues.
Are you sure that this mutex is in libheif?
--
Reply to this email directly or view it on GitHub:
#1336 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
|
To be clear Magick::MagickCleanUp::~MagickCleanUp() /src/graphicsmagick/Magick++/lib/Image.cpp does not intentionally use any mutexes. The core GraphicsMagick C library only uses simple mutexes (my recollection of use of recursive mutexes was from maybe 20 years ago). The MagickCleanUp() method calls "extern C" MagickPlusPlusDestroyMagick(), which then invokes the C function DestroyMagick() in the GraphicsMagick library. But we do not see mention of DestroyMagick() in the call stack. Since GraphicsMagick is not using recursive mutexes, it must be some C++ code which is performing static destruction since this is a very static C/C++ program. It is difficult to assure correct ordering while a static C++ program is destroying its objects. |
|
The (only) Lines 88 to 92 in 1e9ccf1 Your error output says that is crashes when it tries to lock it. |
|
I see that your use of a recursive mutex is a C++ thing and it is using a static object, which would be involved with C++ static construction and destruction. Static destruction of C++ objects would occur after main() returns. Unfortunately, linking against, and initializing, so many 3rd-party libraries, has been using more execution time than a fairly substantial operation involving reading/writing an image file. In order to lessen the overhead, GraphicsMagick is currently using lazy initialization of libheif like this: At global scope: And in ReadHEIFImage(), which may be called many times: and in UnregisterHEIFImage(), which should be called once: When Magick++ calls DestroyMagick() then UnregisterHEIFImage() would be called. |
|
I will create a new issue regarding the recursive_mutex destruction issue since there is another oss-fuzz issue open (since May!) specifically about that. |
|
libheif is also doing "reference counting" in heif_init() and heif_deinit(). When you call it several times, it will only initialize it once. Same for destruction. It will only release everything when the number of calls to heif_deinit() matches the number of calls to heif_init(). Thus, I don't think the "heif_initialized" optimization is needed. I'll try replacing the recursive_mutex with an atomic integer counter... |
|
I opened issue 1345 specifically about the recursive_mutex issue. It includes data from a different oss-fuzz report. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
As part of GraphicsMagick oss-fuzz fuzz testing, oss-fuzz has detected an unusual stack overflow in latest libheif code.
The issue will eventually be visible to all at graphicsmagick:coder_AVIF_fuzzer: Stack-overflow in ImageItem_Grid::get_decoder.
The report by oss-fuzz is not very conclusive, but one of the stack traces appears to show that ImageItem_Grid::get_decoder() is making a recursive call and has called itself 240 times. It is possible that additional context due to AddressSanitizer might limit how much can be put on the stack.
This is the file which provokes the issue (with .jpg added to satisfy github):
The text was updated successfully, but these errors were encountered: