Subject: Re: bug#36370: 27.0.50; XFIXNAT called on negative numbers

From: Paul Eggert <[email-masked]>
Date: Thu, 27 Jun 2019 01:28:30 -0700

Pip Cet wrote:

> I'd prefer if the code were written as though FIXNATs weren't FIXNUMs at all.

I'm more of the opposite opinion. The original reason for XFIXNAT (under its old 
name) was performance, because long ago Emacs put the type tag in the most 
significant bits, and it was faster to mask it out than to smear the sign bit 
when you knew the fixnum was nonnegative. Nowadays that original reason is 
obsolete because the tags are in the least significant bits and XFIXNAT is just 
an alias for XFIXNUM, and if it were up to me we'd get rid of XFIXNAT entirely, 
and just use XFIXNUM. But old habits die hard....

>        ascent = image_spec_value (spec, QCascent, NULL);
>        if (FIXNUMP (ascent))
>          img->ascent = XFIXNAT (ascent);
> 
> in image.c is safe, because lookup_image is called only after valid_image_p.

Sorry, you've lost me. How does valid_image_p guarantee that 'ascent' (if a 
fixnum) is in the range 0..INT_MAX? Because if it's not in that range, the above 
code could trap.

> I'd prefer AREF (status, 8) here, for what readability the code gives us.

Sure, sounds good. See attached further patch.

> I'd prefer leaving the macros for now

I actually did it that way at first, but that failed miserably as the macros 
evaluated their arguments more than once when debugging , and lots of code 
expects them to behave like functions and evaluate their arguments exactly once. 
So functions it is.

> eassume (inline_function (x))
> doesn't work very well on current GCC, while eassume (MACRO (x)) is
> fine, so I'm sometimes building with DEFINE_KEY_OPS_AS_MACROS.

Hmm, I see I should have helped GCC more there. First, the two new eassumes 
should just be easserts as the assumptions are not helpful to the compiler. 
Second, I should restore the eassume that tells GCC that XFIXNAT returns a 
nonnegative integer.

> (The
> eassume/eassert situation is something else I've been planning to look
> at in more detail, though debugging it will be easier when
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91005 is fixed).

Sorry, I don't follow. That bug report seems to be about nested C functions; why 
is it relevant to Emacs, which doesn't use nested C functions?

>> * The ccl.c, fileio.c, image.c, and process.c fixes should use XFIXNUM,
>> not XFIXNAT, since the code is range-checking after extraction anyway
>> and the latter range-checking should do the right thing. Come to think
> 
> That's an argument that either XFIXNUM or XFIXNAT are fine, and in
> this case XFIXNAT seems, to me, much more readable.

No, as the range-checking is done after the XFIXNAT, which means the XFIXNAT 
could have an assertion failure.

>> of it, the ccl.c and image.c code is buggy regardless of the
>> XFIXNUM/XFIXNAT business since the Emacs integer might not fit into int
>> range, and that should be fixed too.
> 
> I don't see how either ccl.c or image.c are buggy for out-of-range
> integer arguments.

For example, ccl.c in master has this:

   if (FIXNUMP (AREF (status, i)))
     {
       i = XFIXNAT (AREF (status, 8));
       if (ccl.ic < i && i < ccl.size)
	ccl.ic = i;
     }

where i is a 32-bit int. If AREF (status, 8) is the 62-bit Lisp fixnum with 
value '2 * (EMACS_INT) INT_MAX + ccl.ic + 3', the assignment to i overflows, 
which can trap. Even if the assignment doesn't trap but merely wraps around, the 
next line won't do range checking correctly.

> When you want a position, you want a FIXNAT,
> not a negative number, so CHECK_FIXNAT_COERCE_MARKER everywhere would
> catch those cases where someone passes an obviously invalid argument.

But why stop there? 0 is an obviously invalid argument for buffers. :-) I think 
this is just an area where we disagree - I want a range check where you want a 
type check. It's safe either way and the range check is a tad faster (as the 
range check needs to be done anyway) and results in a crisper diagnostic when it 
fails.
> I'd prefer not touching the dosfns.c code, for the simple reason that
> if anyone still uses it, they might rely on the broken behavior.

