#2 trying to read some test files causes segfault

I have compiled and installed this pixbuf loader. But in case where it fails to read (such as your test samles advanced.jp2, complex.jp2 and weird.jpm), causes segfault and crashes the application.

The problem is in util_destroy() function. when something fails, util_destroy() is called and it fails to free data creating segfault.

Though I dont see anything wrong in that function, but there is a issue related to pointer.

Thanks for reporting, I know it's cliché but it doesn't crash for me:

[0/1] Running all tests.
 1/11 minimal          OK             0.02s
 2/11 basic            OK             0.01s
 3/11 relax            OK             0.02s
 4/11 codestream       OK             0.05s
 5/11 codestream_mono  OK             0.02s
 6/11 codestream_color OK             0.03s
 7/11 advanced         OK             0.19s
 8/11 complex          OK             0.24s
 9/11 normal           OK             0.07s
10/11 large            OK             0.53s
11/11 cmyk             OK             0.12s

Ok:                 11  
Expected Fail:      0   
Fail:               0   
Unexpected Pass:    0   
Skipped:            0   
Timeout:            0 

What version of OpenJPEG are you using? Also what distro, if on GNU/Linux?

I remember experiencing segfaults on older versions of OpenJPEG, one might have to build from the latest commit as it might include a fix. Try doing that then rebuilding, worth a shot.

If that doesn't work I'll need a backtrace, to do that you can run

meson test --gdb

Then hit Q and enter repeatedly until it segfaults and type "bt" to get the backtrace, paste that here :-)

Edit: also, weird.jpm isn't supported by OpenJPEG, that one's not supposed to work at all, hence no test for it.

My system is Debian Buster. and openjpeg version is v2.3

If your test is successful then openjpeg version is responsible for failure to decode. But here the problem is weird. when somethng fails it calls util_destroy() to free stream, codec and image. and it causes segfault. But if i remove util_destroy() calls and use goto statement instead, it successfully frees memory and does not crash.

such as if in gdk_pixbuf__jp2_image_load() function i use code like this...

    if(!opj_decode(codec, stream, image) && opj_end_decompress(codec, stream))
    {
        g_set_error(error, GDK_PIXBUF_ERROR, GDK_PIXBUF_ERROR_FAILED, "Failed to decode the image");
        goto end;
    }

and then at the end of this function...

end:
    if(stream != NULL)
    {
        opj_stream_destroy(stream);
    }
    if(codec != NULL)
    {
        opj_destroy_codec(codec);
    }
    if(image != NULL)
    {
        opj_image_destroy(image);
    }
    return pixbuf;

It causes no segfault. I dont understand why util_destroy() causes segfault.

Installed debian buster in a VM and built. This is the backtrace that happens:

Starting program: /home/dev/jp2-pixbuf-loader/build/tests/complex 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

** (process:8720): WARNING **: 08:35:12.940: /home/dev/jp2-pixbuf-loader/tests/complex.jp2
[New Thread 0x7ffff7192700 (LWP 8724)]

Thread 1 "complex" received signal SIGSEGV, Segmentation fault.
0x00007ffff72af24f in ?? () from /lib/x86_64-linux-gnu/libopenjp2.so.7
(gdb) bt
#0  0x00007ffff72af24f in  () at /lib/x86_64-linux-gnu/libopenjp2.so.7
#1  0x00007ffff72b4560 in  () at /lib/x86_64-linux-gnu/libopenjp2.so.7
#2  0x00007ffff7299837 in opj_stream_destroy () at /lib/x86_64-linux-gnu/libopenjp2.so.7
#3  0x00007ffff7fc35b5 in util_destroy (stream=0x5555555689e0, codec=0x555555568960, image=0x55555557d330) at ../src/util.h:118
#4  0x00007ffff7fc4fce in gdk_pixbuf__jp2_image_load (fp=0x555555559260, error=0x7fffffffe050) at ../src/io-jp2.c:100
#5  0x00007ffff7fa0cf2 in gdk_pixbuf_new_from_file () at /lib/x86_64-linux-gnu/libgdk_pixbuf-2.0.so.0
#6  0x000055555555525b in main (argc=1, argv=0x7fffffffe168) at ../tests/complex.c:30

As you can see, the segfault does not happen in util_destroy, but in opj_stream_destroy, a function from OpenJPEG.

To fix this, you will have to build from latest commit:

git clone https://github.com/uclouvain/openjpeg
cd openjpeg
mkdir build
cd build
cmake -G "Unix Makefiles" ..
make
sudo make install

rebuild jp2-pixbuf-loader, now it works.

I think in your code, some how wrong pointer is passed to opj_stream_destroy() via util_destroy(). hence openjpeg can not free.

But if I use goto statement , then segfault does not happen. can you explain this?

