Unitialized memory access corrupting images

[Danack]

*Edit - was "Valgrind reports issues." but there's a repo case now.*

While investigating some issue with PHP Imagick library with Valgrind, it reported that there were some uninitialised variables being used by ImageMagick. These are also shown when running ImageMagick directly from the command line.

Code: Select all

valgrind /usr/local/bin/convert  -size 100x100 gradient:  gradient.jpg

Gives error messages:

==4790== Conditional jump or move depends on uninitialised value(s)
==4790== at 0x4EEF95F: DrawGradientImage (quantum.h:92)
==4790== by 0x4F5AD75: GradientImage (paint.c:488)
==4790== by 0x4FFC5C7: ReadGRADIENTImage (gradient.c:159)
==4790== by 0x4EC83A6: ReadImage (constitute.c:547)
==4790== by 0x4EC958A: ReadImages (constitute.c:853)
==4790== by 0x541888A: ConvertImageCommand (convert.c:615)
==4790== by 0x5499C50: MagickCommandGenesis (mogrify.c:168)
==4790== by 0x400924: main (convert.c:81)
==4790==
==4790== Conditional jump or move depends on uninitialised value(s)
==4790== at 0x4EEF9AB: DrawGradientImage (quantum.h:92)
==4790== by 0x4F5AD75: GradientImage (paint.c:488)
==4790== by 0x4FFC5C7: ReadGRADIENTImage (gradient.c:159)
==4790== by 0x4EC83A6: ReadImage (constitute.c:547)
==4790== by 0x4EC958A: ReadImages (constitute.c:853)
==4790== by 0x541888A: ConvertImageCommand (convert.c:615)
==4790== by 0x5499C50: MagickCommandGenesis (mogrify.c:168)
==4790== by 0x400924: main (convert.c:81)
==4790==
==4790== Conditional jump or move depends on uninitialised value(s)
==4790== at 0x4EB1827: TransformRGBImage (quantum-private.h:410)
==4790== by 0x508D761: WriteJPEGImage (jpeg.c:2121)
==4790== by 0x4EC7B6B: WriteImage (constitute.c:1111)
==4790== by 0x4EC9ECF: WriteImages (constitute.c:1330)
==4790== by 0x541919D: ConvertImageCommand (convert.c:3155)
==4790== by 0x5499C50: MagickCommandGenesis (mogrify.c:168)
==4790== by 0x400924: main (convert.c:81)
==4790==
==4790== Use of uninitialised value of size 8
==4790== at 0x4EB187E: TransformRGBImage (colorspace.c:2356)
==4790== by 0x508D761: WriteJPEGImage (jpeg.c:2121)
==4790== by 0x4EC7B6B: WriteImage (constitute.c:1111)
==4790== by 0x4EC9ECF: WriteImages (constitute.c:1330)
==4790== by 0x541919D: ConvertImageCommand (convert.c:3155)
==4790== by 0x5499C50: MagickCommandGenesis (mogrify.c:168)
==4790== by 0x400924: main (convert.c:81)

