[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.
0001-Improve-XFIXNUM-cleanup-a-bit.txt
Description: Text document
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Pip Cet, 2019/06/25
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Paul Eggert, 2019/06/26
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Pip Cet, 2019/06/27
- bug#36370: 27.0.50; XFIXNAT called on negative numbers,
Paul Eggert <=
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Pip Cet, 2019/06/27
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Eli Zaretskii, 2019/06/27
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Paul Eggert, 2019/06/27
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Pip Cet, 2019/06/27
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Paul Eggert, 2019/06/27
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Pip Cet, 2019/06/27
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Bruno Haible, 2019/06/27
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Paul Eggert, 2019/06/27
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Pip Cet, 2019/06/28
- bug#36370: 27.0.50; XFIXNAT called on negative numbers, Bruno Haible, 2019/06/28