Re: glibc 2.36 build failure on Fedora 36 with gcc 12 (-Werror=use-after-free)

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

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

On 7/29/22 08:26, Carlos O'Donell via Libc-alpha wrote:
> Just posting here for the record that I see a glibc 2.36 build failure
> with Fedora 36 and gcc 12.
> 
> localealias.c: In function ‘read_alias_file’:
> localealias.c:335:56: error: pointer may be used after ‘realloc’ [-Werror=use-after-free]
>    335 |                               map[i].alias += new_pool - string_space;
>        |                                               ~~~~~~~~~^~~~~~~~~~~~~~
> localealias.c:325:49: note: call to ‘realloc’ here
>    325 |                       char *new_pool = (char *) realloc (string_space, new_size);
>        |                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> I'm going to review this quickly before we make the glibc release.
> 

One option is to fix the conformance bug rather than continue to try to 
paper it over with a pragma.

In looking into it, I found two kinds of conformance bugs in 
localealias.c: first, accessing a realloced pointer (which GCC warns 
about), and second, calling a function via an incompatible function 
pointer (which GCC doesn't warn about). Proposed patch attached. I have 
compiled but not tested this.

[-- Attachment #2: 0001-Avoid-undefined-behavior-in-localealias.c.patch --]
[-- Type: text/x-patch, Size: 5831 bytes --]

From c6a660aaae9efab49ed670971aa1dad7ed011e22 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 29 Jul 2022 09:33:55 -0700
Subject: [PATCH] Avoid undefined behavior in localealias.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Do not access a pointer after realloc, and do not
assume that function pointers can be cast willy-nilly.
Problem reported by Carlos O’Donell in:
https://sourceware.org/pipermail/libc-alpha/2022-July/141127.html
* intl/localealias.c (struct alias_map): Members are
now offsets from string_space, not pointers, so that
they survive realloc.  All uses changed.
(_nl_expand_alias): Pass name as-is to bsearch, and
use bsearch_alias_compare which knows about this.
(_nl_expand_alias, read_alias_file):
Do not cast function pointers in a way that violates the C standard.
(read_alias_file): Remove no-longer-needed pragma
and code to adjust pointers after realloc.
Pass qsort_alias_compare to qsort.
(alias_compare): Change signature so that it merely compares strings.
(qsort_alias_compare, bsearch_alias_compare): New functions.
---
 intl/localealias.c | 80 +++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 47 deletions(-)

diff --git a/intl/localealias.c b/intl/localealias.c
index b36092363a..56b4ea58a4 100644
--- a/intl/localealias.c
+++ b/intl/localealias.c
@@ -121,8 +121,9 @@ __libc_lock_define_initialized (static, lock)
 
 struct alias_map
 {
-  const char *alias;
-  const char *value;
+  /* These are offsets from string_space, so that they survive realloc.  */
+  size_t alias;
+  size_t value;
 };
 
 
@@ -141,8 +142,8 @@ static size_t maxmap;
 /* Prototypes for local functions.  */
 static size_t read_alias_file (const char *fname, int fname_len);
 static int extend_alias_table (void);
-static int alias_compare (const struct alias_map *map1,
-			  const struct alias_map *map2);
+static int qsort_alias_compare (void const *map1, void const *map2);
+static int bsearch_alias_compare (void const *key, void const *elt);
 
 
 const char *
@@ -160,23 +161,17 @@ _nl_expand_alias (const char *name)
 
   do
     {
-      struct alias_map item;
-
-      item.alias = name;
-
       if (nmap > 0)
-	retval = (struct alias_map *) bsearch (&item, map, nmap,
+	retval = (struct alias_map *) bsearch (name, map, nmap,
 					       sizeof (struct alias_map),
-					       (int (*) (const void *,
-							 const void *)
-						) alias_compare);
+					       bsearch_alias_compare);
       else
 	retval = NULL;
 
       /* We really found an alias.  Return the value.  */
       if (retval != NULL)
 	{
-	  result = retval->value;
+	  result = string_space + retval->value;
 	  break;
 	}
 
@@ -318,14 +313,6 @@ read_alias_file (const char *fname, int fname_len)
 
 		  if (string_space_act + alias_len + value_len > string_space_max)
 		    {
-#pragma GCC diagnostic push
-
-#if defined __GNUC__ && __GNUC__ >= 12
-  /* Suppress the valid GCC 12 warning until the code below is changed
-     to avoid using pointers to the reallocated block.  */
-#  pragma GCC diagnostic ignored "-Wuse-after-free"
-#endif
-
 		    /* Increase size of memory pool.  */
 		      size_t new_size = (string_space_max
 					 + (alias_len + value_len > 1024
@@ -334,33 +321,18 @@ read_alias_file (const char *fname, int fname_len)
 		      if (new_pool == NULL)
 			goto out;
 
-		      if (__builtin_expect (string_space != new_pool, 0))
-			{
-			  size_t i;
-
-			  for (i = 0; i < nmap; i++)
-			    {
-			      map[i].alias += new_pool - string_space;
-			      map[i].value += new_pool - string_space;
-			    }
-			}
-
 		      string_space = new_pool;
 		      string_space_max = new_size;
 		    }
 
-		  map[nmap].alias =
-		    (const char *) memcpy (&string_space[string_space_act],
-					   alias, alias_len);
+		  memcpy (&string_space[string_space_act], alias, alias_len);
+		  map[nmap].alias = string_space_act;
 		  string_space_act += alias_len;
 
-		  map[nmap].value =
-		    (const char *) memcpy (&string_space[string_space_act],
-					   value, value_len);
+		  memcpy (&string_space[string_space_act], value, value_len);
+		  map[nmap].value = string_space_act;
 		  string_space_act += value_len;
 
-#pragma GCC diagnostic pop
-
 		  ++nmap;
 		  ++added;
 		}
@@ -384,8 +356,7 @@ read_alias_file (const char *fname, int fname_len)
   fclose (fp);
 
   if (added > 0)
-    qsort (map, nmap, sizeof (struct alias_map),
-	   (int (*) (const void *, const void *)) alias_compare);
+    qsort (map, nmap, sizeof (struct alias_map), qsort_alias_compare);
 
   return added;
 }
@@ -409,15 +380,14 @@ extend_alias_table (void)
   return 0;
 }
 
-
 static int
-alias_compare (const struct alias_map *map1, const struct alias_map *map2)
+alias_compare (char const *alias1, char const *alias2)
 {
 #if defined _LIBC || defined HAVE_STRCASECMP
-  return strcasecmp (map1->alias, map2->alias);
+  return strcasecmp (alias1, alias2);
 #else
-  const unsigned char *p1 = (const unsigned char *) map1->alias;
-  const unsigned char *p2 = (const unsigned char *) map2->alias;
+  const unsigned char *p1 = (const unsigned char *) alias1;
+  const unsigned char *p2 = (const unsigned char *) alias2;
   unsigned char c1, c2;
 
   if (p1 == p2)
@@ -439,3 +409,19 @@ alias_compare (const struct alias_map *map1, const struct alias_map *map2)
   return c1 - c2;
 #endif
 }
+
+static int
+qsort_alias_compare (void const *vmap1, void const *vmap2)
+{
+  struct alias_map const *map1 = vmap1;
+  struct alias_map const *map2 = vmap2;
+  return alias_compare (string_space + map1->alias,
+			string_space + map2->alias);
+}
+
+static int
+bsearch_alias_compare (void const *vkey, void const *velt)
+{
+  struct alias_map const *elt = velt;
+  return alias_compare (vkey, string_space + elt->alias);
+}
-- 
2.37.1