_dl_map_object_deps: Use struct scratch_buffer [BZ #18023]

Message ID 20180626180542.5AAAD43994575@oldenburg.str.redhat.com
State Committed
Headers

Commit Message

Florian Weimer June 26, 2018, 6:05 p.m. UTC
  The function comment suggests that _dl_map_object_deps cannot use
malloc, but it already allocates the l_initfini array on the heap, so
the additional allocation should be acceptable.

2018-06-26  Florian Weimer  <fweimer@redhat.com>

	[BZ #18023]
	* elf/dl-deps.c (_dl_map_object_deps): Use struct
	scratch_buffer instead of extend_alloca.
  

Comments

Carlos O'Donell June 27, 2018, 3:34 p.m. UTC | #1
On 06/26/2018 02:05 PM, Florian Weimer wrote:
> The function comment suggests that _dl_map_object_deps cannot use
> malloc, but it already allocates the l_initfini array on the heap, so
> the additional allocation should be acceptable.
> 
> 2018-06-26  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #18023]
> 	* elf/dl-deps.c (_dl_map_object_deps): Use struct
> 	scratch_buffer instead of extend_alloca.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 
> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index 20b8e94f2e..9d9b1ba7f2 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -27,6 +27,7 @@
>  #include <unistd.h>
>  #include <sys/param.h>
>  #include <ldsodefs.h>
> +#include <scratch_buffer.h>
>  
>  #include <dl-dst.h>
>  
> @@ -181,9 +182,8 @@ _dl_map_object_deps (struct link_map *map,
>    /* Pointer to last unique object.  */
>    tail = &known[nlist - 1];
>  
> -  /* No alloca'd space yet.  */
> -  struct link_map **needed_space = NULL;
> -  size_t needed_space_bytes = 0;
> +  struct scratch_buffer needed_space;
> +  scratch_buffer_init (&needed_space);

OK.

>  
>    /* Process each element of the search list, loading each of its
>       auxiliary objects and immediate dependencies.  Auxiliary objects
> @@ -213,13 +213,12 @@ _dl_map_object_deps (struct link_map *map,
>        if (l->l_searchlist.r_list == NULL && l->l_initfini == NULL
>  	  && l != map && l->l_ldnum > 0)
>  	{
> -	  size_t new_size = l->l_ldnum * sizeof (struct link_map *);
> -
> -	  if (new_size > needed_space_bytes)
> -	    needed_space
> -	      = extend_alloca (needed_space, needed_space_bytes, new_size);
> -
> -	  needed = needed_space;
> +	  /* l->l_ldnum includes space for the terminating NULL.  */
> +	  if (!scratch_buffer_set_array_size
> +	      (&needed_space, l->l_ldnum, sizeof (struct link_map *)))
> +	    _dl_signal_error (ENOMEM, map->l_name, NULL,
> +			      N_("cannot allocate dependency buffer"));
> +	  needed = needed_space.data;

OK.

>  	}
>  
>        if (l->l_info[DT_NEEDED] || l->l_info[AUXTAG] || l->l_info[FILTERTAG])
> @@ -438,8 +437,11 @@ _dl_map_object_deps (struct link_map *map,
>  	  struct link_map **l_initfini = (struct link_map **)
>  	    malloc ((2 * nneeded + 1) * sizeof needed[0]);
>  	  if (l_initfini == NULL)
> -	    _dl_signal_error (ENOMEM, map->l_name, NULL,
> -			      N_("cannot allocate dependency list"));
> +	    {
> +	      scratch_buffer_free (&needed_space);
> +	      _dl_signal_error (ENOMEM, map->l_name, NULL,
> +				N_("cannot allocate dependency list"));

OK.

> +	    }
>  	  l_initfini[0] = l;
>  	  memcpy (&l_initfini[1], needed, nneeded * sizeof needed[0]);
>  	  memcpy (&l_initfini[nneeded + 1], l_initfini,
> @@ -457,6 +459,8 @@ _dl_map_object_deps (struct link_map *map,
>      }
>  
>   out:
> +  scratch_buffer_free (&needed_space);

OK. Verified flow in the function can't get around this.

> +
>    if (errno == 0 && errno_saved != 0)
>      __set_errno (errno_saved);
>  
> 

cheers,
Carlos.
  

Patch

diff --git a/elf/dl-deps.c b/elf/dl-deps.c
index 20b8e94f2e..9d9b1ba7f2 100644
--- a/elf/dl-deps.c
+++ b/elf/dl-deps.c
@@ -27,6 +27,7 @@ 
 #include <unistd.h>
 #include <sys/param.h>
 #include <ldsodefs.h>
+#include <scratch_buffer.h>
 
 #include <dl-dst.h>
 
@@ -181,9 +182,8 @@  _dl_map_object_deps (struct link_map *map,
   /* Pointer to last unique object.  */
   tail = &known[nlist - 1];
 
-  /* No alloca'd space yet.  */
-  struct link_map **needed_space = NULL;
-  size_t needed_space_bytes = 0;
+  struct scratch_buffer needed_space;
+  scratch_buffer_init (&needed_space);
 
   /* Process each element of the search list, loading each of its
      auxiliary objects and immediate dependencies.  Auxiliary objects
@@ -213,13 +213,12 @@  _dl_map_object_deps (struct link_map *map,
       if (l->l_searchlist.r_list == NULL && l->l_initfini == NULL
 	  && l != map && l->l_ldnum > 0)
 	{
-	  size_t new_size = l->l_ldnum * sizeof (struct link_map *);
-
-	  if (new_size > needed_space_bytes)
-	    needed_space
-	      = extend_alloca (needed_space, needed_space_bytes, new_size);
-
-	  needed = needed_space;
+	  /* l->l_ldnum includes space for the terminating NULL.  */
+	  if (!scratch_buffer_set_array_size
+	      (&needed_space, l->l_ldnum, sizeof (struct link_map *)))
+	    _dl_signal_error (ENOMEM, map->l_name, NULL,
+			      N_("cannot allocate dependency buffer"));
+	  needed = needed_space.data;
 	}
 
       if (l->l_info[DT_NEEDED] || l->l_info[AUXTAG] || l->l_info[FILTERTAG])
@@ -438,8 +437,11 @@  _dl_map_object_deps (struct link_map *map,
 	  struct link_map **l_initfini = (struct link_map **)
 	    malloc ((2 * nneeded + 1) * sizeof needed[0]);
 	  if (l_initfini == NULL)
-	    _dl_signal_error (ENOMEM, map->l_name, NULL,
-			      N_("cannot allocate dependency list"));
+	    {
+	      scratch_buffer_free (&needed_space);
+	      _dl_signal_error (ENOMEM, map->l_name, NULL,
+				N_("cannot allocate dependency list"));
+	    }
 	  l_initfini[0] = l;
 	  memcpy (&l_initfini[1], needed, nneeded * sizeof needed[0]);
 	  memcpy (&l_initfini[nneeded + 1], l_initfini,
@@ -457,6 +459,8 @@  _dl_map_object_deps (struct link_map *map,
     }
 
  out:
+  scratch_buffer_free (&needed_space);
+
   if (errno == 0 && errno_saved != 0)
     __set_errno (errno_saved);