Skip to content
Permalink
Browse files

...

  • Loading branch information...
Cristy
Cristy committed Jan 11, 2017
1 parent 37a1710 commit 4493d9ca1124564da17f9b628ef9d0f1a6be9738
Showing with 12 additions and 7 deletions.
  1. +5 −0 ChangeLog
  2. +7 −7 coders/mpc.c
@@ -1,3 +1,8 @@
2017-01-10 6.9.7-4 Cristy <quetzlzacatenango@image...>
* Recognize XML policy closing tags (reference
https://www.imagemagick.org/discourse-server/viewtopic.php?f=3&t=31182).
* Fix memory leak in MPC image format.

2017-01-07 6.9.7-3 Cristy <quetzlzacatenango@image...>
* Release ImageMagick version 6.9.7-3, GIT revision 11280:7d65a81:20170107.

@@ -67,6 +67,7 @@
#include "magick/profile.h"
#include "magick/property.h"
#include "magick/quantum-private.h"
#include "magick/resource_.h"
#include "magick/static.h"
#include "magick/statistic.h"
#include "magick/string_.h"
@@ -841,7 +842,9 @@ static Image *ReadMPCImage(const ImageInfo *image_info,ExceptionInfo *exception)
/*
Create image colormap.
*/
if (AcquireImageColormap(image,image->colors) == MagickFalse)
image->colormap=(PixelPacket *) AcquireQuantumMemory(image->colors+1,
sizeof(*image->colormap));
if (image->colormap == (PixelPacket *) NULL)
ThrowReaderException(ResourceLimitError,"MemoryAllocationFailed");
if (image->colors != 0)
{
@@ -930,12 +933,9 @@ static Image *ReadMPCImage(const ImageInfo *image_info,ExceptionInfo *exception)
if ((image_info->ping != MagickFalse) && (image_info->number_scenes != 0))
if (image->scene >= (image_info->scene+image_info->number_scenes-1))
break;
status=SetImageExtent(image,image->columns,image->rows);
if (status == MagickFalse)
{
InheritException(exception,&image->exception);
return(DestroyImageList(image));
}
if ((AcquireMagickResource(WidthResource,image->columns) == MagickFalse) ||
(AcquireMagickResource(HeightResource,image->rows) == MagickFalse))
ThrowReaderException(ImageError,"WidthOrHeightExceedsLimit");
/*
Attach persistent pixel cache.
*/

3 comments on commit 4493d9c

@mgerstner

This comment has been minimized.

Copy link

replied Jan 24, 2017

For this commit a CVE was assigned on oss-sec:

http://seclists.org/oss-sec/2017/q1/110

Use CVE-2017-5507

However, we fail to understand what kind of memory leak has been fixed here. Can you please elaborate or point us in the right direction? Thank you.

@mikayla-grace

This comment has been minimized.

Copy link

replied Jan 24, 2017

The MPC image format memory maps an existing ImageMagick pixel cache in memory. The previous AcquireImageColormap() method call (prior to this commit), subsequently calls SetImageStorageClass(). This in turn calls SyncImagePixelCache(), which improperly allocates a new pixel cache. Next, the MPC coder calls PersistPixelCache() which overwrites the previous pixel cache causing a memory leak. Now we allocate the image colormap inside the MPC module which avoids the spurious call to SyncImagePixelCache(). Its so simple :-).

Its good to avoid memory leaks, however, we're not sure it makes sense to classify this as a security issue.

@mgerstner

This comment has been minimized.

Copy link

replied Jan 25, 2017

Memory leaks are generally also handled as security issues, because they might allow denial of service attacks for long running network services, for example. Sometimes it seems over-motivated, however :-)

Thank you very much for the insight!

Please sign in to comment.
You can’t perform that action at this time.