bug#28824: 26.0.90; display of pbm images broken?

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

Eli Zaretskii wrote:
>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sun, 15 Oct 2017 18:42:27 +0100
>>
>> Eli can decide if reverting the previous "unsigned char" -> "char"
>> changes is a better fix.
> 
> Looks like going back to unsigned should be cleaner, but I'd like to
> hear what Paul thinks.

Andy's fixes look good to me. Going back to unsigned would result in several 
pointer casts that are more dangerous than converting to unsigned. They can be 
further improved by encapsulating this stuff into a function, and I installed 
the attached (the second one is Andy's other fix, which I installed in his name).

I looked for related bugs in image.c (i.e., caused by my earlier patch) and did 
not find any.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-regression-in-display-of-PPM-images.patch --]
[-- Type: text/x-patch; name="0001-Fix-regression-in-display-of-PPM-images.patch", Size: 2311 bytes --]

From cac318a1fce0a4c8c5c1f81f715c06c49c117592 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 16 Oct 2017 01:14:58 -0700
Subject: [PATCH 1/2] Fix regression in display of PPM images

Problem reported by Roland Winkler (Bug#28824#35).
Based on a patch proposed by Andy Moreton (Bug#28824#38).
* src/image.c (pbm_scan_index): New function.
(pbm_load): Use it to decode raw data correctly when its top bit
is set.
---
 src/image.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/src/image.c b/src/image.c
index cd4901b..335a43e 100644
--- a/src/image.c
+++ b/src/image.c
@@ -5277,6 +5277,25 @@ pbm_scan_number (char **s, char *end)
   return val;
 }
 
+/* Scan an index from *S and return it.  It is a one-byte unsigned
+   index if !TWO_BYTE, and a two-byte big-endian unsigned index if
+   TWO_BYTE.  */
+
+static int
+pbm_scan_index (char **s, bool two_byte)
+{
+  char *p = *s;
+  unsigned char c0 = *p++;
+  int n = c0;
+  if (two_byte)
+    {
+      unsigned char c1 = *p++;
+      n = (n << 8) + c1;
+    }
+  *s = p;
+  return n;
+}
+
 
 /* Load PBM image IMG for use on frame F.  */
 
@@ -5499,7 +5518,8 @@ pbm_load (struct frame *f, struct image *img)
   else
     {
       int expected_size = height * width;
-      if (max_color_idx > 255)
+      bool two_byte = 255 < max_color_idx;
+      if (two_byte)
 	expected_size *= 2;
       if (type == PBM_COLOR)
 	expected_size *= 3;
@@ -5522,24 +5542,14 @@ pbm_load (struct frame *f, struct image *img)
 	    int r, g, b;
 
 	    if (type == PBM_GRAY && raw_p)
-	      {
-		r = g = b = *p++;
-		if (max_color_idx > 255)
-		  r = g = b = r * 256 + *p++;
-	      }
+	      r = g = b = pbm_scan_index (&p, two_byte);
 	    else if (type == PBM_GRAY)
 	      r = g = b = pbm_scan_number (&p, end);
 	    else if (raw_p)
 	      {
-		r = *p++;
-		if (max_color_idx > 255)
-		  r = r * 256 + *p++;
-		g = *p++;
-		if (max_color_idx > 255)
-		  g = g * 256 + *p++;
-		b = *p++;
-		if (max_color_idx > 255)
-		  b = b * 256 + *p++;
+		r = pbm_scan_index (&p, two_byte);
+		g = pbm_scan_index (&p, two_byte);
+		b = pbm_scan_index (&p, two_byte);
 	      }
 	    else
 	      {
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Don-t-reject-PBM-header-whitespace-unnecessarily.patch --]
[-- Type: text/x-patch; name="0002-Don-t-reject-PBM-header-whitespace-unnecessarily.patch", Size: 1010 bytes --]

From 000abe3c89b1934c23377a792a838f289d4bbf8a Mon Sep 17 00:00:00 2001
From: Andy Moreton <andrewjmoreton@gmail.com>
Date: Mon, 16 Oct 2017 01:23:32 -0700
Subject: [PATCH 2/2] Don't reject PBM header whitespace unnecessarily

* lisp/image.el (image-type-header-regexps):
Allow two or more CRs or LFs in initial whitespace sequences.  See:
http://netpbm.sourceforge.net/doc/pbm.html
---
 lisp/image.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/image.el b/lisp/image.el
index 1d07761..32df508 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -34,8 +34,8 @@ 'image-refresh
 (defconst image-type-header-regexps
   `(("\\`/[\t\n\r ]*\\*.*XPM.\\*/" . xpm)
     ("\\`P[1-6]\\(?:\
-\\(?:\\(?:#[^\r\n]*[\r\n]\\)?[[:space:]]\\)+\
-\\(?:\\(?:#[^\r\n]*[\r\n]\\)?[0-9]\\)+\
+\\(?:\\(?:#[^\r\n]*[\r\n]\\)*[[:space:]]\\)+\
+\\(?:\\(?:#[^\r\n]*[\r\n]\\)*[0-9]\\)+\
 \\)\\{2\\}" . pbm)
     ("\\`GIF8[79]a" . gif)
     ("\\`\x89PNG\r\n\x1a\n" . png)
-- 
2.7.4