bug#51172: Fix null-dereference warnings when compiling Emacs with GCC

bug#51172: Fix null-dereference warnings when compiling Emacs with GCC

[Paul Eggert]

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

The attached patch against emacs-28 suppresses the final remaining GCC 
11.2.1 diagnostics that are emitted after emacs-28 is configured with 
--enable-gcc-warnings on x86-64.

The patch is benign, and it's conceivable that changing xmalloc etc. to 
always return nonnull fixes unlikely and obscure bugs (though I haven't 
checked this). However, I didn't install the patch into the emacs-28 
branch on the off-chance that Eli would prefer this sort of thing to be 
installed into the master branch.

[-- Attachment #2: 0001-Pacify-GCC-Wanalyzer-possible-null-dereference.patch --]
[-- Type: text/x-patch, Size: 11413 bytes --]

From 2accf49753fe151a53d485c4756ddc92d53d0b3f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 12 Oct 2021 17:25:54 -0700
Subject: [PATCH] Pacify GCC -Wanalyzer-possible-null-dereference
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This fixes the only remaining GCC diagnostics when emacs-28 is
configured with --enable-gcc-warnings.  It does so by adding
ATTRIBUTE_RETURNS_NONNULL so that GCC knows certain functions
return nonnull.  It also arranges for three of those functions to
always return nonnull; I thought these functions already were
doing so, but apparently not, and it is conceivable (though I
haven’t checked this) that changing these functions to always
return nonnull even on non-GNU platforms may fix unlikely
portability bugs elsewhere in Emacs.  I used GCC 11.2.1 20210728
(Red Hat 11.2.1-1) on x86-64 when checking the diagnostics.
* src/alloc.c (xmalloc, xzalloc, xrealloc): Don’t worry about the
special case where SIZE == 0, since lmalloc and lrealloc now
return null only on allocation failure.
(lmalloc, lrealloc): Return null only on allocation failure,
instead of having special cases that treat malloc (0) and
realloc (X, 0) as successes even when they return null.
* src/lisp.h: Add ATTRIBUTE_RETURNS_NONNULL to a few functions
that always return nonnull pointers, so that gcc -fanalyzer
does not issue diagnostics like “alloc.c: In function
‘allocate_vector_block’: alloc.c:2985:15: warning: dereference of
possibly-NULL ‘block’ [CWE-690] [-Wanalyzer-possible-null-dereference]”
as per <https://cwe.mitre.org/data/definitions/690.html>.
---
 src/alloc.c | 40 ++++++++++++++++++++++++----------------
 src/lisp.h  | 51 ++++++++++++++++++++++++++++++++-------------------
 2 files changed, 56 insertions(+), 35 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 0c04d5cde0..b1f8064a9d 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -765,7 +765,7 @@ xmalloc (size_t size)
   val = lmalloc (size, false);
   MALLOC_UNBLOCK_INPUT;
 
-  if (!val && size)
+  if (!val)
     memory_full (size);
   MALLOC_PROBE (size);
   return val;
@@ -782,7 +782,7 @@ xzalloc (size_t size)
   val = lmalloc (size, true);
   MALLOC_UNBLOCK_INPUT;
 
-  if (!val && size)
+  if (!val)
     memory_full (size);
   MALLOC_PROBE (size);
   return val;
@@ -796,15 +796,15 @@ xrealloc (void *block, size_t size)
   void *val;
 
   MALLOC_BLOCK_INPUT;
-  /* We must call malloc explicitly when BLOCK is 0, since some
-     reallocs don't do this.  */
+  /* Call malloc when BLOCK is null,
+     since lrealloc does not allow a null BLOCK.  */
   if (! block)
     val = lmalloc (size, false);
   else
     val = lrealloc (block, size);
   MALLOC_UNBLOCK_INPUT;
 
-  if (!val && size)
+  if (!val)
     memory_full (size);
   MALLOC_PROBE (size);
   return val;
@@ -988,8 +988,7 @@ record_xmalloc (size_t size)
 
 /* Like malloc but used for allocating Lisp data.  NBYTES is the
    number of bytes to allocate, TYPE describes the intended use of the
-   allocated memory block (for strings, for conses, ...).
-   NBYTES must be positive.  */
+   allocated memory block (for strings, for conses, ...).  */
 
 #if ! USE_LSB_TAG
 void *lisp_malloc_loser EXTERNALLY_VISIBLE;
@@ -1330,16 +1329,20 @@ laligned (void *p, size_t size)
 	  || size % LISP_ALIGNMENT != 0);
 }
 
