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

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

[Joe Simmons-Talbott]

Replace alloca usage with a scratch_buffer.
---
 nss/makedb.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/nss/makedb.c b/nss/makedb.c
index 48c8fe1333..c2779d9d9e 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;
@@ -802,6 +812,7 @@ write_output (int fd)
   assert (iov_nelts <= INT_MAX);
   if (writev (fd, iov, iov_nelts) != keydataoffset)
     {
+      scratch_buffer_free (&sbuf);
       error (0, errno, gettext ("failed to write new database file"));
       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] nss: Get rid of alloca usage in makedb's write_output.

[Andreas Schwab]

On Sep 26 2023, Joe Simmons-Talbott wrote:

> @@ -802,6 +812,7 @@ write_output (int fd)
>    assert (iov_nelts <= INT_MAX);
>    if (writev (fd, iov, iov_nelts) != keydataoffset)
>      {
> +      scratch_buffer_free (&sbuf);
>        error (0, errno, gettext ("failed to write new database file"));
>        return EXIT_FAILURE;

Does scratch_buffer_free guarantee that errno is not changed?

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Joe Simmons-Talbott]

On Thu, Sep 28, 2023 at 01:16:00PM +0200, Andreas Schwab wrote:
> On Sep 26 2023, Joe Simmons-Talbott wrote:
> 
> > @@ -802,6 +812,7 @@ write_output (int fd)
> >    assert (iov_nelts <= INT_MAX);
> >    if (writev (fd, iov, iov_nelts) != keydataoffset)
> >      {
> > +      scratch_buffer_free (&sbuf);
> >        error (0, errno, gettext ("failed to write new database file"));
> >        return EXIT_FAILURE;
> 
> Does scratch_buffer_free guarantee that errno is not changed?

scratch_buffer_free doesn't do anything other than call free when the
buffer has been heap-allocated.  IIUC free preserves errno since 2.33 in
the default free.  So I guess if there is a non-default free that
doesn't preserve errno then there is no explicit guarantee.  Should I
adjust scratch_buffer_free to explicitly preserve errno (in a separate
patch) or just preserve errno around this one call to
scratch_buffer_free?

Thanks,
Joe

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

[Andreas Schwab]

On Sep 28 2023, Joe Simmons-Talbott wrote:

> On Thu, Sep 28, 2023 at 01:16:00PM +0200, Andreas Schwab wrote:
>> On Sep 26 2023, Joe Simmons-Talbott wrote:
>> 
>> > @@ -802,6 +812,7 @@ write_output (int fd)
>> >    assert (iov_nelts <= INT_MAX);
>> >    if (writev (fd, iov, iov_nelts) != keydataoffset)
>> >      {
>> > +      scratch_buffer_free (&sbuf);
>> >        error (0, errno, gettext ("failed to write new database file"));
>> >        return EXIT_FAILURE;
>> 
>> Does scratch_buffer_free guarantee that errno is not changed?
>
> scratch_buffer_free doesn't do anything other than call free when the
> buffer has been heap-allocated.  IIUC free preserves errno since 2.33 in
> the default free.  So I guess if there is a non-default free that
> doesn't preserve errno then there is no explicit guarantee.  Should I
> adjust scratch_buffer_free to explicitly preserve errno (in a separate
> patch) or just preserve errno around this one call to
> scratch_buffer_free?

You could just move the call down.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Gabriel Ravier]

On 9/28/23 14:32, Andreas Schwab wrote:
> On Sep 28 2023, Joe Simmons-Talbott wrote:
>
>> On Thu, Sep 28, 2023 at 01:16:00PM +0200, Andreas Schwab wrote:
>>> On Sep 26 2023, Joe Simmons-Talbott wrote:
>>>
>>>> @@ -802,6 +812,7 @@ write_output (int fd)
>>>>     assert (iov_nelts <= INT_MAX);
>>>>     if (writev (fd, iov, iov_nelts) != keydataoffset)
>>>>       {
>>>> +      scratch_buffer_free (&sbuf);
>>>>         error (0, errno, gettext ("failed to write new database file"));
>>>>         return EXIT_FAILURE;
>>> Does scratch_buffer_free guarantee that errno is not changed?
>> scratch_buffer_free doesn't do anything other than call free when the
>> buffer has been heap-allocated.  IIUC free preserves errno since 2.33 in
>> the default free.  So I guess if there is a non-default free that
>> doesn't preserve errno then there is no explicit guarantee.  Should I
>> adjust scratch_buffer_free to explicitly preserve errno (in a separate
>> patch) or just preserve errno around this one call to
>> scratch_buffer_free?
> You could just move the call down.
>
Hmmm, this solves the issue for this patch but it seems like something 
that would merit further discussion in my opinion - personally I think 
the answer should just be to assume a compliant `free` is present, but 
others might have good arguments as to why this shouldn't be the case.