[v2] nss: Get rid of alloca usage in makedb's write_output.
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
Commit Message
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(-)
Comments
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!
@@ -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;
}