Re: [PATCH v2] nss: Get rid of alloca usage in makedb's write_output.

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

On Wed, Oct 04, 2023 at 03:09:14PM +0200, Arjun Shankar wrote:
> Hi Joe,
> 
> > Replace alloca usage with a scratch_buffer.
> > ---
> > Changes to v1:
> >  * move scratch_buffer_free after the call to error ().
> >
> >  nss/makedb.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/nss/makedb.c b/nss/makedb.c
> > index 48c8fe1333..67c3c4c893 100644
> > --- a/nss/makedb.c
> > +++ b/nss/makedb.c
> > @@ -25,6 +25,7 @@
> >  #include <inttypes.h>
> >  #include <libintl.h>
> >  #include <locale.h>
> > +#include <scratch_buffer.h>
> >  #include <search.h>
> >  #include <stdbool.h>
> >  #include <stdio.h>
> 
> OK.
> 
> > @@ -739,7 +740,16 @@ write_output (int fd)
> >    struct nss_db_header *header;
> >    uint64_t file_offset = (sizeof (struct nss_db_header)
> >                           + (ndatabases * sizeof (header->dbs[0])));
> > -  header = alloca (file_offset);
> > +  struct scratch_buffer sbuf;
> > +  scratch_buffer_init (&sbuf);
> > +
> > +
> > +  if (!scratch_buffer_set_array_size (&sbuf, 1, file_offset))
> > +    {
> > +      error (0, errno, gettext ("failed to allocate memory"));
> > +      return EXIT_FAILURE;
> > +    }
> 
> The way scratch_buffer_init and scratch_buffer_set_array_size are
> implemented currently, a failure of the set_array_size here looks like
> it doesn't necessarily need a scratch_buffer_free to be called after
> failure because there's nothing to be freed. But I think calling
> scratch_buffer_free here before returning here is the safer bet.
> 

I haven't been including a scratch_buffer_free call on failure of
scratch_buffer_set_array_size.  Both failure code paths for
scratch_buffer_set_array_size reset the buffer to the default character
array and any heap allocated storage is freed.  If there is agreement
that a call to scratch_buffer_free is desired I'm happy to add it.

Thanks,
Joe