all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#36370: 27.0.50; XFIXNAT called on negative numbers
@ 2019-06-25  5:36 Pip Cet
  2019-06-27  1:10 ` Paul Eggert
  0 siblings, 1 reply; 36+ messages in thread
From: Pip Cet @ 2019-06-25  5:36 UTC (permalink / raw)
  To: 36370

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

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.

[-- Attachment #2: xfixnat.diff --]
[-- Type: text/x-patch, Size: 8925 bytes --]

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;

^ permalink raw reply related	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2019-07-02 23:39 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-25  5:36 bug#36370: 27.0.50; XFIXNAT called on negative numbers Pip Cet
2019-06-27  1:10 ` Paul Eggert
2019-06-27  6:16   ` Pip Cet
2019-06-27  8:28     ` Paul Eggert
2019-06-27 13:17       ` Pip Cet
2019-06-27 13:37         ` Eli Zaretskii
2019-06-27 19:38         ` Paul Eggert
2019-06-27 19:56           ` Pip Cet
2019-06-27 21:13             ` Paul Eggert
     [not found]             ` <5284eb58-3560-da42-d1d1-3bdb930eae49@cs.ucla.edu>
2019-06-27 21:37               ` Pip Cet
2019-06-27 23:45               ` Bruno Haible
     [not found]               ` <2715311.ceefYqj39C@omega>
2019-06-28  0:04                 ` Paul Eggert
2019-06-28 11:06                 ` Pip Cet
2019-06-28 12:14                   ` Bruno Haible
     [not found]                   ` <8979488.cRkkfcT1mV@omega>
2019-06-28 12:29                     ` Bruno Haible
2019-06-28 13:51                     ` Pip Cet
     [not found]                     ` <CAOqdjBfS99UpLZ-qLe4=FMXMsr+T3LUvJEsf_gfmF6wwLbqgOw@mail.gmail.com>
2019-06-28 17:46                       ` Paul Eggert
2019-06-28 19:11                       ` Bruno Haible
     [not found]                       ` <a293f2fe-99b3-3776-f27b-35e3a93d1d34@cs.ucla.edu>
2019-06-28 19:15                         ` Pip Cet
2019-06-28 19:56                           ` Bruno Haible
2019-06-28 21:08                             ` Pip Cet
2019-06-29  5:41                           ` Paul Eggert
     [not found]                           ` <87168b28-192b-6666-e9b6-9cdc2ed3917a@cs.ucla.edu>
2019-06-29  6:48                             ` Pip Cet
     [not found]                             ` <CAOqdjBfcNbXFw3Fb0wgRR10PNbkJQ+88ObE9KEghLSb-ptdrbA@mail.gmail.com>
2019-06-29 17:31                               ` Paul Eggert
     [not found]                               ` <791ae316-3a6f-605a-0da5-874fe3d224c5@cs.ucla.edu>
2019-06-30  9:21                                 ` Pip Cet
     [not found]                       ` <11002295.LrvMqknVDZ@omega>
2019-06-28 21:07                         ` Pip Cet
2019-06-28 23:30                           ` Bruno Haible
     [not found]                           ` <2067160.1HRgjLhtDS@omega>
2019-06-29  5:40                             ` Paul Eggert
2019-06-29  5:44                             ` Pip Cet
     [not found]                             ` <CAOqdjBcNA4mDiwsd_jbeePGMdUwPvkFCNdgtZvmiQnYmJNR3pA@mail.gmail.com>
2019-06-29 10:31                               ` Bruno Haible
     [not found]                               ` <2515002.Q0mBYvUW8C@omega>
2019-06-29 17:11                                 ` Paul Eggert
     [not found]                                 ` <99bacb9f-1192-1315-85d7-5ab4924dfef8@cs.ucla.edu>
2019-06-29 17:48                                   ` Bruno Haible
2019-06-30 15:30                                 ` Pip Cet
     [not found]                                 ` <CAOqdjBeiMno7nGKwk7SSZQob+CTyG39KRTM9EEebq7NQavLR-Q@mail.gmail.com>
2019-06-30 15:45                                   ` Bruno Haible
2019-07-02 23:39                                     ` Paul Eggert
2019-07-01  1:46                                   ` Richard Stallman

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.