bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#36370: 27.0.50; XFIXNAT called on negative numbers


From: Paul Eggert
Subject: bug#36370: 27.0.50; XFIXNAT called on negative numbers
Date: Thu, 27 Jun 2019 01:28:30 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.1

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.

Attachment: 0001-Improve-XFIXNUM-cleanup-a-bit.txt
Description: Text document


reply via email to

[Prev in Thread] Current Thread [Next in Thread]