Legacy ImageMagick Discussions Archive

Hello,

The ReadProfile() function in coders/jpeg.c deals with a possible previous profile in the image:

previous_profile=GetImageProfile(image,name);
if (previous_profile != (const StringInfo *) NULL)
{
SetStringInfoLength(profile,GetStringInfoLength(profile)+
GetStringInfoLength(previous_profile));
(void) memcpy(GetStringInfoDatum(profile),GetStringInfoDatum(profile)+
GetStringInfoLength(previous_profile),GetStringInfoLength(profile));
(void) memcpy(GetStringInfoDatum(profile),
GetStringInfoDatum(previous_profile),
GetStringInfoLength(previous_profile));
}

The first memcpy() causes an access violation, by reading non-allocated memory.

To understand, using abbreviated variables, this code is equivalent to :

// ptr is a buffer of l bytes (stands for profile->datum, after the resize)
// old is a buffer of l_old bytes (stands for previous_profile->datum)
memcpy(ptr, ptr + l_old, l) <== overrun of l_old bytes !!!
memcpy(ptr, old, l_old)


As I am not familiar with the ImageMagick source code, I cannot understand what it i supposed to do. However :

The first memcpy() could be:
memcpy(ptr + l_old, ptr, l - l_old) if the previous profile has to be placed at the beginning

Or, if the previous profile has to be appended at the end, the both memcpy() should replace by only one:
memcpy(ptr + l_old, old, l_old)


This problem seems to appear in version 6.7.1 (more precisely in the revision r4987), after the removal of ConcatenateStringInfo() call and to be platform independant.

I have tested on W7 64 bit, with Version: ImageMagick 6.7.2-0 2011-08-24 Q16 and the last Windows source code (6.7.2-3) compiled with VC++ 2010.

The command I use is : convert P1030267.JPG -resize "x1080>" a.jpg

One of my pictures that causes the problem : http://imageshack.us/photo/my-images/16/p1030267rc.jpg/ (taken with a Panasonic Lumix FX40 and edited with Picasa one year ago).


Best regards,
René

We can reproduce the problem you posted and have a patch. Look for it in the next point release of ImageMagick. Thanks.

First of all, if someone should be thanked, it's you !

But I still don't understand why you copy a buffer twice at the same location (profile->datum). The first argument of memcpy() is the destination pointer, not the source one.

Would you rather write, in http://trac.imagemagick.org/browser/Ima ... ers/jpeg.c line 679 : (memmove() handles overlaps, but not memcpy()).

Regards,
rené

We added your patch to the Subversion trunk. It will be available by sometime tomorrow. Thanks.

Hello,

the patch has been applied into the trunk, but the (automatic?) merge into branch 6.7.2 has failed :

if you compare
http://trac.imagemagick.org/browser/Ima ... peg.c#L646
and
http://trac.imagemagick.org/browser/Ima ... peg.c#L671
you'll see that the call to SetStringInfoLength() has disappeared.

So, convert still hangs on some images

However, I have compiled a 7.0.0 version (from svn revision 5237), it works fine.

Hope this is the only merge/report error...

Regards,
rené

We fixed the copy-paste bug just a few minutes ago. Look for ImageMagick 6.7.2-4 with the fix in just a few hours.

Merci beaucoup