-/* Like malloc and realloc except that if SIZE is Lisp-aligned, make
-   sure the result is too, if necessary by reallocating (typically
-   with larger and larger sizes) until the allocator returns a
-   Lisp-aligned pointer.  Code that needs to allocate C heap memory
+/* Like malloc and realloc except return null only on failure,
+   the result is Lisp-aligned if SIZE is, and lrealloc's pointer
+   argument must be nonnull.  Code allocating C heap memory
    for a Lisp object should use one of these functions to obtain a
    pointer P; that way, if T is an enum Lisp_Type value and L ==
    make_lisp_ptr (P, T), then XPNTR (L) == P and XTYPE (L) == T.
 
+   If CLEARIT, arrange for the allocated memory to be cleared.
+   This might use calloc, as calloc can be faster than malloc+memset.
+
    On typical modern platforms these functions' loops do not iterate.
-   On now-rare (and perhaps nonexistent) platforms, the loops in
+   On now-rare (and perhaps nonexistent) platforms, the code can loop,
+   reallocating (typically with larger and larger sizes) until the
+   allocator returns a Lisp-aligned pointer.  This loop in
    theory could repeat forever.  If an infinite loop is possible on a
    platform, a build would surely loop and the builder can then send
    us a bug report.  Adding a counter to try to detect any such loop
@@ -1353,8 +1356,13 @@ lmalloc (size_t size, bool clearit)
   if (! MALLOC_IS_LISP_ALIGNED && size % LISP_ALIGNMENT == 0)
     {
       void *p = aligned_alloc (LISP_ALIGNMENT, size);
-      if (clearit && p)
-	memclear (p, size);
+      if (p)
+	{
+	  if (clearit)
+	    memclear (p, size);
+	}
+      else if (! (MALLOC_0_IS_NONNULL || size))
+	return aligned_alloc (LISP_ALIGNMENT, LISP_ALIGNMENT);
       return p;
     }
 #endif
@@ -1362,7 +1370,7 @@ lmalloc (size_t size, bool clearit)
   while (true)
     {
       void *p = clearit ? calloc (1, size) : malloc (size);
-      if (laligned (p, size))
+      if (laligned (p, size) && (MALLOC_0_IS_NONNULL || size || p))
 	return p;
       free (p);
       size_t bigger = size + LISP_ALIGNMENT;
@@ -1377,7 +1385,7 @@ lrealloc (void *p, size_t size)
   while (true)
     {
       p = realloc (p, size);
-      if (laligned (p, size))
+      if (laligned (p, size) && (size || p))
 	return p;
       size_t bigger = size + LISP_ALIGNMENT;
       if (size < bigger)
diff --git a/src/lisp.h b/src/lisp.h
index 480c389a3b..31656bb3b1 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3947,7 +3947,8 @@ build_string (const char *str)
 
 extern Lisp_Object pure_cons (Lisp_Object, Lisp_Object);
 extern Lisp_Object make_vector (ptrdiff_t, Lisp_Object);
-extern struct Lisp_Vector *allocate_nil_vector (ptrdiff_t);
+extern struct Lisp_Vector *allocate_nil_vector (ptrdiff_t)
+  ATTRIBUTE_RETURNS_NONNULL;
 
 /* Make an uninitialized vector for SIZE objects.  NOTE: you must
    be sure that GC cannot happen until the vector is completely
@@ -3960,7 +3961,8 @@ build_string (const char *str)
 
    allocate_vector has a similar problem.  */
 
-extern struct Lisp_Vector *allocate_vector (ptrdiff_t);
+extern struct Lisp_Vector *allocate_vector (ptrdiff_t)
+  ATTRIBUTE_RETURNS_NONNULL;
 
 INLINE Lisp_Object
 make_uninit_vector (ptrdiff_t size)
