MagickRemoveImageProfile returns freed memory in 6.3.5-2, -3

[mi]

The following piece of code now causes a crash on FreeBSD, where malloc/free track freed memory:

Code: Select all

profile = MagickRemoveImageProfile(wandPtr, (const char*)name, &length);
puts(profile);
MagickRelinquishMemory(profile);

According to the debugger, both profile and name are (or recently were) valid:

Code: Select all

(gdb) p profile
$4 = (unsigned char *) 0xb88000 "foo"
(gdb) p name
$5 = 0x5d9730 "icc"

But, according to FreeBSD's free, the memory pointed to by profile has already been freed...

This worked with 6.3.3-5 -- not sure, when it broke Just checked with 6.3.5-3 -- still broken.

[magick]

We have a patch for the problem you reported. It will be available in ImageMagick 6.3.5-3 Beta sometime tomorrow.

[mi]

Ok, thanks. This calls into question your testing methods, however

I encountered this crash by running TclMagick's self-tests. Does ImageMagick bundle any (non-)regression tests with the distribution, that anyone can run after building -- to quickly detect possible developer bugs as well as any local miscompilations?

Thanks!

[magick]

We run over 1000 regression tests before we release ImageMagick. Unfortunately MagickRemoveImageProfile() was not one of the tests, however, we will include a unit test for it in the next release.

[mi]

Can one download the regression tests? I'd like to include them in the routine build of ImageMagick -- to catch local miscompilations and other problems.

I doubt, you test on platforms as "exotic" as FreeBSD/amd64...

[magick]

The regression tests are part of the source distribution. Type

• make check

to run them.

I doubt, you test on platforms as "exotic" as FreeBSD/amd64.

FreeBSD is far from exotic and yes we run the regression tests under FreeBSD and Linux and Solaris and Mac OS X and HPUX and AIX and others.

[mi]

In many cases, the profile returned by the function is not really needed by the caller and ends up being passed to MagickRelinquishMemory right away.

By specifying the length-pointer as NULL, the caller could indicate their disinterest:

Code: Select all

--- wand/magick-image.c  2007-07-16 20:49:49.000000000 -0400
+++ wand/magick-image.c  2007-07-20 09:43:29.000000000 -0400
@@ -7714,6 +7714,8 @@
       return((unsigned char *) NULL);
     }
-  *length=0;
   profile=RemoveImageProfile(wand->images,name);
+  if (length == NULL)
+    return NULL;
+  *length=0;
   if (profile != (StringInfo *) NULL)
     *length=GetStringInfoLength(profile);

[mi]

FreeBSD (on i386) is not exotic, but amd64 has not seen regression tests in a while. When I try to do `make check' here, I get:

Code: Select all

wand/wandtest.c: In function `main':
wand/wandtest.c:72: error: `description' undeclared (first use in this function)
wand/wandtest.c:72: error: (Each undeclared identifier is reported only once
wand/wandtest.c:72: error: for each function it appears in.)
wand/wandtest.c: At top level:
wand/wandtest.c:417: error: conflicting types for 'magick_wand'
wand/wandtest.c:388: error: previous declaration of 'magick_wand' was here
wand/wandtest.c:417: warning: initialization makes integer from pointer without a cast
wand/wandtest.c:417: error: initializer element is not constant
wand/wandtest.c:417: warning: data definition has no type or storage class
wand/wandtest.c:418: error: syntax error before "void"
wand/wandtest.c:400: error: register name not specified for 'i'

This is easy to pach -- just remove the blank lines in the ``#define ThrowAPIException''...
I can't see, how it could possibly have worked anywhere, though, which means, you don't run these tests as often as you upload tar-balls into beta/ subdirectory of the ftp-site.

Having fixed the macro, I can the actual tests, and get a whole lot failures and segfaults (mostly in FPX, which is known to be severely broken on 64-bit platforms), but not only...

I wish you really were testing on FreeBSD/amd64...

[magick]

We do our regression tests on an AMD64 Redhat box. We'll try to locate an AMD64 FreeBSD box and run the regression tests. We are confident they will run without complaint.

[mi]

I hate to be rude, but you are not running them often enough. FreeBSD or RedHat -- gcc would not have compiled the wandtest.c as it is currently shipped (two blank lines inserted into the ThrowAPIException-macro).

This means, uploads to the ftp-site happen more often, than attempts at "make check".

Don't kill yourself (yet) trying to find a FreeBSD/amd64 box. Just run every beta through "make check" on the machines you have already -- something you demonstrably do not do at the moment...

[magick]

Our ImageMagick release build script cannot complete unless the regression tests run and complete successfully. Not sure why its failing for you but the important thing is that it works for us.

[mi]

Our ImageMagick release build script cannot complete unless the regression tests run and complete successfully.

Can you post a link to the build log of the currently posted beta/ImageMagick-6.3.5-3.tar.bz2?

Here is my build log

Not sure why its failing for you but the important thing is that it works for us.

Come on. You are writing this for me - the user - not for yourself...

[magick]

Well that's the problem. Did you notice ImageMagick 6.3.5-3 is in the Beta directory? Its not released so there may be some problems with it. Blessed regression testing is only valid for the ImageMagick release versions.

[mi]

Did you notice ImageMagick 6.3.5-3 is in the Beta directory? Its not released so there may be some problems with it. Blessed regression testing is only valid for the ImageMagick release versions.

Throughout this thread I kept repeating, that I used the 6.3.5-3 version, which, you are right, is currently in beta.

I also wrote:

I can't see, how it could possibly have worked anywhere, though, which means, you don't run these tests as often as you upload tar-balls into beta/ subdirectory of the ftp-site.

In other words, I know, it is "beta", but I still don't understand, why you don't run through regression tests even the beta code... It is (or should be) much easier to have computer(s) do the work, than to wrangle with the upset users on the forums...

You are right that "there may be some problems with it", but that should not include any of the problems, for which test-cases have already been developed. What's the point of uploading even a beta, if it has not passed the automated regression tests?

[magick]

We do run the regressions on Beta release-- just not as diligently as a release. Just a few days ago we added regression tests for Wand profiles, properties, and options and it looks like there is a version skew between the Beta and our development area. We'll get that fixed and release a new Beta by tomorrow.