bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Thu, Jun 27, 2019 at 1:11 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
> Thanks for looking into this. Cleaning this up has been on my to-do list
> for a while, and something along these lines has been discussed on the
> mailing list. Some comments on your fixes:

I really think I should reiterate that readability is much much more
important than whatever performance tweaks XFIXNAT might allow. I'd
prefer if the code were written as though FIXNATs weren't FIXNUMs at
all: pair FIXNATP with XFIXNAT, and provide FIXNAT versions of
CHECK_RANGED_FIXNUM and CHECK_FIXNUM_COERCE_MARKER.

Hmm. I'm not sure it's a problem for you, but I'm finding it rather
hard to check in all cases whether XFIXNAT is safe. For example,

      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.

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

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

 # define XHASH(a) lisp_h_XHASH (a)
 # if USE_LSB_TAG
 #  define make_fixnum(n) lisp_h_make_fixnum (n)
-#  define XFIXNAT(a) lisp_h_XFIXNAT (a)
-#  define XFIXNUM(a) lisp_h_XFIXNUM (a)
+#  define XFIXNUM_RAW(a) lisp_h_XFIXNUM_RAW (a)
 #  define XTYPE(a) lisp_h_XTYPE (a)
 # endif
 #endif

I'd prefer leaving the macros for now; 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. (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).

> * 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.

> 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.

> * Instead of adding a new macro CHECK_FIXNAT_COERCE_MARKER, the code
> should stick with CHECK_FIXNUM_COERCE_MARKER and do the usual
> range-checking on the results. This should yield more-precise
> diagnostics. Most of the range-checking is there already.

I disagree, obviously. 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.

> * The set_text_properties fix can be done more efficiently via EQ.

I suspect your change results in a few more insns being emitted, but
I'll have to check.

> * I'm mildly inclined to think that w32term.c and xterm.c should treate
> negative minimum values as 0 when the actual value must be nonnegative.
> That is, a negative minimum should act like a minimum of 0, not as a
> minimum of 1 as in your proposed patch.

Doesn't matter, so I decided not to fiddle with this code.

> To help prevent similar problems in the future, XFIXNUM, XFIXNAT etc.
> should check that their arguments are of the proper type when
> ENABLE_CHECKING is nonzero.
>
> A proposed patch is attached; it should do all the above.

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.

> PS. At some point we should also check that make_fixnum is invoked only
> on values in fixnum range, but that's a matter for a later patch.

Definitely.