We can't leave it alone, as XFIXNAT is now checking that its result is 
nonnegative when debugging is enabled. However, we could instead change the 
XFIXNATs to XFIXNUMs: this will generate exactly the same machine code on that 
platform when debugging is disabled, and will not trap when debugging is 
enabled. See attached patch.
From 3f8090e91125de98798d4bfb5f91934bcac9626b Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Thu, 27 Jun 2019 01:14:53 -0700
Subject: [PATCH] Improve XFIXNUM cleanup a bit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Based on Pip Cet’s review (Bug#36370#13).
* src/ccl.c (Fccl_execute_on_string): Use clearer indexing.
* src/dosfns.c (Fint86, Fdos_memput):
Avoid runtime checks for negative fixnums when debugging.
This restores the earlier machine code.
* src/lisp.h (XFIXNUM, XUFIXNUM): Use eassert, not eassume.
(XFIXNAT): At the start, merely eassert FIXNUMP rather
than eassuming FIXNATP.  At the end, eassume that the
result is nonnegative.  This restores help to the compiler
that the previous patch mistakenly removed.
---
 src/ccl.c    |  4 ++--
 src/dosfns.c | 22 +++++++++++-----------
 src/lisp.h   | 10 ++++++----
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/src/ccl.c b/src/ccl.c
index f1d4c28df1..ff42c6f25f 100644
--- a/src/ccl.c
+++ b/src/ccl.c
@@ -2062,9 +2062,9 @@ #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 (FIXNUMP (AREF (status, 8)))
     {
-      EMACS_INT ic = XFIXNUM (AREF (status, i));
+      EMACS_INT ic = XFIXNUM (AREF (status, 8));
       if (ccl.ic < ic && ic < ccl.size)
 	ccl.ic = ic;
     }
diff --git a/src/dosfns.c b/src/dosfns.c
index fb5bcc9ad3..635f29bd65 100644
--- a/src/dosfns.c
+++ b/src/dosfns.c
@@ -72,16 +72,16 @@ DEFUN ("int86", Fint86, Sint86, 2, 2, 0,
   if (no < 0 || no > 0xff || ASIZE (registers) != 8)
     return Qnil;
   for (i = 0; i < 8; i++)
-    CHECK_FIXNAT (AREF (registers, i));
+    CHECK_FIXNUM (AREF (registers, i));
 
-  inregs.x.ax    = (unsigned long) XFIXNAT (AREF (registers, 0));
-  inregs.x.bx    = (unsigned long) XFIXNAT (AREF (registers, 1));
-  inregs.x.cx    = (unsigned long) XFIXNAT (AREF (registers, 2));
-  inregs.x.dx    = (unsigned long) XFIXNAT (AREF (registers, 3));
-  inregs.x.si    = (unsigned long) XFIXNAT (AREF (registers, 4));
-  inregs.x.di    = (unsigned long) XFIXNAT (AREF (registers, 5));
-  inregs.x.cflag = (unsigned long) XFIXNAT (AREF (registers, 6));
-  inregs.x.flags = (unsigned long) XFIXNAT (AREF (registers, 7));
+  inregs.x.ax    = (unsigned long) XFIXNUM (AREF (registers, 0));
+  inregs.x.bx    = (unsigned long) XFIXNUM (AREF (registers, 1));
+  inregs.x.cx    = (unsigned long) XFIXNUM (AREF (registers, 2));
+  inregs.x.dx    = (unsigned long) XFIXNUM (AREF (registers, 3));
+  inregs.x.si    = (unsigned long) XFIXNUM (AREF (registers, 4));
+  inregs.x.di    = (unsigned long) XFIXNUM (AREF (registers, 5));
+  inregs.x.cflag = (unsigned long) XFIXNUM (AREF (registers, 6));
+  inregs.x.flags = (unsigned long) XFIXNUM (AREF (registers, 7));
 
   int86 (no, &inregs, &outregs);
 
@@ -139,8 +139,8 @@ DEFUN ("msdos-memput", Fdos_memput, Sdos_memput, 2, 2, 0,
 
   for (i = 0; i < len; i++)
     {
-      CHECK_FIXNAT (AREF (vector, i));
-      buf[i] = (unsigned char) XFIXNAT (AREF (vector, i)) & 0xFF;
+      CHECK_FIXNUM (AREF (vector, i));
+      buf[i] = (unsigned char) XFIXNUM (AREF (vector, i)) & 0xFF;
     }
 
   dosmemput (buf, len, offs);
diff --git a/src/lisp.h b/src/lisp.h
index 077d236065..a0619e64f2 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1195,7 +1195,7 @@ XFIXNUM_RAW (Lisp_Object a)
 INLINE EMACS_INT
 XFIXNUM (Lisp_Object a)
 {
-  eassume (FIXNUMP (a));
+  eassert (FIXNUMP (a));
   return XFIXNUM_RAW (a);
 }
 
@@ -1209,7 +1209,7 @@ XUFIXNUM_RAW (Lisp_Object a)
 INLINE EMACS_UINT
 XUFIXNUM (Lisp_Object a)
 {
-  eassume (FIXNUMP (a));
+  eassert (FIXNUMP (a));
   return XUFIXNUM_RAW (a);
 }
 
@@ -2828,9 +2828,11 @@ FIXNATP (Lisp_Object x)
 INLINE EMACS_INT
 XFIXNAT (Lisp_Object a)
 {
-  eassume (FIXNATP (a));
+  eassert (FIXNUMP (a));
   EMACS_INT int0 = Lisp_Int0;
-  return USE_LSB_TAG ? XFIXNUM (a) : XLI (a) - (int0 << VALBITS);
+  EMACS_INT result = USE_LSB_TAG ? XFIXNUM (a) : XLI (a) - (int0 << VALBITS);
+  eassume (0 <= result);
+  return result;
 }
 
 INLINE bool
-- 
2.21.0