Re: Dynamic modules: MODULE_HANDLE_SIGNALS etc.

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

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.

[-- Attachment #2: 0001-Fix-check-for-subscript-errors-in-module-calls.patch --]
[-- Type: text/x-diff, Size: 1647 bytes --]

From 08b456856baa9e033ee1b210a03373589dd7765a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 1 Apr 2016 01:24:55 -0700
Subject: [PATCH] Fix check for subscript errors in module calls

* src/emacs-module.c (check_vec_index): New function.
(module_vec_set, module_vec_get): Use it instead of a
not-strict-enough check.
---
 src/emacs-module.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/emacs-module.c b/src/emacs-module.c
index f9ede84..b57636e 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -588,13 +588,21 @@ module_set_user_finalizer (emacs_env *env, emacs_value uptr,
 }
 
 static void
+check_vec_index (Lisp_Object lvec, ptrdiff_t i)
+{
+  CHECK_VECTOR (lvec);
+  if (! (0 <= i && i < ASIZE (lvec)))
+    args_out_of_range_3 (make_fixnum_or_float (i),
+			 make_number (0), make_number (ASIZE (lvec) - 1));
+}
+
+static void
 module_vec_set (emacs_env *env, emacs_value vec, ptrdiff_t i, emacs_value val)
 {
   /* FIXME: This function should return bool because it can fail.  */
   MODULE_FUNCTION_BEGIN ();
   Lisp_Object lvec = value_to_lisp (vec);
-  CHECK_VECTOR (lvec);
-  CHECK_RANGED_INTEGER (make_number (i), 0, ASIZE (lvec) - 1);
+  check_vec_index (lvec, i);
   ASET (lvec, i, value_to_lisp (val));
 }
 
@@ -603,8 +611,7 @@ module_vec_get (emacs_env *env, emacs_value vec, ptrdiff_t i)
 {
   MODULE_FUNCTION_BEGIN (module_nil);
   Lisp_Object lvec = value_to_lisp (vec);
-  CHECK_VECTOR (lvec);
-  CHECK_RANGED_INTEGER (make_number (i), 0, ASIZE (lvec) - 1);
+  check_vec_index (lvec, i);
   return lisp_to_value (AREF (lvec, i));
 }
 
-- 
2.5.5