Message ID | 20230928141125.600816-1-josimmon@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v2] nss: Get rid of alloca usage in makedb's write_output. | expand |
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.
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
> > 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!
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; }