[PATCH] libasm: Fix use-after-free issue with circular single linked list cleanup

[PATCH] libasm: Fix use-after-free issue with circular single linked list cleanup

[Mark Wielaard]

Pointed out by gcc 12 with -Wuse-after-free=3

In function ‘free_section’
asm_end.c:552:17: error: pointer ‘data’ used after ‘free’ [-Werror=use-after-free]
  552 |     while (oldp != scnp->content);
      |            ~~~~~^~~~~~~~~~~~~~~~
asm_end.c:550:9: note: call to ‘free’ here
  550 |         free (oldp);
      |         ^~~~~~~~~~~

Fix by freeing scnp->content last.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libasm/ChangeLog |  4 ++++
 libasm/asm_end.c | 18 ++++++++++--------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/libasm/ChangeLog b/libasm/ChangeLog
index a12d14b3..f23d5914 100644
--- a/libasm/ChangeLog
+++ b/libasm/ChangeLog
@@ -1,3 +1,7 @@
+2023-02-17  Mark Wielaard  <mark@klomp.org>
+
+	* asm_end.c (free_section): free scnp->content last.
+
 2022-12-20  Mark Wielaard  <mark@klomp.org>
 
 	* disasm_begin.c: Include libeblP.h.
diff --git a/libasm/asm_end.c b/libasm/asm_end.c
index c06d2366..29165ac4 100644
--- a/libasm/asm_end.c
+++ b/libasm/asm_end.c
@@ -541,16 +541,18 @@ free_section (AsmScn_t *scnp)
   if (scnp->subnext != NULL)
     free_section (scnp->subnext);
 
+  /* This is a circular single linked list.  */
   struct AsmData *data = scnp->content;
   if (data != NULL)
-    do
-      {
-	oldp = data;
-	data = data->next;
-	free (oldp);
-      }
-    while (oldp != scnp->content);
-
+    {
+      while (data != scnp->content)
+	{
+	  oldp = data;
+	  data = data->next;
+	  free (oldp);
+	}
+      free (scnp->content);
+    }
   free (scnp);
 }
 
-- 
2.39.1

Re: [PATCH] libasm: Fix use-after-free issue with circular single linked list cleanup

[Mark Wielaard]

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

Hi,

On Fri, 2023-02-17 at 15:00 +0100, Mark Wielaard wrote:
> Pointed out by gcc 12 with -Wuse-after-free=3
> 
> In function ‘free_section’
> asm_end.c:552:17: error: pointer ‘data’ used after ‘free’ [-Werror=use-after-free]
>   552 |     while (oldp != scnp->content);
>       |            ~~~~~^~~~~~~~~~~~~~~~
> asm_end.c:550:9: note: call to ‘free’ here
>   550 |         free (oldp);
>       |         ^~~~~~~~~~~
> 
> Fix by freeing scnp->content last.

I pushed this and also committed the attached patch that adds -Wuse-
after-free=3 if the compiler supports it.

Cheers,

Mark

