From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.devel Subject: Re: Dynamic modules: MODULE_HANDLE_SIGNALS etc. Date: Fri, 1 Apr 2016 01:29:36 -0700 Organization: UCLA Computer Science Department Message-ID: <56FE3170.6070208@cs.ucla.edu> References: <83mvu1x6t3.fsf@gnu.org> <565779CD.80405@cs.ucla.edu> <83io4nuc68.fsf@gnu.org> <56D5C627.7000209@cs.ucla.edu> <56D736D2.504@cs.ucla.edu> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------080204070804020607020307" X-Trace: ger.gmane.org 1459499416 24077 80.91.229.3 (1 Apr 2016 08:30:16 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 1 Apr 2016 08:30:16 +0000 (UTC) Cc: aurelien.aptel+emacs@gmail.com, tzz@lifelogs.com, dancol@dancol.org, emacs-devel@gnu.org To: Philipp Stephani , Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Apr 01 10:30:06 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1aluT0-0004VJ-3x for ged-emacs-devel@m.gmane.org; Fri, 01 Apr 2016 10:30:06 +0200 Original-Received: from localhost ([::1]:36594 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aluSz-0000TR-DP for ged-emacs-devel@m.gmane.org; Fri, 01 Apr 2016 04:30:05 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:52643) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aluSk-0000TE-Vm for emacs-devel@gnu.org; Fri, 01 Apr 2016 04:29:51 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aluSj-0005LT-RA for emacs-devel@gnu.org; Fri, 01 Apr 2016 04:29:50 -0400 Original-Received: from zimbra.cs.ucla.edu ([131.179.128.68]:47526) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aluSe-0005KP-Sq; Fri, 01 Apr 2016 04:29:45 -0400 Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id F081A1601D5; Fri, 1 Apr 2016 01:29:40 -0700 (PDT) Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id mmYZzDd6xitF; Fri, 1 Apr 2016 01:29:40 -0700 (PDT) Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 18AB51611FF; Fri, 1 Apr 2016 01:29:40 -0700 (PDT) X-Virus-Scanned: amavisd-new at zimbra.cs.ucla.edu Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id VY5YGO4J6qLx; Fri, 1 Apr 2016 01:29:39 -0700 (PDT) Original-Received: from [192.168.1.9] (unknown [100.32.155.148]) by zimbra.cs.ucla.edu (Postfix) with ESMTPSA id C7CDE1601D5; Fri, 1 Apr 2016 01:29:39 -0700 (PDT) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 In-Reply-To: X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 131.179.128.68 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:202557 Archived-At: This is a multi-part message in MIME format. --------------080204070804020607020307 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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. --------------080204070804020607020307 Content-Type: text/x-diff; name="0001-Fix-check-for-subscript-errors-in-module-calls.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-Fix-check-for-subscript-errors-in-module-calls.patch" >From 08b456856baa9e033ee1b210a03373589dd7765a Mon Sep 17 00:00:00 2001 From: Paul Eggert 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 --------------080204070804020607020307--