[Pkg-gmagick-im-team] Bug#773980: libmagickcore-6.q16-2: 8:6.8.9.9-4 breaks images in GNU Emacs 24 (displayed as single colour rectangles)

[Adam Sjøgren]

Bastien writes:

> The problematic commit is http://trac.imagemagick.org/changeset/17297
> and it is needed from a security point of view :S

I think the change of xpm.c looks odd. The function in play is changed
to this:

static size_t CopyXPMColor(char *destination,const char *source,size_t length)
{
  register const char
    *p;

  p=source;
  while (--length && (*p != '\0'))
    *destination++=(*p++);
  *destination='\0';
  return((size_t) (p-source));
}

The patch changed "length--" into "--length" in the while(). So now it
copies one char less than length.

Which means that if you call CopyXPMColor() with length 1, then nothing
is copied, if you call it with 10, then 9 chars are copied...

That sounds wrong, right?

If the problem the patch was trying to solve is that a '\0' is written
outside the destination when length is MaxTextExtent, then it looks like
the correct fix is to change the three calls to CopyXPMColor() like
this:

--- xpm.c	2014-12-27 23:21:38.356159705 +0100
+++ xpm.c.changed	2014-12-28 00:05:28.792472158 +0100
@@ -385,7 +385,7 @@
 
     p=next;
     next=NextXPMLine(p);
-    (void) CopyXPMColor(key,p,MagickMin((size_t) width,MaxTextExtent));
+    (void) CopyXPMColor(key,p,MagickMin((size_t) width,MaxTextExtent-1));
     status=AddValueToSplayTree(xpm_colors,ConstantString(key),(void *) j);
     /*
       Parse color.
@@ -400,7 +400,7 @@
           break;
         if (next != (char *) NULL)
           (void) CopyXPMColor(target,q,MagickMin((size_t) (next-q),
-            MaxTextExtent));
+            MaxTextExtent-1));
         else
           (void) CopyMagickString(target,q,MaxTextExtent);
         q=ParseXPMColor(target,MagickFalse);
@@ -443,7 +443,7 @@
         indexes=GetAuthenticIndexQueue(image);
         for (x=0; x < (ssize_t) image->columns; x++)
         {
-          p+=CopyXPMColor(key,p,MagickMin(width,MaxTextExtent));
+          p+=CopyXPMColor(key,p,MagickMin(width,MaxTextExtent-1));
           j=(ssize_t) GetValueFromSplayTree(xpm_colors,key);
           if (image->storage_class == PseudoClass)
             SetPixelIndex(indexes+x,j);

I have no clue about the conventions of the source code (i.e. is
MaxTextExtent with or without '\0'?)

The commit you found does not explain the problem it is fixing (no
commit message), so I don't quite know how to check if my guess solves
the original problem :-/

I have tried reverting the "length--" to "--length" change in
0038-Fix-another-crash-in-pnm-and-xpm-parser.patch, built a new package
locally and installed it, and I can confirm that it is the cause of the
regression (i.e. also in the emacs case).


  Best regards,

    Adam

-- 
 "Subdued flamboyance"                                        Adam Sjøgren
                                                         asjo at koldfront.dk