Re: Dynamic modules: MODULE_HANDLE_SIGNALS etc.

[Paul Eggert]

Philipp Stephani wrote:

> Why did you remove the checks? I think all of them are necessary
> and lead to undefined behavior if they are violated.

Looking at my patch:

> -      verify (EMACS_INT_MAX > MOST_POSITIVE_FIXNUM);

EMACS_INT_MAX cannot possibly be less than MOST_POSITIVE_FIXNUM, since 
MOST_POSITIVE_FIXNUM is an EMACS_INT.

>        EMACS_INT refcount = XFASTINT (value) + 1;
> -      if (FIXNUM_OVERFLOW_P (refcount)) xsignal0 (Qoverflow_error);
> +      if (MOST_POSITIVE_FIXNUM < refcount)
> +       xsignal0 (Qoverflow_error);

Adding 1 to the result of XFASTINT cannot possibly yield an integer that is less 
than MOST_NEGATIVE_FIXNUM, so there's no need for the extra runtime check that 
FIXNUM_OVERFLOW_P would impose.

>    ptrdiff_t raw_size = SBYTES (lisp_str_utf8);
> -  if (raw_size == PTRDIFF_MAX) xsignal0 (Qoverflow_error);

raw_size cannot possibly be PTRDIFF_MAX, since SBYTES always returns a value no 
greater than STRING_BYTES_BOUND, and STRING_BYTES_BOUND is less than PTRDIFF_MAX.

> -  if (length > STRING_BYTES_BOUND) xsignal0 (Qoverflow_error);
>    Lisp_Object lstr = make_unibyte_string (str, length);

make_unibyte_string already checks for string length overflow, so the caller 
need not check this.

> -  if (FIXNUM_OVERFLOW_P (i)) xsignal0 (Qoverflow_error);
>    CHECK_RANGED_INTEGER (make_number (i), 0, ASIZE (lvec) - 1);

CHECK_RANGED_INTEGER already checks that the integer is in range, so the caller 
doesn't need to check that again. Hmm, here, though, there is a problem, in that 
make_number can silently overflow. Sorry about that. I fixed this by installing 
the attached further patch.


> if one of the used functions is
> changed to throw signals

We shouldn't have to worry about that.  Changing core functions to throw signals 
would break lots of other code.  We don't need to burden readers of these common 
core functions with every single design constraint that affects them. It'd be OK 
to put this commentary somewhere else, just not in a place where it clutters up 
the main code.

0001-Fix-check-for-subscript-errors-in-module-calls.patch
Description: Text Data