Random results from MagickIdentifyImageType() for PNGs with palette and alpha

[canavan]

For PNGs with palette and alpha, the reuslt of MagickIdentifyImageType() randomly varies between 5 and 7. Tested with ImageMagick 7.0.1-3 and ImageMagick 7.0.1-6. A small test program is below, it fails with any image (with alpha) generated by pngquant; the image of a motorcycle on the right on https://pngquant.org/ is suitable to reproduce the problem.

valgrind says:

Code: Select all

==17931== Memcheck, a memory error detector
==17931== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==17931== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==17931== Command: /home/canavan/src/imagemagick_example
==17931== 
>/tmp/bad-palette-alpha.png<
==17931== Conditional jump or move depends on uninitialised value(s)
==17931==    at 0x56104D2: IsPixelInfoEquivalent (pixel-accessor.h:521)
==17931==    by 0x56104D2: CheckImageColors.part.2 (histogram.c:718)
==17931==    by 0x557B8A2: IdentifyImageType (attribute.c:827)
==17931==    by 0x400BC3: fooImage (in /home/canavan/src/imagemagick_example)
==17931==    by 0x400C99: main (in /home/canavan/src/imagemagick_example)
==17931== 
type: 7 /tmp/bad-palette-alpha.png
>/tmp/bad-palette-alpha.png<
type: 7 /tmp/bad-palette-alpha.png
>/tmp/bad-palette-alpha.png<
type: 7 /tmp/bad-palette-alpha.png
>/tmp/bad-palette-alpha.png<
type: 7 /tmp/bad-palette-alpha.png
>/tmp/bad-palette-alpha.png<
type: 7 /tmp/bad-palette-alpha.png
>/tmp/bad-palette-alpha.png<
type: 7 /tmp/bad-palette-alpha.png
>/tmp/bad-palette-alpha.png<
type: 5 /tmp/bad-palette-alpha.png
failed: /tmp/bad-palette-alpha.png exp: 7 act: 5
==17931== 
==17931== HEAP SUMMARY:
==17931==     in use at exit: 0 bytes in 0 blocks
==17931==   total heap usage: 12,537 allocs, 12,537 frees, 22,753,712 bytes allocated
==17931== 
==17931== All heap blocks were freed -- no leaks are possible
==17931== 
==17931== For counts of detected and suppressed errors, rerun with: -v
==17931== Use --track-origins=yes to see where uninitialised values come from
==17931== ERROR SUMMARY: 1047576 errors from 1 contexts (suppressed: 0 from 0)

Code: Select all

#include <MagickWand/MagickWand.h>
#include <stdio.h>
#include <stdlib.h>

const char * images[] = {
	"/tmp/Ducati_side_shadow-fs8.png",
	NULL};

const int itypes[] = { 7, 7, 7 };

int fooImage(char* inFile)
{
#define ThrowWandException(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); \
}

  MagickBooleanType
    status;

  MagickWand
    *magick_wand;
  ImageType
    imageType;

  /*
    Read an image.
  */
  MagickWandGenesis();
  magick_wand=NewMagickWand();
  status=MagickReadImage(magick_wand, inFile);
  if (status == MagickFalse)
    ThrowWandException(magick_wand);
  MagickResetIterator(magick_wand);
  imageType = MagickIdentifyImageType(magick_wand);
  fprintf(stderr, "type: %i %s\n", (int)imageType, inFile);
  magick_wand=DestroyMagickWand(magick_wand);
  MagickWandTerminus();
  return (int)imageType;
}

int main(int argc,char **argv)
{
  const char * inFile;
  int i, t;
  if (argc != 1)
    {
      (void) fprintf(stdout,"Usage: %s <image>\n",argv[0]);
      exit(0);
    }
  
  while (1) 
  for (i=0, inFile = images[i];  inFile != NULL; i++, inFile = images[i]) {
    t = fooImage((char *)inFile);
    if (t != itypes[i]) {
       fprintf(stderr, "failed: %s exp: %i act: %i\n", inFile, itypes[i], t);
       return(1);
    }
  }
  return(0);
}

[glennrp]

Aren't you simply overflowing your own "itypes[]" array, when i exceeds 3?

EDIT: never mind, I see you are restarting i==0 each time around your loop.

[snibgo]

As I understand it (I could be wrong), MagickWandGenesis() and MagickWandTerminus() should be called only once per program.

[canavan]

As I understand it (I could be wrong), MagickWandGenesis() and MagickWandTerminus() should be called only once per program.

Calling those just once doesn't change the behaviour for me. Does that change have any measurable results for you?

If the API is sane, it shouldn't matter how often MagickWandGenesis() and MagickWandTerminus() are called, as long as they are always called pairwise in that order, and even that appears to be an unnecessary restriction. I've originally observed the random results from MagickIdentifyImageType() in a PHP program that uses imagick, and that again calls Genesis in module_init(), i.e. at most once for each process.

[alex.schneider]

The following patch proposes a fix for this problem. It does two things:
* memset() to prevent "Conditional jump…" (see valgrind output above)
* copy all data from the PixelInfo struct, not just the colors

Could someone please review (and integrate) the code?

Code: Select all

diff --git a/MagickCore/histogram.c b/MagickCore/histogram.c
index 32294ae..ea7059c 100644
--- MagickCore/histogram.c
+++ MagickCore/histogram.c
@@ -725,9 +725,11 @@ static MagickBooleanType CheckImageColors(const Image *image,
           /*
             Add this unique color to the color list.
           */
-          if (node_info->number_unique == 0)
+          if (node_info->number_unique == 0) {
             node_info->list=(PixelInfo *) AcquireMagickMemory(
               sizeof(*node_info->list));
+            ResetMagickMemory(node_info->list, 0, sizeof(*node_info->list));
+          }
           else
             node_info->list=(PixelInfo *) ResizeQuantumMemory(node_info->list,
               (size_t) (i+1),sizeof(*node_info->list));
@@ -738,6 +740,7 @@ static MagickBooleanType CheckImageColors(const Image *image,
                 image->filename);
               break;
             }
+          GetPixelInfo(image,&node_info->list[i]);
           node_info->list[i].red=(double) GetPixelRed(image,p);
           node_info->list[i].green=(double) GetPixelGreen(image,p);
           node_info->list[i].blue=(double) GetPixelBlue(image,p);

[canavan]

That patch works for me. Any chance to get it into future ImageMagick releases?

[dlemstra]

We already added a different patch for this: http://git.imagemagick.org/repos/ImageM ... 38a72e156a

[canavan]

Thanks. I've verified that your patch fixes the issue I've described here.