Please see gdk_pixbuf__jp2_image_load() function what i have changed.

static GdkPixbuf *gdk_pixbuf__jp2_image_load(FILE *fp, GError **error)
{
    int codec_type;
    GdkPixbuf *pixbuf = NULL;
    opj_codec_t *codec = NULL;
    opj_image_t *image = NULL;
    opj_stream_t *stream = NULL;
    opj_dparameters_t parameters;

    opj_set_default_decoder_parameters(&parameters);

    stream = util_create_stream(fp);
    if(!stream)
    {
        g_set_error(error, GDK_PIXBUF_ERROR, GDK_PIXBUF_ERROR_FAILED, "Failed to create stream fom file");
        goto end;
    }

    codec_type = util_identify(fp);
    if(codec_type < 0)
    {
        g_set_error(error, GDK_PIXBUF_ERROR, GDK_PIXBUF_ERROR_FAILED, "Unknown filetype!");
        goto end;
    }
    codec = opj_create_decompress(codec_type);

    #if DEBUG == TRUE
        opj_set_info_handler(codec, info_callback, 00);
        opj_set_warning_handler(codec, warning_callback, 00);
        opj_set_error_handler(codec, error_callback, 00);
    #endif

    if(!opj_setup_decoder(codec, &parameters))
    {
        g_set_error(error, GDK_PIXBUF_ERROR, GDK_PIXBUF_ERROR_FAILED, "Failed to setup decoder");
        goto end;
    }

    if(!opj_codec_set_threads(codec, 1))
    {
        g_set_error(error, GDK_PIXBUF_ERROR, GDK_PIXBUF_ERROR_FAILED, "Failed to set thread count");
        goto end;
    }

    if(!opj_read_header(stream, codec, &image))
    {
        g_set_error(error, GDK_PIXBUF_ERROR, GDK_PIXBUF_ERROR_FAILED, "Failed to read header");
        goto end;
    }

    if(!opj_decode(codec, stream, image) && opj_end_decompress(codec, stream))
    {
        g_set_error(error, GDK_PIXBUF_ERROR, GDK_PIXBUF_ERROR_FAILED, "Failed to decode the image");
        goto end;
    }

    // Get components and colorspace needed to convert to RGB

    int components = -1;
    COLOR_SPACE colorspace = -1;
    if(!color_info(image, &components, &colorspace))
    {
        g_set_error(error, GDK_PIXBUF_ERROR, GDK_PIXBUF_ERROR_FAILED, "Unsupported colorspace");
        goto end;
    }

    // Allocate space for GdkPixbuf RGB

    guint8 *data = g_malloc(sizeof(guint8) * (int) image->comps[0].w * (int) image->comps[0].h * components);

    // Convert image to RGB depending on the colorspace

    switch(colorspace)
    {
        case COLOR_SPACE_RGB:
            color_convert_rgb(image, data);
            break;
        case COLOR_SPACE_GRAY:
            color_convert_gray(image, data);
            break;
        case COLOR_SPACE_GRAY12:
            color_convert_gray12(image, data);
            break;
        case COLOR_SPACE_SYCC420:
            color_convert_sycc420(image, data);
            break;
        case COLOR_SPACE_SYCC422:
            color_convert_sycc422(image, data);
            break;
        case COLOR_SPACE_SYCC444:
            color_convert_sycc444(image, data);
            break;
        case COLOR_SPACE_CMYK:
            color_convert_cmyk(image, data);
            break;
    }

    pixbuf = gdk_pixbuf_new_from_data(
        (const guchar*) data,                 // Actual data. RGB: {0, 0, 0}. RGBA: {0, 0, 0, 0}.
        GDK_COLORSPACE_RGB,                   // Colorspace (only RGB supported, lol, what's the point)
        (components == 4 || components == 2), // has_alpha
        8,                                    // bits_per_sample (only 8 bit supported, again, why even bother)
        (int) image->comps[0].w,              // width
        (int) image->comps[0].h,              // height
        util_rowstride(image, components),    // rowstride: distance in bytes between row starts
        free_buffer,                          // destroy function
        NULL                                  // closure data to pass to the destroy notification function
    );
end:
    if(stream != NULL)
    {
        opj_stream_destroy(stream);
    }
    if(codec != NULL)
    {
        opj_destroy_codec(codec);
    }
    if(image != NULL)
    {
        opj_image_destroy(image);
    }
    return pixbuf;
}

I see it now.

For some reason util_destroy is called like this:

util_destroy(codec, stream, image);

but util_destroy is defined like this:

void util_destroy(opj_stream_t* stream, opj_codec_t *codec, opj_image_t *image)

codec, stream, image vs stream, codec, image

The wrong pointer is being passed to the opj_stream_destroy function.

Fixed.