Subject: 27.0.50; XFIXNAT called on negative numbers

From: Pip Cet <[email-masked]>
Date: Tue, 25 Jun 2019 05:36:15 +0000

I just spent some time reviewing the use of XFIXNAT, after being
bitten by this code in fileio.c:

      if (FIXNUMP (tem))
    nextpos = XFIXNAT (tem);

In the following cases, negative numbers passed to APIs which aren't
strictly internal or low-level cause us to call XFIXNAT on a negative
number:

(indent-to -100 -100)
(set-text-properties -1 -1 nil "")
(make-network-process :name "name" :family 'ipv6 :remote [-1 0 0 0 0 0 0 0 0])
(end-kbd-macro -1 nil)
(previous-single-property-change (point) 'dummy nil -1)

I think those six cases are worth fixing, but more generally, I think
it would help avoid such errors if XFIXNAT were "always" paired with
FIXNATP, XFIXNUM with FIXNUMP, and XCHARACTER (which would be new)
with CHARACTERP.

So I'm going to be brave here and admit that in many cases, while
XFIXNUM was correct, it took me a long time to see that because the
check was performed somewhere else.

I understand there is a very slight performance hit on some platforms,
but is that really still an issue? Also, consistent pairing would
catch a few cases in which we call XFIXNUM but could get away with an
XFIXNAT, such as this code in self-insert-command:

  if (!CHARACTERP (c))
    bitch_at_user ();
  else {
    int character = translate_char (Vtranslation_table_for_input,
                    XFIXNUM (c));

I also think it would make sense to eassume (FIXNUMP (a)) in both
XFIXNAT and XFIXNUM (but not XHASH, or XUFIXNUM if that's what XHASH
uses). It might catch bugs like

    (set-text-properties nil nil nil "")

currently not throwing an error.

The patch I'm attaching fixes only things I'm reasonably sure are bugs.
diff --git a/src/ccl.c b/src/ccl.c
index ec108e30d8..c853e61982 100644
--- a/src/ccl.c
+++ b/src/ccl.c
@@ -2062,7 +2062,7 @@ #define CCL_EXECUTE_BUF_SIZE 1024
       if (TYPE_RANGED_FIXNUMP (int, AREF (status, i)))
 	ccl.reg[i] = XFIXNUM (AREF (status, i));
     }
-  if (FIXNUMP (AREF (status, i)))
+  if (FIXNATP (AREF (status, i)))
     {
       i = XFIXNAT (AREF (status, 8));
       if (ccl.ic < i && i < ccl.size)
diff --git a/src/dosfns.c b/src/dosfns.c
index 47c545007a..fb5bcc9ad3 100644
--- a/src/dosfns.c
+++ b/src/dosfns.c
@@ -72,7 +72,7 @@ DEFUN ("int86", Fint86, Sint86, 2, 2, 0,
   if (no < 0 || no > 0xff || ASIZE (registers) != 8)
     return Qnil;
   for (i = 0; i < 8; i++)
-    CHECK_FIXNUM (AREF (registers, i));
+    CHECK_FIXNAT (AREF (registers, i));
 
   inregs.x.ax    = (unsigned long) XFIXNAT (AREF (registers, 0));
   inregs.x.bx    = (unsigned long) XFIXNAT (AREF (registers, 1));
@@ -139,7 +139,7 @@ DEFUN ("msdos-memput", Fdos_memput, Sdos_memput, 2, 2, 0,
 
   for (i = 0; i < len; i++)
     {
-      CHECK_FIXNUM (AREF (vector, i));
+      CHECK_FIXNAT (AREF (vector, i));
       buf[i] = (unsigned char) XFIXNAT (AREF (vector, i)) & 0xFF;
     }
 
diff --git a/src/fileio.c b/src/fileio.c
index 5dd14daacb..b2db6efd54 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4719,7 +4719,7 @@ because (1) it preserves some marker positions and (2) it puts less data
 		 the format conversion.  */
 	      Lisp_Object tem = XCAR (old_undo);
 	      if (CONSP (tem) && FIXNUMP (XCAR (tem))
-		  && FIXNUMP (XCDR (tem))
+		  && FIXNATP (XCDR (tem))
 		  && XFIXNAT (XCDR (tem)) == PT + old_inserted)
 		XSETCDR (tem, make_fixnum (PT + inserted));
 	    }
@@ -5391,7 +5391,7 @@ a_write (int desc, Lisp_Object string, ptrdiff_t pos,
     {
       tem = Fcar_safe (Fcar (*annot));
       nextpos = pos - 1;
-      if (FIXNUMP (tem))
+      if (FIXNATP (tem))
 	nextpos = XFIXNAT (tem);
 
       /* If there are no more annotations in this range,
@@ -5847,7 +5847,7 @@ DEFUN ("do-auto-save", Fdo_auto_save, Sdo_auto_save, 0, 2, "",
 
 	    set_buffer_internal (b);
 	    if (NILP (Vauto_save_include_big_deletions)
-		&& FIXNUMP (BVAR (b, save_length))
+		&& FIXNATP (BVAR (b, save_length))
 		/* A short file is likely to change a large fraction;
 		   spare the user annoying messages.  */
 		&& XFIXNAT (BVAR (b, save_length)) > 5000
diff --git a/src/image.c b/src/image.c
index 7b648c46ae..2ecea0893f 100644
--- a/src/image.c
+++ b/src/image.c
@@ -2385,7 +2385,7 @@ lookup_image (struct frame *f, Lisp_Object spec)
 #endif
 
 	  ascent = image_spec_value (spec, QCascent, NULL);
-	  if (FIXNUMP (ascent))
+	  if (FIXNATP (ascent))
 	    img->ascent = XFIXNAT (ascent);
 	  else if (EQ (ascent, Qcenter))
 	    img->ascent = CENTERED_IMAGE_ASCENT;
diff --git a/src/lisp.h b/src/lisp.h
index 77fc22d118..a1bd6d8f6b 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2968,6 +2968,14 @@ #define CHECK_FIXNUM_COERCE_MARKER(x)					\
       CHECK_TYPE (FIXNUMP (x), Qinteger_or_marker_p, x);		\
   } while (false)
 
+#define CHECK_FIXNAT_COERCE_MARKER(x)					\
+  do {									\
+    if (MARKERP ((x)))							\
+      XSETFASTINT (x, marker_position (x));				\
+    else								\
+      CHECK_TYPE (FIXNATP (x), Qinteger_or_marker_p, x);		\
+  } while (false)
+
 INLINE double
 XFLOATINT (Lisp_Object n)
 {
diff --git a/src/process.c b/src/process.c
index 6717ccb418..6bc3c75190 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2673,7 +2673,7 @@ conv_lisp_to_sockaddr (int family, Lisp_Object address, struct sockaddr *sa, int
 	  hostport = XFIXNUM (p->contents[--len]);
 	  sin6->sin6_port = htons (hostport);
 	  for (i = 0; i < len; i++)
-	    if (FIXNUMP (p->contents[i]))
+	    if (FIXNATP (p->contents[i]))
 	      {
 		int j = XFIXNAT (p->contents[i]) & 0xffff;
 		ip6[i] = ntohs (j);
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..6b20a34820 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -134,8 +134,8 @@ validate_interval_range (Lisp_Object object, Lisp_Object *begin,
   ptrdiff_t searchpos;
 
   CHECK_STRING_OR_BUFFER (object);
-  CHECK_FIXNUM_COERCE_MARKER (*begin);
-  CHECK_FIXNUM_COERCE_MARKER (*end);
+  CHECK_FIXNAT_COERCE_MARKER (*begin);
+  CHECK_FIXNAT_COERCE_MARKER (*end);
 
   /* If we are asked for a point, but from a subr which operates
      on a range, then return nothing.  */
@@ -790,14 +790,14 @@ DEFUN ("next-single-char-property-change", Fnext_single_char_property_change,
 	  Fset_buffer (object);
 	}
 
-      CHECK_FIXNUM_COERCE_MARKER (position);
+      CHECK_FIXNAT_COERCE_MARKER (position);
 
       initial_value = Fget_char_property (position, prop, object);
 
       if (NILP (limit))
 	XSETFASTINT (limit, ZV);
       else
-	CHECK_FIXNUM_COERCE_MARKER (limit);
+	CHECK_FIXNAT_COERCE_MARKER (limit);
 
       if (XFIXNAT (position) >= XFIXNAT (limit))
 	{
@@ -874,12 +874,12 @@ DEFUN ("previous-single-char-property-change",
 	  Fset_buffer (object);
 	}
 
-      CHECK_FIXNUM_COERCE_MARKER (position);
+      CHECK_FIXNAT_COERCE_MARKER (position);
 
       if (NILP (limit))
 	XSETFASTINT (limit, BEGV);
       else
-	CHECK_FIXNUM_COERCE_MARKER (limit);
+	CHECK_FIXNAT_COERCE_MARKER (limit);
 
       if (XFIXNAT (position) <= XFIXNAT (limit))
 	{
@@ -942,7 +942,7 @@ DEFUN ("next-property-change", Fnext_property_change,
     XSETBUFFER (object, current_buffer);
 
   if (!NILP (limit) && !EQ (limit, Qt))
-    CHECK_FIXNUM_COERCE_MARKER (limit);
+    CHECK_FIXNAT_COERCE_MARKER (limit);
 
   i = validate_interval_range (object, &position, &position, soft);
 
@@ -975,7 +975,7 @@ DEFUN ("next-property-change", Fnext_property_change,
 
   if (!next
       || (next->position
-	  >= (FIXNUMP (limit)
+	  >= (FIXNATP (limit)
 	      ? XFIXNAT (limit)
 	      : (STRINGP (object)
 		 ? SCHARS (object)
@@ -1009,7 +1009,7 @@ DEFUN ("next-single-property-change", Fnext_single_property_change,
     XSETBUFFER (object, current_buffer);
 
   if (!NILP (limit))
-    CHECK_FIXNUM_COERCE_MARKER (limit);
+    CHECK_FIXNAT_COERCE_MARKER (limit);
 
   i = validate_interval_range (object, &position, &position, soft);
   if (!i)
@@ -1024,7 +1024,7 @@ DEFUN ("next-single-property-change", Fnext_single_property_change,
 
   if (!next
       || (next->position
-	  >= (FIXNUMP (limit)
+	  >= (FIXNATP (limit)
 	      ? XFIXNAT (limit)
 	      : (STRINGP (object)
 		 ? SCHARS (object)
@@ -1056,7 +1056,7 @@ DEFUN ("previous-property-change", Fprevious_property_change,
     XSETBUFFER (object, current_buffer);
 
   if (!NILP (limit))
-    CHECK_FIXNUM_COERCE_MARKER (limit);
+    CHECK_FIXNAT_COERCE_MARKER (limit);
 
   i = validate_interval_range (object, &position, &position, soft);
   if (!i)
@@ -1074,7 +1074,7 @@ DEFUN ("previous-property-change", Fprevious_property_change,
 
   if (!previous
       || (previous->position + LENGTH (previous)
-	  <= (FIXNUMP (limit)
+	  <= (FIXNATP (limit)
 	      ? XFIXNAT (limit)
 	      : (STRINGP (object) ? 0 : BUF_BEGV (XBUFFER (object))))))
     return limit;
@@ -1106,7 +1106,7 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change,
     XSETBUFFER (object, current_buffer);
 
   if (!NILP (limit))
-    CHECK_FIXNUM_COERCE_MARKER (limit);
+    CHECK_FIXNAT_COERCE_MARKER (limit);
 
   i = validate_interval_range (object, &position, &position, soft);
 
@@ -1127,7 +1127,7 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change,
 
   if (!previous
       || (previous->position + LENGTH (previous)
-	  <= (FIXNUMP (limit)
+	  <= (FIXNATP (limit)
 	      ? XFIXNAT (limit)
 	      : (STRINGP (object) ? 0 : BUF_BEGV (XBUFFER (object))))))
     return limit;
@@ -1353,8 +1353,8 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
   /* If we want no properties for a whole string,
      get rid of its intervals.  */
   if (NILP (properties) && STRINGP (object)
-      && XFIXNAT (start) == 0
-      && XFIXNAT (end) == SCHARS (object))
+      && FIXNATP (start) && XFIXNAT (start) == 0
+      && FIXNATP (end) && XFIXNAT (end) == SCHARS (object))
     {
       if (!string_intervals (object))
 	return Qnil;
diff --git a/src/w32term.c b/src/w32term.c
index 5726124b0e..886fc6c751 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -2463,7 +2463,7 @@ w32_draw_glyph_string (struct glyph_string *s)
 		  Lisp_Object val
 		    = buffer_local_value (Qunderline_minimum_offset,
 					s->w->contents);
-		  if (FIXNUMP (val))
+		  if (FIXNATP (val))
 		    minimum_offset = XFIXNAT (val);
 		  else
 		    minimum_offset = 1;
diff --git a/src/xterm.c b/src/xterm.c
index 1acff2af0d..64467e0d92 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -3806,7 +3806,7 @@ x_draw_glyph_string (struct glyph_string *s)
 		  Lisp_Object val
 		    = buffer_local_value (Qunderline_minimum_offset,
 					  s->w->contents);
-		  if (FIXNUMP (val))
+		  if (FIXNATP (val))
 		    minimum_offset = XFIXNAT (val);
 		  else
 		    minimum_offset = 1;