Subject: bug#41936: closed (Re: bug#41936: 28.0.50; AREF: assert that the index is inside bounds)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 18 Jun 2020 21:07:01 +0000

Your bug report

#41936: 28.0.50; AREF: assert that the index is inside bounds

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
41936: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=41936
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#41936: 28.0.50; AREF: assert that the index is inside bounds
From: Paul Eggert <[email-masked]>
To: Tino Calancha <[email-masked]>
Cc: [email-masked], eli zaretskii <[email-masked]>, [email-masked]
Date: Thu, 18 Jun 2020 14:06:31 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 6/18/20 1:12 PM, Tino Calancha wrote:
> Is it OK for you to add the following patch?

Yes, good idea. I wondered a while ago (to myself) why AREF doesn't check
subscripts when Emacs is configured with --enable-checking. Now that I think
about it more, it's most likely because AREF was a macro and didn't want to
evaluate its index argument multiple times. We don't need to worry about that
any more.

aref_addr should have a similar check (off by one since one can address one past
the end of an array).

There's no need to change test/manual/etags/c-src/emacs/src/lisp.h as that's
just a data file (and changes can be harmful there as they can mess up the tests).

I installed the attached.
>From e14eec7cd4a4217a0908a35415610e0fdb8604f0 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Thu, 18 Jun 2020 14:01:56 -0700
Subject: [PATCH] Check AREF and aref_addr subscripts

* src/lisp.h (gc_asize): Move before first use.
(AREF, aref_addr): Check subscripts.
Co-authored-by: Tino Calancha <[email-masked]>
---
 src/lisp.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/lisp.h b/src/lisp.h
index 3442699088..7b4f484b9b 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1671,6 +1671,13 @@ ASIZE (Lisp_Object array)
   return size;
 }
 
+INLINE ptrdiff_t
+gc_asize (Lisp_Object array)
+{
+  /* Like ASIZE, but also can be used in the garbage collector.  */
+  return XVECTOR (array)->header.size & ~ARRAY_MARK_FLAG;
+}
+
 INLINE ptrdiff_t
 PVSIZE (Lisp_Object pv)
 {
@@ -1853,22 +1860,17 @@ bool_vector_set (Lisp_Object a, EMACS_INT i, bool b)
 INLINE Lisp_Object
 AREF (Lisp_Object array, ptrdiff_t idx)
 {
+  eassert (0 <= idx && idx < gc_asize (array));
   return XVECTOR (array)->contents[idx];
 }
 
 INLINE Lisp_Object *
 aref_addr (Lisp_Object array, ptrdiff_t idx)
 {
+  eassert (0 <= idx && idx <= gc_asize (array));
   return & XVECTOR (array)->contents[idx];
 }
 
-INLINE ptrdiff_t
-gc_asize (Lisp_Object array)
-{
-  /* Like ASIZE, but also can be used in the garbage collector.  */
-  return XVECTOR (array)->header.size & ~ARRAY_MARK_FLAG;
-}
-
 INLINE void
 ASET (Lisp_Object array, ptrdiff_t idx, Lisp_Object val)
 {
-- 
2.17.1

Subject: 28.0.50; AREF: assert that the index is inside bounds
From: Tino Calancha <[email-masked]>
To: [email-masked]
Date: Thu, 18 Jun 2020 22:12:20 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

Severity: wishlist,patch
X-Debbugs-Cc: Paul Eggert <[email-masked]>, Eli Zaretskii <[email-masked]>, <[email-masked]>

I was bitten by an out-of-bounds index at AREF while working
in a new feature.

A similar assert as we do in ASET would have allowed me
to diagnostic the bug in minutes; instead, it took me
few days to realize the bug.

Is it OK for you to add the following patch?

--8<-----------------------------cut here---------------start------------->8---
commit 8d904d41fcb8ef29ac8205761077a11f900916bc
Author: Tino Calancha <[email-masked]>
Date:   Thu Jun 18 22:01:07 2020 +0200

    AREF: assert that the index is inside bounds
    
    * src/lisp.h (gc_asize): Move before first use.
      (AREF): Assert the index is inside its bounds.
    * test/manual/etags/c-src/emacs/src/lisp.h (AREF):
      Same.

diff --git a/src/lisp.h b/src/lisp.h
index 3442699088..21722e4a78 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1671,6 +1671,13 @@ ASIZE (Lisp_Object array)
   return size;
 }
 
+INLINE ptrdiff_t
+gc_asize (Lisp_Object array)
+{
+  /* Like ASIZE, but also can be used in the garbage collector.  */
+  return XVECTOR (array)->header.size & ~ARRAY_MARK_FLAG;
+}
+
 INLINE ptrdiff_t
 PVSIZE (Lisp_Object pv)
 {
@@ -1853,6 +1860,7 @@ bool_vector_set (Lisp_Object a, EMACS_INT i, bool b)
 INLINE Lisp_Object
 AREF (Lisp_Object array, ptrdiff_t idx)
 {
+  eassert (0 <= idx && idx < gc_asize (array));
   return XVECTOR (array)->contents[idx];
 }
 
@@ -1862,13 +1870,6 @@ aref_addr (Lisp_Object array, ptrdiff_t idx)
   return & XVECTOR (array)->contents[idx];
 }
 
-INLINE ptrdiff_t
-gc_asize (Lisp_Object array)
-{
-  /* Like ASIZE, but also can be used in the garbage collector.  */
-  return XVECTOR (array)->header.size & ~ARRAY_MARK_FLAG;
-}
-
 INLINE void
 ASET (Lisp_Object array, ptrdiff_t idx, Lisp_Object val)
 {
diff --git a/test/manual/etags/c-src/emacs/src/lisp.h b/test/manual/etags/c-src/emacs/src/lisp.h
index eceef4c00d..b2e32554c3 100644
--- a/test/manual/etags/c-src/emacs/src/lisp.h
+++ b/test/manual/etags/c-src/emacs/src/lisp.h
@@ -1478,6 +1478,7 @@ enum
 INLINE Lisp_Object
 AREF (Lisp_Object array, ptrdiff_t idx)
 {
+  eassert (0 <= idx && idx < gc_asize (array));
   return XVECTOR (array)->contents[idx];
 }

--8<-----------------------------cut here---------------end--------------->8---

In GNU Emacs 28.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2020-06-18 built on calancha-pc.dy.bbexcite.jp
Repository revision: ba450b6f462e278fcd3bc96c88f154fce219f5fc
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)