@@ -3992,7 +3994,8 @@ make_nil_vector (ptrdiff_t size)
 }
 
 extern struct Lisp_Vector *allocate_pseudovector (int, int, int,
-						  enum pvec_type);
+						  enum pvec_type)
+  ATTRIBUTE_RETURNS_NONNULL;
 
 /* Allocate uninitialized pseudovector with no Lisp_Object slots.  */
 
@@ -4024,7 +4027,7 @@ #define ALLOCATE_ZEROED_PSEUDOVECTOR(type, field, tag)		       \
 extern void init_alloc_once (void);
 extern void init_alloc (void);
 extern void syms_of_alloc (void);
-extern struct buffer * allocate_buffer (void);
+extern struct buffer *allocate_buffer (void) ATTRIBUTE_RETURNS_NONNULL;
 extern int valid_lisp_object_p (Lisp_Object);
 
 /* Defined in gmalloc.c.  */
@@ -4182,7 +4185,8 @@ xsignal (Lisp_Object error_symbol, Lisp_Object data)
     (Lisp_Object (*) (ptrdiff_t, Lisp_Object *), ptrdiff_t, Lisp_Object *,
      Lisp_Object, Lisp_Object (*) (Lisp_Object, ptrdiff_t, Lisp_Object *));
 extern Lisp_Object internal_catch_all (Lisp_Object (*) (void *), void *, Lisp_Object (*) (enum nonlocal_exit, Lisp_Object));
-extern struct handler *push_handler (Lisp_Object, enum handlertype);
+extern struct handler *push_handler (Lisp_Object, enum handlertype)
+  ATTRIBUTE_RETURNS_NONNULL;
 extern struct handler *push_handler_nosignal (Lisp_Object, enum handlertype);
 extern void specbind (Lisp_Object, Lisp_Object);
 extern void record_unwind_protect (void (*) (Lisp_Object), Lisp_Object);
@@ -4323,9 +4327,10 @@ XMODULE_FUNCTION (Lisp_Object o)
 
 /* Defined in fileio.c.  */
 
-extern char *splice_dir_file (char *, char const *, char const *);
+extern char *splice_dir_file (char *, char const *, char const *)
+  ATTRIBUTE_RETURNS_NONNULL;
 extern bool file_name_absolute_p (const char *);
