Segfault in extractError function

[denisenkom]

Hi all,

I encountered SIGSEGV error in Gst.extractError function, here is full error report file: hs_err_pid32632.log

When a JNA function is invoked with a Structure.ByReference parameter, JNA automatically reads the contents of this structure upon completion of the native function call. This step is crucial for propagating any modifications made by the native function back to a corresponding Java class that mirrors the native structure. The implementation responsible for this process can be found here.

The seg fault happens when extractError function calls g_error_free function which releases memory used by GErrorStruct structure. Since g_error_free function releases memory used by the passed structure and structure is passed by reference, JNA attempts to read from a memory that has been released. This leads to "use-after-free" bug.

I made a fix to prevent JNA from reading structure content after call to g_error_free function: https://github.com/gstreamer-java/gst1-java-core/pull/279

[denisenkom]

Versions:

Java: Corretto-16.0.2.7.1
OS: Darwin 22.6.0
JNA: 5.14.0
gst1-java-core: 1.4.0

[neilcsmith-net]

Thanks! I'll take a proper look in the new year. I'm not sure whether to fix it in the way you have here, although it's probably safer to do so. It might indicate issues in a few other places. I need to check if anywhere else is trying to use volatile to indicate not auto-syncing the field, whereas it only switches off auto-writing.

The other option is probably to use setAutoSynch(false) in the constructors of this struct. The code path via extractError also needs checking for tests, because I think it's missing.

It's interesting that this issue has not been reported before, which might be due to memory allocator differences, or just that it's rare(?)

[denisenkom]

I think an ideal solution would be to have an annotation on that parameter in that function to indicate that this is an input only parameter but JNA does not have such annotation afaik.

Yeah it is interesting, I am able to reproduce this issue with 100% reproducibility rate. And able to confidently confirm that the fix in my pull request works.

[denisenkom]

A friendly reminder. Does this PR require changes?

[neilcsmith-net]

I think it's good, although a test that fail before application and passes after would make it perfect!

However, at the moment we need to make a decision on whether we're going to do any further releases of this library. I will merge if and when we decide to do so.

Thanks!

[denisenkom]

On Mac I am able to reproduce a test crash before fix and a test pass after fix with the following test:

    @Test
    public void testParseLaunchThrowingError() {
        ArrayList<GError> errors = new ArrayList<GError>();
        assertThrows(GstException.class, () -> {
            Gst.parseLaunch("fakesrcc ! fakesinkk", errors);
        });
    }

On Linux I wasn't able to reproduce crash with this test with debug heap enabled so far.

While trying to reproduce I found similar memory issues. I get a crash when running tests on Linux with debug heap enabled (with or without fix applied). I was able to isolate crashing tests to EventTest and QueryTest. This command causes test to crash:

LD_PRELOAD=/lib/x86_64-linux-gnu/libc_malloc_debug.so.0 MALLOC_CHECK_=3 mvn -Dtest='EventTest,QueryTest' test

While this command passes:

LD_PRELOAD=/lib/x86_64-linux-gnu/libc_malloc_debug.so.0 MALLOC_CHECK_=3 mvn -Dtest='!EventTest,!QueryTest' test

Wanted to share those intermediate results. I will spend a bit more time trying to reproduce the original issue on Linux.