memory overrun in coders/jpeg.c

[rene-d]

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é

[magick]

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

[rene-d]

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 :

Code: Select all

	      (void) memmove(GetStringInfoDatum(profile) + GetStringInfoLength(previous_profile),
            GetStringInfoDatum(profile),
            length);

(memmove() handles overlaps, but not memcpy()).

Regards,
rené

[magick]

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

[rene-d]

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é

[magick]

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.

[rene-d]

Merci beaucoup