Subject: 27.0.50; Trivial image.c bugs

From: Pip Cet <[email-masked]>
Date: Thu, 27 Jun 2019 16:28:05 +0000

These are all in the category "Lisp code does something silly, the
image code breaks".

(let ((l `(image :type xbm :type xbm :height 1 :width 1 :data
,(bool-vector t))))
  (insert-image l))

inserts an image. It should consider the spec erroneous.
--
(let ((tail (cons :invalid nil)))
   (setcdr tail tail)
   (insert-image `(image :type xbm . ,tail)))

causes an infinite loop. It should be considered invalid.
--
(insert-image `(image :dummy :type :type xbm :height 1 :width 1 :data
,(bool-vector t)))

produces an error. It should arguably behave the same as

(insert-image `(image :dummy :dummy :type xbm :height 1 :width 1 :data
,(bool-vector t)))
--
(let* ((circ1 (cons :dummy nil))
       (circ2 (cons :dummy nil))
       (spec1 `(image :type xbm :width 1 :height 1 :data ,(bool-vector
1) :ignored ,circ1))
       (spec2 `(image :type xbm :width 1 :height 1 :data ,(bool-vector
1) :ignored ,circ2)))
  (setcdr circ1 circ1)
  (setcdr circ2 circ2)
  (insert-image spec1)
  (insert-image spec2))

livelocks emacs somehow. It should...I don't know. Abort because the
spec is circular? Not compare specs using Fequal?
--
(insert-image `(image :type postscript :pt-width 100 :pt-height 100
              :ascent 0
              :bounding-box (0 0 100 100) :file "dummy.ps"
              :loader ,(lambda (frame spec width height id colors)
                 (setf (plist-get spec :ascent)
                       -1))))

livelocks Emacs in the display code. It should automatically switch to
the buffer called "image.c" and rewrite the code there not to call
Lisp.
--
These probably aren't worth fixing in their own right, but someone
might think image.c is a good place to take plist handling code
from...

I think with the exception of the contrived last example, these are
all easy to fix, but a bit harder to fix well. I've tried to do the
former, for now, but I'd welcome any help for me to do the latter.
From eef20e02d25a16f71ad0904644d68450a6181fe2 Mon Sep 17 00:00:00 2001
From: Pip Cet <[email-masked]>
Date: Thu, 27 Jun 2019 16:19:25 +0000
Subject: [PATCH] Fix minor image bugs.

    * src/image.c (valid_image_p): Don't check value elements of the plist
for key equality.
(parse_image_spec): Use FOR_EACH_TAIL_SAFE to abort for circular
structures. Fix off-by-one error.
(image_spec_value): Handle circular structures.
(equal_lists): Introduce; compares two lists' elements with `eq'
(search_image_cache): Don't call Fequal from redisplay code.
---
 src/image.c | 73 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 25 deletions(-)

diff --git a/src/image.c b/src/image.c
index 7b648c46ae..89ba2b9d29 100644
--- a/src/image.c
+++ b/src/image.c
@@ -800,17 +800,22 @@ valid_image_p (Lisp_Object object)
     {
       Lisp_Object tail = XCDR (object);
       FOR_EACH_TAIL_SAFE (tail)
-	if (EQ (XCAR (tail), QCtype))
-	  {
-	    tail = XCDR (tail);
-	    if (CONSP (tail))
-	      {
-		struct image_type const *type = lookup_image_type (XCAR (tail));
-		if (type)
-		  return type->valid_p (object);
-	      }
-	    break;
-	  }
+	{
+	  if (EQ (XCAR (tail), QCtype))
+	    {
+	      tail = XCDR (tail);
+	      if (CONSP (tail))
+		{
+		  struct image_type const *type = lookup_image_type (XCAR (tail));
+		  if (type)
+		    return type->valid_p (object);
+		}
+	      break;
+	    }
+	  tail = XCDR (tail);
+	  if (! CONSP (tail))
+	    return false;
+	}
     }
 
   return false;
@@ -897,7 +902,7 @@ parse_image_spec (Lisp_Object spec, struct image_keyword *keywords,
     return 0;
 
   plist = XCDR (spec);
-  while (CONSP (plist))
+  FOR_EACH_TAIL_SAFE (plist)
     {
       Lisp_Object key, value;
 
@@ -911,7 +916,6 @@ parse_image_spec (Lisp_Object spec, struct image_keyword *keywords,
       if (!CONSP (plist))
 	return 0;
       value = XCAR (plist);
-      plist = XCDR (plist);
 
       /* Find key in KEYWORDS.  Error if not found.  */
       for (i = 0; i < nkeywords; ++i)
@@ -921,10 +925,10 @@ parse_image_spec (Lisp_Object spec, struct image_keyword *keywords,
       if (i == nkeywords)
 	continue;
 
-      /* Record that we recognized the keyword.  If a keywords
+      /* Record that we recognized the keyword.  If a keyword
 	 was found more than once, it's an error.  */
       keywords[i].value = value;
-      if (keywords[i].count > 1)
+      if (keywords[i].count > 0)
 	return 0;
       ++keywords[i].count;
 
@@ -1006,14 +1010,22 @@ parse_image_spec (Lisp_Object spec, struct image_keyword *keywords,
 
       if (EQ (key, QCtype) && !EQ (type, value))
 	return 0;
-    }
 
-  /* Check that all mandatory fields are present.  */
-  for (i = 0; i < nkeywords; ++i)
-    if (keywords[i].mandatory_p && keywords[i].count == 0)
-      return 0;
+      if (EQ (XCDR (plist), Qnil))
+	{
+	  /* Check that all mandatory fields are present.  */
+	  for (i = 0; i < nkeywords; ++i)
+	    if (keywords[i].mandatory_p && keywords[i].count == 0)
+	      return 0;
+
+	  return 1;
+	}
+
+      if (! CONSP (plist))
+	return 0;
+    }
 
-  return NILP (plist);
+  return 0;
 }

@@ -1028,9 +1040,8 @@ image_spec_value (Lisp_Object spec, Lisp_Object key, bool *found)
 
   eassert (valid_image_p (spec));
 
-  for (tail = XCDR (spec);
-       CONSP (tail) && CONSP (XCDR (tail));
-       tail = XCDR (XCDR (tail)))
+  tail = XCDR (spec);
+  FOR_EACH_TAIL_SAFE (tail)
     {
       if (EQ (XCAR (tail), key))
 	{
@@ -1038,6 +1049,9 @@ image_spec_value (Lisp_Object spec, Lisp_Object key, bool *found)
 	    *found = 1;
 	  return XCAR (XCDR (tail));
 	}
+      tail = XCDR (tail);
+      if (! CONSP (tail))
+	break;
     }
 
   if (found)
@@ -1572,6 +1586,15 @@ make_image_cache (void)
   return c;
 }
 
+/* Compare two (non-circular) lists, comparing each element with `eq'. */
+static bool
+equal_lists (Lisp_Object a, Lisp_Object b)
+{
+  while (CONSP (a) && CONSP (b) && Feq (XCAR (a), XCAR (b)))
+    a = XCDR (a), b = XCDR (b);
+
+  return EQ (a, b);
+}
 
 /* Find an image matching SPEC in the cache, and return it.  If no
    image is found, return NULL.  */
@@ -1598,7 +1621,7 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash)
 
   for (img = c->buckets[i]; img; img = img->next)
     if (img->hash == hash
-	&& !NILP (Fequal (img->spec, spec))
+	&& !equal_lists (img->spec, spec)
 	&& img->frame_foreground == FRAME_FOREGROUND_PIXEL (f)
 	&& img->frame_background == FRAME_BACKGROUND_PIXEL (f))
       break;
-- 
2.20.1