This is ImageMagick-6.8.8-9 compiled on `CentOS release 6.4 (Final)

Apologies in advance if reporting potential bugs is not appropriate

cheers
Dan

[Danack]

Hi,

I'm reasonably sure that this issue is causing some image corruption, or at least it's highly coincidental to some image corruption.

I don't have a 100% reproducible test case, but what I'm seeing is a that sometimes there is a small amount of corruption when calling the ImageMagick wand methods to create a gradient through the PHP Imagick library. An example image is:



Which has a pattern of dark dots in the middle of the image where there shouldn't be any.

The PHP code is:

Code: Select all

        $imagick = new \Imagick();
        $imagick->newPseudoImage(400, 400, 'gradient:black-white');
        $alpha = 1;

        $tint = new \ImagickPixel("rgb(100, 0, 100)");
        $opacity = new \ImagickPixel("rgb(128, 128, 128, $alpha)");
        $imagick->tintImage($tint, $opacity);
        $imagick->setImageFormat('png');
        header("Content-Type: image/png");
        echo $imagick->getImageBlob();

I haven't been able to reproduce an image with the error from the command line convert tool, and it does not happen 100% when calling ImageMagick through the PHP Imagick library.


I should have mentioned that this is with ImageMagick 6.8.8-9 and the head version of Imagick https://github.com/mkoppanen/imagick/

[Danack]

I have a reproducible test case for this issue, code is below. Using the debugging function 'mallopt' to initialize any malloc to be non-zero and then generating an image reliably produces images that are pretty obviously reading from uninitialized memory. e.g.

mallopt set to 0, image produced is fine:


mallopt set to 6, image is reading uninitialized memory:


Please note mallopt is only available on Mac/Linux and this was tested on Centos 6. Also, you could just use the valgrind tool to tell you where at least some of the reads of uninitialized memory are occurring.

Code: Select all

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <malloc.h>
#include <wand/MagickWand.h>


void ThrowWandException(MagickWand *wand) {
    char *description; 
    ExceptionType severity; 
    description = MagickGetException(wand, &severity); 
    (void) fprintf(stderr,"%s %s %lu %s\n",GetMagickModule(),description); 
    description=(char *) MagickRelinquishMemory(description); 
    exit(-1); 
}


void makeImage(int number);

int main(int argc,char **argv) {

    int i;

    //http://man7.org/linux/man-pages/man3/mallopt.3.html
    //http://www.gnu.org/software/libc/manual/html_node/Hooks-for-Malloc.html

    for (i=0; i<16 ; i++) {
        mallopt (M_PERTURB, i);
        makeImage(i);
    }

    return (0);
}
    
void makeImage(int number) {
    
    MagickBooleanType status;
    
    MagickWand *pseudo_wand;
    
    char filename[1000];
        
    int columns = 256 + number;
    int rows = 256 + number;

    MagickWandGenesis();

    pseudo_wand = NewMagickWand();
 
    status = MagickSetSize(pseudo_wand, columns, rows);
    if (status == MagickFalse) {
        ThrowWandException(pseudo_wand);
    }

    if (MagickReadImage(pseudo_wand, "gradient:rgb(255,127,255)-rgb(128,0,255)") == MagickFalse) {
        printf("Failed to generate gradient\n");
        ThrowWandException(pseudo_wand);
    }

	status = MagickSetImageFormat(pseudo_wand, "png");

	/* No magick is going to happen */
	if (status == MagickFalse) {
		printf("Failed to set format");
		ThrowWandException(pseudo_wand);
	}

    sprintf(filename, "./output/gradientMemTest_%x.png", number);

    status = MagickWriteImages(pseudo_wand, filename, MagickTrue);

    if (status == MagickFalse) {
        ThrowWandException(pseudo_wand);
    }
        
    DestroyMagickWand(pseudo_wand);
    MagickWandTerminus();
}

[magick]

We cannot reproduce the problem with ImageMagick 6.8.9-4, the current release. We fixed the size of each file to 256x256 and compared each additional generated gradient to the first and thay all returned an RMSE of 0 indicating that they are all exact copies of each other. Inspecting the images does not show any artifacts in the image.

[Danack]

Hi,

I did a little more investigation:

> We fixed the size of each file to 256x256

That does 'fix', or rather hide, the problem. The problem does definitely only manifests itself when the image size is at certain sizes.

I don't have a good C debugging environment set up currently, but doing a lot of printf debugging, I think the problem seemed to lie in the `pixel cache`. That's certainly where the valgrind error messages were originating from

I may be able to get access to a windows box to use Visual Studio at some point - but have you been able to test using valgrind on to see those error messages when the image size is not a nice round number?

cheers
Dan

[Danack]

Hi,

I think I've found the problem - as valgrind suggested there is nothing setting the pixels before they're read when a gradient image is generated.

* ReadGRADIENTImage in gradient.c calls AcquireImage but doesn't set any pixels.

* GradientImage in paint.c - doesn't appear to set any pixels

* DrawGradientImage in draw.c - assumes that the pixels have already been set and does an addition operation for writing the gradient pixels i.e. it calls

Code: Select all

	MagickPixelCompositeOver(&composite,composite.opacity,&pixel,
        pixel.opacity,&pixel);

Which will have surprising results some of the time.


It looks like a fix is to do the following. In ReadGRADIENTImage in gradient.c before the line "status=GradientImage(image,LocaleCompare(imag" add the code to set the background.

Code: Select all

  
  CacheView
      *image_view;
    ssize_t
      y;
      
      MagickPixelPacket background;
  
      GetMagickPixelPacket(image,&background);
  
    status=MagickTrue;
    exception=(&image->exception);
    image_view=AcquireAuthenticCacheView(image,exception);
  #if defined(MAGICKCORE_OPENMP_SUPPORT)
    #pragma omp parallel for schedule(static,4) shared(status) \
      magick_threads(image,image,image->rows,1)
  #endif
    for (y=0; y < (ssize_t) image->rows; y++)
    {
      register IndexPacket
        *restrict indexes;
  
      register PixelPacket
        *restrict q;
  
      register ssize_t
        x; 
  
      if (status == MagickFalse)
        continue;
      q=QueueCacheViewAuthenticPixels(image_view,0,y,image->columns,1,exception);
      if (q == (PixelPacket *) NULL)
        {
          status=MagickFalse;
          continue;
        }
      indexes=GetCacheViewAuthenticIndexQueue(image_view);
      for (x=0; x < (ssize_t) image->columns; x++)
      {
        SetPixelPacket(image,&background,q,indexes+x);
        q++;
      }
      if (SyncCacheViewAuthenticPixels(image_view,exception) == MagickFalse)
        status=MagickFalse;
    }
    image_view=DestroyCacheView(image_view);
    

It removes all valgrind reports of issues for the test program, and also makes the corruption go away.

[magick]

We can reproduce the problem you posted and have a patch in ImageMagick 6.8.9-7 Beta available by sometime tomorrow. Thanks.