Bug in j2k_decode()
CarloWood
Please let me know if this reached the right people.
In j2k_decode(opj_j2k_t *j2k, opj_cio_t *cio, opj_codestream_info_t
*cstr_info)
in libopenjpeg/j2k.c
we have
image = opj_image_create0();
in the same function, before any initialization, we have
calls to
opj_image_destroy(image);
opj_image_destroy reads and uses the uninitialized
value of image->comps and when non-zero, image->numcomps.
If the latter is also non-zero, undefined behavior follows (crash,
or freeing invalid pointers).
The bug is therefore that image->comps is not set to NULL
directly after the line image = opj_image_create0();
This bug was introduced, according to the ChangeLog, here:
October 18, 2007
* [FOD] Changed the ROI parameter in the image_to_j2k codec to make it
correspond to the documentation (i.e. -ROI c=0,U=25)
* [FOD] Patch from Callum Lewick. Memset patch.
The main idea of the patch is that currently
opj_malloc clears all allocations, which unnecessarily
dirties up the cache and eats bandwidth. This patch
makes it no longer do so, and I've painstakingly determined which
allocations actually need
to be cleared and changed them to use opj_calloc()
instead. I previously tried to just get rid of the opj_*alloc wrappers
but that
idea was nixed, so this time I'm trying it with
macros. I also put in a gcc pragma that helps enforce their use. Which
got messy. :) It caught a
few places they weren't used but it also revealed that
the mj2 tools are not very cleanly separated from the library. It
includes all the
internal headers, but it wasn't using the malloc
wrappers. I figured the binaries should be "external" and have minimal
knowledge of the
internals of the library. I patched them to not
include opj_includes.h, and include only the internal headers they
actually need. However,
they're using the opj_clock() function, which is in
with the malloc wrappers. So I decided to move the malloc wrappers to
their own header.
But mj2.c seems to want to be "internal", so I patched
it to use the wrappers. Note that this patch changes the semantics of
opj_malloc, it no longer
clears the memory it allocates. If you need it to be
cleared, you must use opj_calloc instead, or memset it yourself. It is
also somewhat
invasive, please test it extensively. I've been
pounding on it all summer with my test suite, Second Life, and
valgrind, and it checks out clean.
Note that I ran into this WITH a secondlife client, but do not run
into it normally,
only when today I switched to some different grid. The source speak
for itself
though: this is a bug.
Callum Lerwick
> Not sure where to report bugs, but this seems the only way...
> Please let me know if this reached the right people.
>
> In j2k_decode(opj_j2k_t *j2k, opj_cio_t *cio, opj_codestream_info_t
> *cstr_info)
> in libopenjpeg/j2k.c
>
> we have
>
> image = opj_image_create0();
>
> in the same function, before any initialization, we have
> calls to
>
> opj_image_destroy(image);
>
> opj_image_destroy reads and uses the uninitialized
> value of image->comps and when non-zero, image->numcomps.
> If the latter is also non-zero, undefined behavior follows (crash,
> or freeing invalid pointers).
>
> The bug is therefore that image->comps is not set to NULL
> directly after the line image = opj_image_create0();
What version are you looking at? opj_image_create0() should be
handling the initialization, not the caller, which in my copy of
svn528 looks like this:
opj_image_t* opj_image_create0(void) {
opj_image_t *image = (opj_image_t*)opj_calloc(1, sizeof(opj_image_t));
return image;
}
The entire structure is initialized to zero by calloc(). Thus
image->comps should be NULL already when opj_image_create0() returns.
Incidentally, free()ing NULL is a valid no-op, so the null checks are
un-neccessary, opj_image_destroy() can be reduced to this:
void OPJ_CALLCONV opj_image_destroy(opj_image_t *image) {
int i;
if(image->comps) {
/* image components */
for(i = 0; i < image->numcomps; i++) {
opj_image_comp_t *image_comp = &image->comps[i];
opj_free(image_comp->data);
}
opj_free(image->comps);
}
opj_free(image);
}
> Note that I ran into this WITH a secondlife client, but do not run
> into it normally,
> only when today I switched to some different grid. The source speak
> for itself
> though: this is a bug.
Looks correct to me, as of svn528 at least. But I'm getting sleepy so
I might be missing something.
If you can provide a test case image, valgrind will reveal all. Also,
such a test case can go into a test suite to ensure it never breaks
again.
CarloWood
libopenjpeg-2.1.3.0.so
opj_image_create0, in this version, looks like this:
opj_image_t* opj_image_create0(void) {
opj_image_t *image = (opj_image_t*)opj_malloc(sizeof
(opj_image_t));
return image;
}
It uses opj_malloc, not calloc.