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

Post any defects you find in the released or beta versions of the ImageMagick software here. Include the ImageMagick version, OS, and any command-line required to reproduce the problem. Got a patch for a bug? Post it here.
Post Reply
canavan
Posts: 23
Joined: 2013-02-18T10:12:03-07:00
Authentication code: 6789

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

Post by 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);
}
User avatar
glennrp
Posts: 1147
Joined: 2006-04-01T08:16:32-07:00
Location: Maryland 39.26.30N 76.16.01W

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

Post by 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
Posts: 12159
Joined: 2010-01-23T23:01:33-07:00
Authentication code: 1151
Location: England, UK

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

Post by snibgo »

As I understand it (I could be wrong), MagickWandGenesis() and MagickWandTerminus() should be called only once per program.
snibgo's IM pages: im.snibgo.com
canavan
Posts: 23
Joined: 2013-02-18T10:12:03-07:00
Authentication code: 6789

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

Post by 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
Posts: 3
Joined: 2016-06-03T04:36:05-07:00
Authentication code: 1151

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

Post by 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
Posts: 23
Joined: 2013-02-18T10:12:03-07:00
Authentication code: 6789

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

Post by canavan »

That patch works for me. Any chance to get it into future ImageMagick releases?
User avatar
dlemstra
Posts: 1570
Joined: 2013-05-04T15:28:54-07:00
Authentication code: 6789
Contact:

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

Post by dlemstra »

We already added a different patch for this: http://git.imagemagick.org/repos/ImageM ... 38a72e156a
.NET + ImageMagick = Magick.NET https://github.com/dlemstra/Magick.NET, @MagickNET, Donate
canavan
Posts: 23
Joined: 2013-02-18T10:12:03-07:00
Authentication code: 6789

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

Post by canavan »

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