Re: [PATCH] elf: dl-load: Get rid of alloca usage.

[Joe Simmons-Talbott <josimmon@redhat.com>]

On Tue, Oct 17, 2023 at 02:50:41PM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 02/10/23 10:24, Joe Simmons-Talbott wrote:
> > Replace alloca usage with scratch_buffers.  Change the sematics of
> > is_trusted_path_normalize to return 1, 0, or -1 on error.
> > ---
> >  elf/dl-load.c | 72 ++++++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 60 insertions(+), 12 deletions(-)
> > 
> > diff --git a/elf/dl-load.c b/elf/dl-load.c
> > index 2923b1141d..c8e135b6e5 100644
> > --- a/elf/dl-load.c
> > +++ b/elf/dl-load.c
> > @@ -21,6 +21,7 @@
> >  #include <errno.h>
> >  #include <fcntl.h>
> >  #include <libintl.h>
> > +#include <scratch_buffer.h>
> >  #include <stdbool.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > @@ -124,14 +125,21 @@ static const size_t system_dirs_len[] =
> >  };
> >  #define nsystem_dirs_len array_length (system_dirs_len)
> >  
> > -static bool
> > +static int
> 
> The main problem is _dl_dst_substitute, which calls is_trusted_path_normalize,
> is not suppose to fail.  The expand_dynamic_string_token does handle null,
> but fillin_rpath will just ignore the entry.  I think it should 
>  _dl_signal_error (ENOMEM, ...).
> 
> Same for expand_dst macro, which does not expect to _dl_dst_substitute fail
> with memory allocation.
> 
> So I think it would be better to split the patch to first only remove the 
> alloca from is_trusted_path_normalize, add memory allocation checks on
> _dl_dst_substitute to return NULL if is_trusted_path_normalize fails to
> allocate memory, and add NULL checks for _dl_dst_substitute callers as well.

I've posted a v2 which hopefully does as you've suggested.  I also added
the scratch_buffer_free that was overlooked.  Thanks for your review.

Thanks,
Joe