| Message ID | 20230608155844.976554-1-josimmon@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hurd: writev: Get rid of alloca | expand |
Hello, Joe Simmons-Talbott via Libc-alpha, le jeu. 08 juin 2023 11:58:43 -0400, a ecrit: > Use a scratch_buffer rather than alloca to avoid potential stack > overflows. > > Checked on i686-gnu and x86_64-linux-gnu Applied after fixing spaces and comments, thanks! Samuel > --- > sysdeps/posix/writev.c | 35 ++++++++++++----------------------- > 1 file changed, 12 insertions(+), 23 deletions(-) > > diff --git a/sysdeps/posix/writev.c b/sysdeps/posix/writev.c > index 53e090c087..0cee0aa692 100644 > --- a/sysdeps/posix/writev.c > +++ b/sysdeps/posix/writev.c > @@ -19,19 +19,13 @@ > #include <unistd.h> > #include <string.h> > #include <limits.h> > +#include <scratch_buffer.h> > #include <stdbool.h> > #include <sys/param.h> > #include <sys/uio.h> > #include <errno.h> > > > -static void > -ifree (char **ptrp) > -{ > - free (*ptrp); > -} > - > - > /* Write data pointed by the buffers described by VECTOR, which > is a vector of COUNT 'struct iovec's, to file descriptor FD. > The data is written in the order specified. > @@ -53,22 +47,15 @@ __writev (int fd, const struct iovec *vector, int count) > bytes += vector[i].iov_len; > } > > - /* Allocate a temporary buffer to hold the data. We should normally > - use alloca since it's faster and does not require synchronization > - with other threads. But we cannot if the amount of memory > - required is too large. */ > - char *buffer; > - char *malloced_buffer __attribute__ ((__cleanup__ (ifree))) = NULL; > - if (__libc_use_alloca (bytes)) > - buffer = (char *) __alloca (bytes); > - else > - { > - malloced_buffer = buffer = (char *) malloc (bytes); > - if (buffer == NULL) > - /* XXX I don't know whether it is acceptable to try writing > - the data in chunks. Probably not so we just fail here. */ > - return -1; > - } > + /* Allocate a temporary buffer to hold the data. Use a scratch_buffer > + since it's faster for small buffer sizes but can handle larger > + allocations as well. */ > + > + struct scratch_buffer buf; > + scratch_buffer_init (&buf); > + if (!scratch_buffer_set_array_size (&buf, 1, bytes)) > + return -1; > + char *buffer = buf.data; > > /* Copy the data into BUFFER. */ > size_t to_copy = bytes; > @@ -86,6 +73,8 @@ __writev (int fd, const struct iovec *vector, int count) > > ssize_t bytes_written = __write (fd, buffer, bytes); > > + scratch_buffer_free (&buf); > + > return bytes_written; > } > libc_hidden_def (__writev) > -- > 2.39.2 >
On 08/06/23 12:58, Joe Simmons-Talbott via Libc-alpha wrote: > Use a scratch_buffer rather than alloca to avoid potential stack > overflows. > > Checked on i686-gnu and x86_64-linux-gnu > --- > sysdeps/posix/writev.c | 35 ++++++++++++----------------------- > 1 file changed, 12 insertions(+), 23 deletions(-) > > diff --git a/sysdeps/posix/writev.c b/sysdeps/posix/writev.c > index 53e090c087..0cee0aa692 100644 > --- a/sysdeps/posix/writev.c > +++ b/sysdeps/posix/writev.c > @@ -19,19 +19,13 @@ > #include <unistd.h> > #include <string.h> > #include <limits.h> > +#include <scratch_buffer.h> > #include <stdbool.h> > #include <sys/param.h> > #include <sys/uio.h> > #include <errno.h> > > > -static void > -ifree (char **ptrp) > -{ > - free (*ptrp); > -} > - > - > /* Write data pointed by the buffers described by VECTOR, which > is a vector of COUNT 'struct iovec's, to file descriptor FD. > The data is written in the order specified. > @@ -53,22 +47,15 @@ __writev (int fd, const struct iovec *vector, int count) > bytes += vector[i].iov_len; > } > > - /* Allocate a temporary buffer to hold the data. We should normally > - use alloca since it's faster and does not require synchronization > - with other threads. But we cannot if the amount of memory > - required is too large. */ > - char *buffer; > - char *malloced_buffer __attribute__ ((__cleanup__ (ifree))) = NULL; > - if (__libc_use_alloca (bytes)) > - buffer = (char *) __alloca (bytes); > - else > - { > - malloced_buffer = buffer = (char *) malloc (bytes); > - if (buffer == NULL) > - /* XXX I don't know whether it is acceptable to try writing > - the data in chunks. Probably not so we just fail here. */ > - return -1; > - } I am not sure if this is fully correct, since writev is a 'shall occurs' cancellation entrypoint and cancelling a large writev operation now will leak memory. So I think we should either continue to keep the cleanup handler or define IOV_MAX on Hurd and use it do define a static buffer. > + /* Allocate a temporary buffer to hold the data. Use a scratch_buffer > + since it's faster for small buffer sizes but can handle larger > + allocations as well. */ > + > + struct scratch_buffer buf; > + scratch_buffer_init (&buf); > + if (!scratch_buffer_set_array_size (&buf, 1, bytes)) > + return -1; > + char *buffer = buf.data; > > /* Copy the data into BUFFER. */ > size_t to_copy = bytes; > @@ -86,6 +73,8 @@ __writev (int fd, const struct iovec *vector, int count) > > ssize_t bytes_written = __write (fd, buffer, bytes); > > + scratch_buffer_free (&buf); > + > return bytes_written; > } > libc_hidden_def (__writev)
On Mon, Jun 19, 2023 at 09:24:34AM -0300, Adhemerval Zanella Netto wrote: > > > On 08/06/23 12:58, Joe Simmons-Talbott via Libc-alpha wrote: > > Use a scratch_buffer rather than alloca to avoid potential stack > > overflows. > > > > Checked on i686-gnu and x86_64-linux-gnu > > --- > > sysdeps/posix/writev.c | 35 ++++++++++++----------------------- > > 1 file changed, 12 insertions(+), 23 deletions(-) > > > > diff --git a/sysdeps/posix/writev.c b/sysdeps/posix/writev.c > > index 53e090c087..0cee0aa692 100644 > > --- a/sysdeps/posix/writev.c > > +++ b/sysdeps/posix/writev.c > > @@ -19,19 +19,13 @@ > > #include <unistd.h> > > #include <string.h> > > #include <limits.h> > > +#include <scratch_buffer.h> > > #include <stdbool.h> > > #include <sys/param.h> > > #include <sys/uio.h> > > #include <errno.h> > > > > > > -static void > > -ifree (char **ptrp) > > -{ > > - free (*ptrp); > > -} > > - > > - > > /* Write data pointed by the buffers described by VECTOR, which > > is a vector of COUNT 'struct iovec's, to file descriptor FD. > > The data is written in the order specified. > > @@ -53,22 +47,15 @@ __writev (int fd, const struct iovec *vector, int count) > > bytes += vector[i].iov_len; > > } > > > > - /* Allocate a temporary buffer to hold the data. We should normally > > - use alloca since it's faster and does not require synchronization > > - with other threads. But we cannot if the amount of memory > > - required is too large. */ > > - char *buffer; > > - char *malloced_buffer __attribute__ ((__cleanup__ (ifree))) = NULL; > > - if (__libc_use_alloca (bytes)) > > - buffer = (char *) __alloca (bytes); > > - else > > - { > > - malloced_buffer = buffer = (char *) malloc (bytes); > > - if (buffer == NULL) > > - /* XXX I don't know whether it is acceptable to try writing > > - the data in chunks. Probably not so we just fail here. */ > > - return -1; > > - } > > I am not sure if this is fully correct, since writev is a 'shall occurs' > cancellation entrypoint and cancelling a large writev operation now will > leak memory. So I think we should either continue to keep the cleanup > handler or define IOV_MAX on Hurd and use it do define a static buffer. Thanks for catching that. I'll propose a patch to fix it shortly. Thanks, Joe
diff --git a/sysdeps/posix/writev.c b/sysdeps/posix/writev.c index 53e090c087..0cee0aa692 100644 --- a/sysdeps/posix/writev.c +++ b/sysdeps/posix/writev.c @@ -19,19 +19,13 @@ #include <unistd.h> #include <string.h> #include <limits.h> +#include <scratch_buffer.h> #include <stdbool.h> #include <sys/param.h> #include <sys/uio.h> #include <errno.h> -static void -ifree (char **ptrp) -{ - free (*ptrp); -} - - /* Write data pointed by the buffers described by VECTOR, which is a vector of COUNT 'struct iovec's, to file descriptor FD. The data is written in the order specified. @@ -53,22 +47,15 @@ __writev (int fd, const struct iovec *vector, int count) bytes += vector[i].iov_len; } - /* Allocate a temporary buffer to hold the data. We should normally - use alloca since it's faster and does not require synchronization - with other threads. But we cannot if the amount of memory - required is too large. */ - char *buffer; - char *malloced_buffer __attribute__ ((__cleanup__ (ifree))) = NULL; - if (__libc_use_alloca (bytes)) - buffer = (char *) __alloca (bytes); - else - { - malloced_buffer = buffer = (char *) malloc (bytes); - if (buffer == NULL) - /* XXX I don't know whether it is acceptable to try writing - the data in chunks. Probably not so we just fail here. */ - return -1; - } + /* Allocate a temporary buffer to hold the data. Use a scratch_buffer + since it's faster for small buffer sizes but can handle larger + allocations as well. */ + + struct scratch_buffer buf; + scratch_buffer_init (&buf); + if (!scratch_buffer_set_array_size (&buf, 1, bytes)) + return -1; + char *buffer = buf.data; /* Copy the data into BUFFER. */ size_t to_copy = bytes; @@ -86,6 +73,8 @@ __writev (int fd, const struct iovec *vector, int count) ssize_t bytes_written = __write (fd, buffer, bytes); + scratch_buffer_free (&buf); + return bytes_written; } libc_hidden_def (__writev)