[-- Attachment #2: Type: text/x-patch, Size: 3425 bytes --]

From c9c055a6949702af57e46f4eac3355d4a4f94c5d Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Tue, 21 Feb 2023 13:12:38 +0100
Subject: [PATCH] configure: Check for and -Wuse-after-free=3 when available

gcc already includes -Wuse-after-free=2 in -Wall. -Wuse-after-free=3
also warns for indeterminate pointers in equality expressions.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 ChangeLog        |  4 ++++
 config/ChangeLog |  5 +++++
 config/eu.am     |  8 ++++++++
 configure.ac     | 11 ++++++++++-
 4 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index d99d837d..5da4f352 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2023-02-21  Mark Wielaard  <mark@klomp.org>
+
+	* configure.ac: Check for -Wuse-after-free=3
+
 2023-02-15  Mark Wielaard  <mark@klomp.org>
 
 	* configure.ac: Error out when demangler is enabled, but
diff --git a/config/ChangeLog b/config/ChangeLog
index c63caa0c..ce1f74f6 100644
--- a/config/ChangeLog
+++ b/config/ChangeLog
@@ -1,3 +1,8 @@
+2023-02-21  Mark Wielaard  <mark@klomp.org>
+
+	* eu.am (USE_AFTER_FREE3_WARNING): Define.
+	(AM_CFLAGS): Use USE_AFTER_FREE3_WARNING.
+
 2022-10-02  Mark Wielaard  <mark@klomp.org>
 
 	* elfutils.spec.in: Update for 0.188.
diff --git a/config/eu.am b/config/eu.am
index c3cefe7e..e6c241f9 100644
--- a/config/eu.am
+++ b/config/eu.am
@@ -1,6 +1,7 @@
 ## Common automake fragments for elfutils subdirectory makefiles.
 ##
 ## Copyright (C) 2010, 2014, 2016 Red Hat, Inc.
+## Copyright (C) 2023, Mark J. Wielaard <mark@klomp.org>
 ##
 ## This file is part of elfutils.
 ##
@@ -87,10 +88,17 @@ else
 NO_PACKED_NOT_ALIGNED_WARNING=
 endif
 
+if HAVE_USE_AFTER_FREE3_WARNING
+USE_AFTER_FREE3_WARNING=-Wuse-after-free=3
+else
+USE_AFTER_FREE3_WARNING=
+endif
+
 AM_CFLAGS = -std=gnu99 -Wall -Wshadow -Wformat=2 \
 	    -Wold-style-definition -Wstrict-prototypes $(TRAMPOLINES_WARNING) \
 	    $(LOGICAL_OP_WARNING) $(DUPLICATED_COND_WARNING) \
 	    $(NULL_DEREFERENCE_WARNING) $(IMPLICIT_FALLTHROUGH_WARNING) \
+	    $(USE_AFTER_FREE3_WARNING) \
 	    $(if $($(*F)_no_Werror),,-Werror) \
 	    $(if $($(*F)_no_Wunused),,-Wunused -Wextra) \
 	    $(if $($(*F)_no_Wstack_usage),,$(STACK_USAGE_WARNING)) \
diff --git a/configure.ac b/configure.ac
index 4c8a4c31..142a89f6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,7 +2,7 @@ dnl Process this file with autoconf to produce a configure script.
 dnl Configure input file for elfutils.                     -*-autoconf-*-
 dnl
 dnl Copyright (C) 1996-2019 Red Hat, Inc.
-dnl Copyright (C) 2022 Mark J. Wielaard <mark@klomp.org>
+dnl Copyright (C) 2022, 2023 Mark J. Wielaard <mark@klomp.org>
 dnl
 dnl This file is part of elfutils.
 dnl
@@ -618,6 +618,15 @@ CFLAGS="$old_CFLAGS"])
 AM_CONDITIONAL(HAVE_NO_PACKED_NOT_ALIGNED_WARNING,
 	       [test "x$ac_cv_no_packed_not_aligned" != "xno"])
 
+AC_CACHE_CHECK([whether the compiler accepts -Wuse-after-free=3], ac_cv_use_after_free3, [dnl
+old_CFLAGS="$CFLAGS"
+CFLAGS="$CFLAGS -Wuse-after-free=3 -Werror"
+AC_COMPILE_IFELSE([AC_LANG_SOURCE([])],
+		  ac_cv_use_after_free3=yes, ac_cv_use_after_free3=no)
+CFLAGS="$old_CFLAGS"])
+AM_CONDITIONAL(HAVE_USE_AFTER_FREE3_WARNING,
+	       [test "x$ac_cv_use_after_free3" != "xno"])
+
 AC_CACHE_CHECK([whether the compiler accepts -fno-addrsig], ac_cv_fno_addrsig, [dnl
 old_CFLAGS="$CFLAGS"
 CFLAGS="$CFLAGS -fno-addrsig -Werror"
-- 
2.39.2