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

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

[Joe Simmons-Talbott]

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>
@@ -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;
+    }
+  header = sbuf.data;
 
   header->magic = NSS_DB_MAGIC;
   header->ndbs = ndatabases;
@@ -803,6 +813,7 @@ write_output (int fd)
   if (writev (fd, iov, iov_nelts) != keydataoffset)
     {
       error (0, errno, gettext ("failed to write new database file"));
+      scratch_buffer_free (&sbuf);
       return EXIT_FAILURE;
     }
 
@@ -810,6 +821,7 @@ write_output (int fd)
   DIAG_POP_NEEDS_COMMENT;
 #endif
 
+  scratch_buffer_free (&sbuf);
   return EXIT_SUCCESS;
 }
 
-- 
2.39.2

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

[Arjun Shankar]

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.

> +  header = sbuf.data;
>
>    header->magic = NSS_DB_MAGIC;
>    header->ndbs = ndatabases;
> @@ -803,6 +813,7 @@ write_output (int fd)
>    if (writev (fd, iov, iov_nelts) != keydataoffset)
>      {
>        error (0, errno, gettext ("failed to write new database file"));
> +      scratch_buffer_free (&sbuf);
>        return EXIT_FAILURE;
>      }

OK.

>
> @@ -810,6 +821,7 @@ write_output (int fd)
>    DIAG_POP_NEEDS_COMMENT;
>  #endif
>
> +  scratch_buffer_free (&sbuf);
>    return EXIT_SUCCESS;
>  }

OK.

-- 
Arjun Shankar
he/him/his

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

[Joe Simmons-Talbott]

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

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

[Arjun Shankar]

> > 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.

You're right. There is some code that does a free (e.g. an instance in
catgets/gencat.c, and one in nscd/nscd_getgr_r.c via a goto), and some
that does not (e.g. stdio-common/fxprintf.c and
sysdeps/posix/getaddrinfo.c -- which as you said, you've committed
recently).

Reviewed-by: Arjun Shankar <arjun@redhat.com>

Cheers!

-- 
Arjun Shankar
he/him/his