Fix Hurd getcwd build with GCC >= 13

Message ID 18587337-7815-4056-ebd0-724df262d591@codesourcery.com
State Committed, archived
Headers
Series Fix Hurd getcwd build with GCC >= 13 |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Joseph Myers April 26, 2023, 5:14 p.m. UTC
  The build of glibc for i686-gnu has been failing for a while with GCC
mainline / GCC 13:

../sysdeps/mach/hurd/getcwd.c: In function '__hurd_canonicalize_directory_name_internal':
../sysdeps/mach/hurd/getcwd.c:242:48: error: pointer 'file_name' may be used after 'realloc' [-Werror=use-after-free]
  242 |                   file_namep = &buf[file_namep - file_name + size / 2];
      |                                     ~~~~~~~~~~~^~~~~~~~~~~
../sysdeps/mach/hurd/getcwd.c:236:25: note: call to 'realloc' here
  236 |                   buf = realloc (file_name, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~

This appears to be a genuine bug; fix by doing the subtraction before
the reallocation makes the pointer invalid for arithmetic.

Tested with build-many-glibcs.py for i686-gnu.
  

Comments

Samuel Thibault April 26, 2023, 11:29 p.m. UTC | #1
Joseph Myers, le mer. 26 avril 2023 17:14:18 +0000, a ecrit:
> The build of glibc for i686-gnu has been failing for a while with GCC
> mainline / GCC 13:
> 
> ../sysdeps/mach/hurd/getcwd.c: In function '__hurd_canonicalize_directory_name_internal':
> ../sysdeps/mach/hurd/getcwd.c:242:48: error: pointer 'file_name' may be used after 'realloc' [-Werror=use-after-free]
>   242 |                   file_namep = &buf[file_namep - file_name + size / 2];
>       |                                     ~~~~~~~~~~~^~~~~~~~~~~
> ../sysdeps/mach/hurd/getcwd.c:236:25: note: call to 'realloc' here
>   236 |                   buf = realloc (file_name, size);
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This appears to be a genuine bug; fix by doing the subtraction before
> the reallocation makes the pointer invalid for arithmetic.

Well, it's actually not a genuine bug: the values of the file_namep
and file_name pointers are still coherent, so the subtraction is still
valid. But better please gcc 13 indeed, I have pushed it, thanks!

> Tested with build-many-glibcs.py for i686-gnu.
> 
> diff --git a/sysdeps/mach/hurd/getcwd.c b/sysdeps/mach/hurd/getcwd.c
> index f24b35b380..cd3aedd9cd 100644
> --- a/sysdeps/mach/hurd/getcwd.c
> +++ b/sysdeps/mach/hurd/getcwd.c
> @@ -222,8 +222,9 @@ __hurd_canonicalize_directory_name_internal (file_t thisdir,
>        found:
>  	{
>  	  /* Prepend the directory name just discovered.  */
> +	  size_t offset = file_namep - file_name;
>  
> -	  if (file_namep - file_name < d->d_namlen + 1)
> +	  if (offset < d->d_namlen + 1)
>  	    {
>  	      if (orig_size > 0)
>  		{
> @@ -239,7 +240,7 @@ __hurd_canonicalize_directory_name_internal (file_t thisdir,
>  		      free (file_name);
>  		      return NULL;
>  		    }
> -		  file_namep = &buf[file_namep - file_name + size / 2];
> +		  file_namep = &buf[offset + size / 2];
>  		  file_name = buf;
>  		  /* Move current contents up to the end of the buffer.
>  		     This is guaranteed to be non-overlapping.  */
> 
> -- 
> Joseph S. Myers
> joseph@codesourcery.com
  
Joseph Myers April 26, 2023, 11:34 p.m. UTC | #2
On Thu, 27 Apr 2023, Samuel Thibault wrote:

> Well, it's actually not a genuine bug: the values of the file_namep
> and file_name pointers are still coherent, so the subtraction is still
> valid. But better please gcc 13 indeed, I have pushed it, thanks!

It's a bug because "If a pointer value is used in an evaluation after the 
object the pointer points to (or just past) reaches the end of its 
lifetime, the behavior is undefined.  The representation of a pointer 
object becomes indeterminate when the object the pointer points to (or 
just past) reaches the end of its lifetime." (N3096 wording, but the 
principle that you can't use pointers to deallocated memory, even without 
dereferencing them, dates back a lot further).
  

Patch

diff --git a/sysdeps/mach/hurd/getcwd.c b/sysdeps/mach/hurd/getcwd.c
index f24b35b380..cd3aedd9cd 100644
--- a/sysdeps/mach/hurd/getcwd.c
+++ b/sysdeps/mach/hurd/getcwd.c
@@ -222,8 +222,9 @@  __hurd_canonicalize_directory_name_internal (file_t thisdir,
       found:
 	{
 	  /* Prepend the directory name just discovered.  */
+	  size_t offset = file_namep - file_name;
 
-	  if (file_namep - file_name < d->d_namlen + 1)
+	  if (offset < d->d_namlen + 1)
 	    {
 	      if (orig_size > 0)
 		{
@@ -239,7 +240,7 @@  __hurd_canonicalize_directory_name_internal (file_t thisdir,
 		      free (file_name);
 		      return NULL;
 		    }
-		  file_namep = &buf[file_namep - file_name + size / 2];
+		  file_namep = &buf[offset + size / 2];
 		  file_name = buf;
 		  /* Move current contents up to the end of the buffer.
 		     This is guaranteed to be non-overlapping.  */