-extern char const *get_homedir (void);
+extern char const *get_homedir (void) ATTRIBUTE_RETURNS_NONNULL;
 extern Lisp_Object expand_and_dir_to_file (Lisp_Object);
 extern Lisp_Object write_region (Lisp_Object, Lisp_Object, Lisp_Object,
 				 Lisp_Object, Lisp_Object, Lisp_Object,
@@ -4479,7 +4484,7 @@ fast_string_match_ignore_case (Lisp_Object regexp, Lisp_Object string)
 INLINE void synchronize_system_messages_locale (void) {}
 INLINE void synchronize_system_time_locale (void) {}
 #endif
-extern char *emacs_strerror (int);
+extern char *emacs_strerror (int) ATTRIBUTE_RETURNS_NONNULL;
 extern void shut_down_emacs (int, Lisp_Object);
 
 /* True means don't do interactive redisplay and don't change tty modes.  */
@@ -4545,7 +4550,7 @@ #define DAEMON_RUNNING (w32_daemon_event != INVALID_HANDLE_VALUE)
 
 extern int emacs_spawn (pid_t *, int, int, int, char **, char **,
                         const char *, const char *, const sigset_t *);
-extern char **make_environment_block (Lisp_Object);
+extern char **make_environment_block (Lisp_Object) ATTRIBUTE_RETURNS_NONNULL;
 extern void init_callproc_1 (void);
 extern void init_callproc (void);
 extern void set_initial_environment (void);
@@ -4814,17 +4819,24 @@ SUBR_NATIVE_COMPILED_DYNP (Lisp_Object a)
 extern char my_endbss[];
 extern char *my_endbss_static;
 
-extern void *xmalloc (size_t) ATTRIBUTE_MALLOC_SIZE ((1));
-extern void *xzalloc (size_t) ATTRIBUTE_MALLOC_SIZE ((1));
-extern void *xrealloc (void *, size_t) ATTRIBUTE_ALLOC_SIZE ((2));
+extern void *xmalloc (size_t)
+  ATTRIBUTE_MALLOC_SIZE ((1)) ATTRIBUTE_RETURNS_NONNULL;
+extern void *xzalloc (size_t)
+  ATTRIBUTE_MALLOC_SIZE ((1)) ATTRIBUTE_RETURNS_NONNULL;
+extern void *xrealloc (void *, size_t)
+  ATTRIBUTE_ALLOC_SIZE ((2)) ATTRIBUTE_RETURNS_NONNULL;
 extern void xfree (void *);
-extern void *xnmalloc (ptrdiff_t, ptrdiff_t) ATTRIBUTE_MALLOC_SIZE ((1,2));
+extern void *xnmalloc (ptrdiff_t, ptrdiff_t)
+  ATTRIBUTE_MALLOC_SIZE ((1,2)) ATTRIBUTE_RETURNS_NONNULL;
 extern void *xnrealloc (void *, ptrdiff_t, ptrdiff_t)
-  ATTRIBUTE_ALLOC_SIZE ((2,3));
-extern void *xpalloc (void *, ptrdiff_t *, ptrdiff_t, ptrdiff_t, ptrdiff_t);
-
-extern char *xstrdup (const char *) ATTRIBUTE_MALLOC;
-extern char *xlispstrdup (Lisp_Object) ATTRIBUTE_MALLOC;
+  ATTRIBUTE_ALLOC_SIZE ((2,3)) ATTRIBUTE_RETURNS_NONNULL;
+extern void *xpalloc (void *, ptrdiff_t *, ptrdiff_t, ptrdiff_t, ptrdiff_t)
+  ATTRIBUTE_RETURNS_NONNULL;
+
+extern char *xstrdup (char const *)
+  ATTRIBUTE_MALLOC ATTRIBUTE_RETURNS_NONNULL;
+extern char *xlispstrdup (Lisp_Object)
+  ATTRIBUTE_MALLOC ATTRIBUTE_RETURNS_NONNULL;
 extern void dupstring (char **, char const *);
 
 /* Make DEST a copy of STRING's data.  Return a pointer to DEST's terminating
@@ -4874,7 +4886,8 @@ #define eabs(x)         ((x) < 0 ? -(x) : (x))
 
 enum MAX_ALLOCA { MAX_ALLOCA = 16 * 1024 };
 
-extern void *record_xmalloc (size_t) ATTRIBUTE_ALLOC_SIZE ((1));
+extern void *record_xmalloc (size_t)
+  ATTRIBUTE_ALLOC_SIZE ((1)) ATTRIBUTE_RETURNS_NONNULL;
 
 #define USE_SAFE_ALLOCA			\
   ptrdiff_t sa_avail = MAX_ALLOCA;	\
-- 
2.31.1

bug#51172: Fix null-dereference warnings when compiling Emacs with GCC

[Eli Zaretskii]

> Date: Tue, 12 Oct 2021 17:34:11 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> The attached patch against emacs-28 suppresses the final remaining GCC 
> 11.2.1 diagnostics that are emitted after emacs-28 is configured with 
> --enable-gcc-warnings on x86-64.
> 
> The patch is benign, and it's conceivable that changing xmalloc etc. to 
> always return nonnull fixes unlikely and obscure bugs (though I haven't 
> checked this). However, I didn't install the patch into the emacs-28 
> branch on the off-chance that Eli would prefer this sort of thing to be 
> installed into the master branch.

Yes, please install on master.  We don't expect users to configure the
released Emacs with --enable-gcc-warnings, and the changes are
non-trivial.

> -  /* We must call malloc explicitly when BLOCK is 0, since some
> -     reallocs don't do this.  */
> +  /* Call malloc when BLOCK is null,
> +     since lrealloc does not allow a null BLOCK.  */

Typo in this comment: "malloc" should be "lmalloc".

Thanks.

bug#51172: Fix null-dereference warnings when compiling Emacs with GCC

[Paul Eggert]

On 10/13/21 5:14 AM, Eli Zaretskii wrote:

> Yes, please install on master.

Thanks, done, and closing the bug report. I also got another diagnostic 
in the master branch, and fixed that in a separate commit.