* [PATCH] elf: dl-load: Get rid of alloca usage.
@ 2023-10-02 13:24 Joe Simmons-Talbott
2023-10-10 19:07 ` Joe Simmons-Talbott
2023-10-17 17:50 ` Adhemerval Zanella Netto
0 siblings, 2 replies; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-10-02 13:24 UTC (permalink / raw)
To: libc-alpha; +Cc: Joe Simmons-Talbott
Replace alloca usage with scratch_buffers. Change the sematics of
is_trusted_path_normalize to return 1, 0, or -1 on error.
---
elf/dl-load.c | 72 ++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 60 insertions(+), 12 deletions(-)
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 2923b1141d..c8e135b6e5 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -21,6 +21,7 @@
#include <errno.h>
#include <fcntl.h>
#include <libintl.h>
+#include <scratch_buffer.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
@@ -124,14 +125,21 @@ static const size_t system_dirs_len[] =
};
#define nsystem_dirs_len array_length (system_dirs_len)
-static bool
+static int
is_trusted_path_normalize (const char *path, size_t len)
{
if (len == 0)
- return false;
+ return 0;
+
+ struct scratch_buffer sbuf;
+ scratch_buffer_init (&sbuf);
+
+ if (!scratch_buffer_set_array_size (&sbuf, 1, len + 2))
+ return -1;
- char *npath = (char *) alloca (len + 2);
+ char *npath = sbuf.data;
char *wnp = npath;
+
while (*path != '\0')
{
if (path[0] == '/')
@@ -172,12 +180,12 @@ is_trusted_path_normalize (const char *path, size_t len)
if (wnp - npath >= system_dirs_len[idx]
&& memcmp (trun, npath, system_dirs_len[idx]) == 0)
/* Found it. */
- return true;
+ return 1;
trun += system_dirs_len[idx] + 1;
}
- return false;
+ return 0;
}
/* Given a substring starting at INPUT, just after the DST '$' start
@@ -363,7 +371,7 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
this way because it may be manipulated in some ways with hard
links. */
if (__glibc_unlikely (check_for_trusted)
- && !is_trusted_path_normalize (result, wp - result))
+ && is_trusted_path_normalize (result, wp - result) != 1)
{
*result = '\0';
return result;
@@ -951,6 +959,8 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
/* Initialize to keep the compiler happy. */
const char *errstring = NULL;
int errval = 0;
+ struct scratch_buffer sbuf;
+ scratch_buffer_init (&sbuf);
/* Get file information. To match the kernel behavior, do not fill
in this information for the executable in case of an explicit
@@ -982,6 +992,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
free ((void *) l->l_phdr);
free (l);
free (realname);
+ scratch_buffer_free (&sbuf);
_dl_signal_error (errval, name, NULL, errstring);
}
@@ -998,6 +1009,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
free (realname);
add_name_to_object (l, name);
+ scratch_buffer_free (&sbuf);
return l;
}
}
@@ -1029,6 +1041,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
/* Add the map for the mirrored object to the object list. */
_dl_add_to_namespace_list (l, nsid);
+ scratch_buffer_free (&sbuf);
return l;
}
#endif
@@ -1039,6 +1052,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
loaded. So return now. */
free (realname);
__close_nocancel (fd);
+ scratch_buffer_free (&sbuf);
return NULL;
}
@@ -1071,7 +1085,12 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
phdr = (void *) (fbp->buf + header->e_phoff);
else
{
- phdr = alloca (maplength);
+ if (!scratch_buffer_set_array_size (&sbuf, 1, maplength))
+ {
+ errstring = N_("cannot allocate memory");
+ goto lose_errno;
+ }
+ phdr = sbuf.data;
if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength,
header->e_phoff) != maplength)
{
@@ -1485,7 +1504,10 @@ cannot enable executable stack as shared object requires");
/* Skip auditing and debugger notification when called from 'sprof'. */
if (mode & __RTLD_SPROF)
- return l;
+ {
+ scratch_buffer_free (&sbuf);
+ return l;
+ }
/* Signal that we are going to add new objects. */
struct r_debug *r = _dl_debug_update (nsid);
@@ -1515,6 +1537,7 @@ cannot enable executable stack as shared object requires");
_dl_audit_objopen (l, nsid);
#endif
+ scratch_buffer_free (&sbuf);
return l;
}
\f
@@ -1598,6 +1621,8 @@ open_verify (const char *name, int fd,
/* Initialize it to make the compiler happy. */
const char *errstring = NULL;
int errval = 0;
+ struct scratch_buffer sbuf;
+ scratch_buffer_init (&sbuf);
#ifdef SHARED
/* Give the auditing libraries a chance. */
@@ -1660,6 +1685,7 @@ open_verify (const char *name, int fd,
name = strdupa (realname);
free (realname);
}
+ scratch_buffer_free (&sbuf);
__close_nocancel (fd);
_dl_signal_error (errval, name, NULL, errstring);
}
@@ -1696,6 +1722,7 @@ open_verify (const char *name, int fd,
32-bit and 64-bit binaries can be run this might
happen. */
*found_other_class = true;
+ scratch_buffer_free (&sbuf);
__close_nocancel (fd);
__set_errno (ENOENT);
return -1;
@@ -1734,6 +1761,7 @@ open_verify (const char *name, int fd,
}
if (! __glibc_likely (elf_machine_matches_host (ehdr)))
{
+ scratch_buffer_free (&sbuf);
__close_nocancel (fd);
__set_errno (ENOENT);
return -1;
@@ -1755,7 +1783,14 @@ open_verify (const char *name, int fd,
phdr = (void *) (fbp->buf + ehdr->e_phoff);
else
{
- phdr = alloca (maplength);
+ if (!scratch_buffer_set_array_size (&sbuf, 1, maplength))
+ {
+ errval = errno;
+ errstring = N_("cannot allocate memory");
+ goto lose;
+ }
+ phdr = sbuf.data;
+
if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength,
ehdr->e_phoff) != maplength)
{
@@ -1769,6 +1804,7 @@ open_verify (const char *name, int fd,
(phdr, ehdr->e_phnum, fbp->buf, fbp->len,
loader, fd)))
{
+ scratch_buffer_free (&sbuf);
__close_nocancel (fd);
__set_errno (ENOENT);
return -1;
@@ -1776,6 +1812,7 @@ open_verify (const char *name, int fd,
}
+ scratch_buffer_free (&sbuf);
return fd;
}
\f
@@ -1796,13 +1833,18 @@ open_path (const char *name, size_t namelen, int mode,
int fd = -1;
const char *current_what = NULL;
int any = 0;
+ struct scratch_buffer sbuf;
+ scratch_buffer_init (&sbuf);
if (__glibc_unlikely (dirs == NULL))
/* We're called before _dl_init_paths when loading the main executable
given on the command line when rtld is run directly. */
return -1;
- buf = alloca (max_dirnamelen + max_capstrlen + namelen);
+ if (!scratch_buffer_set_array_size (&sbuf, 1,
+ max_dirnamelen + max_capstrlen + namelen))
+ return -1;
+ buf = sbuf.data;
do
{
struct r_search_path_elem *this_dir = *dirs;
@@ -1901,6 +1943,7 @@ open_path (const char *name, size_t namelen, int mode,
if (*realname != NULL)
{
memcpy (*realname, buf, buflen);
+ scratch_buffer_free (&sbuf);
return fd;
}
else
@@ -1908,12 +1951,16 @@ open_path (const char *name, size_t namelen, int mode,
/* No memory for the name, we certainly won't be able
to load and link it. */
__close_nocancel (fd);
+ scratch_buffer_free (&sbuf);
return -1;
}
}
if (here_any && (err = errno) != ENOENT && err != EACCES)
- /* The file exists and is readable, but something went wrong. */
- return -1;
+ {
+ /* The file exists and is readable, but something went wrong. */
+ scratch_buffer_free (&sbuf);
+ return -1;
+ }
/* Remember whether we found anything. */
any |= here_any;
@@ -1934,6 +1981,7 @@ open_path (const char *name, size_t namelen, int mode,
sps->dirs = (void *) -1;
}
+ scratch_buffer_free (&sbuf);
return -1;
}
--
2.39.2
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] elf: dl-load: Get rid of alloca usage.
2023-10-02 13:24 [PATCH] elf: dl-load: Get rid of alloca usage Joe Simmons-Talbott
@ 2023-10-10 19:07 ` Joe Simmons-Talbott
2023-10-17 17:50 ` Adhemerval Zanella Netto
1 sibling, 0 replies; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-10-10 19:07 UTC (permalink / raw)
To: libc-alpha
Ping.
On Mon, Oct 02, 2023 at 09:24:01AM -0400, Joe Simmons-Talbott wrote:
> Replace alloca usage with scratch_buffers. Change the sematics of
> is_trusted_path_normalize to return 1, 0, or -1 on error.
> ---
> elf/dl-load.c | 72 ++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 60 insertions(+), 12 deletions(-)
>
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 2923b1141d..c8e135b6e5 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -21,6 +21,7 @@
> #include <errno.h>
> #include <fcntl.h>
> #include <libintl.h>
> +#include <scratch_buffer.h>
> #include <stdbool.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -124,14 +125,21 @@ static const size_t system_dirs_len[] =
> };
> #define nsystem_dirs_len array_length (system_dirs_len)
>
> -static bool
> +static int
> is_trusted_path_normalize (const char *path, size_t len)
> {
> if (len == 0)
> - return false;
> + return 0;
> +
> + struct scratch_buffer sbuf;
> + scratch_buffer_init (&sbuf);
> +
> + if (!scratch_buffer_set_array_size (&sbuf, 1, len + 2))
> + return -1;
>
> - char *npath = (char *) alloca (len + 2);
> + char *npath = sbuf.data;
> char *wnp = npath;
> +
> while (*path != '\0')
> {
> if (path[0] == '/')
> @@ -172,12 +180,12 @@ is_trusted_path_normalize (const char *path, size_t len)
> if (wnp - npath >= system_dirs_len[idx]
> && memcmp (trun, npath, system_dirs_len[idx]) == 0)
> /* Found it. */
> - return true;
> + return 1;
>
> trun += system_dirs_len[idx] + 1;
> }
>
> - return false;
> + return 0;
> }
>
> /* Given a substring starting at INPUT, just after the DST '$' start
> @@ -363,7 +371,7 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
> this way because it may be manipulated in some ways with hard
> links. */
> if (__glibc_unlikely (check_for_trusted)
> - && !is_trusted_path_normalize (result, wp - result))
> + && is_trusted_path_normalize (result, wp - result) != 1)
> {
> *result = '\0';
> return result;
> @@ -951,6 +959,8 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> /* Initialize to keep the compiler happy. */
> const char *errstring = NULL;
> int errval = 0;
> + struct scratch_buffer sbuf;
> + scratch_buffer_init (&sbuf);
>
> /* Get file information. To match the kernel behavior, do not fill
> in this information for the executable in case of an explicit
> @@ -982,6 +992,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> free ((void *) l->l_phdr);
> free (l);
> free (realname);
> + scratch_buffer_free (&sbuf);
> _dl_signal_error (errval, name, NULL, errstring);
> }
>
> @@ -998,6 +1009,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> free (realname);
> add_name_to_object (l, name);
>
> + scratch_buffer_free (&sbuf);
> return l;
> }
> }
> @@ -1029,6 +1041,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> /* Add the map for the mirrored object to the object list. */
> _dl_add_to_namespace_list (l, nsid);
>
> + scratch_buffer_free (&sbuf);
> return l;
> }
> #endif
> @@ -1039,6 +1052,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> loaded. So return now. */
> free (realname);
> __close_nocancel (fd);
> + scratch_buffer_free (&sbuf);
> return NULL;
> }
>
> @@ -1071,7 +1085,12 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> phdr = (void *) (fbp->buf + header->e_phoff);
> else
> {
> - phdr = alloca (maplength);
> + if (!scratch_buffer_set_array_size (&sbuf, 1, maplength))
> + {
> + errstring = N_("cannot allocate memory");
> + goto lose_errno;
> + }
> + phdr = sbuf.data;
> if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength,
> header->e_phoff) != maplength)
> {
> @@ -1485,7 +1504,10 @@ cannot enable executable stack as shared object requires");
>
> /* Skip auditing and debugger notification when called from 'sprof'. */
> if (mode & __RTLD_SPROF)
> - return l;
> + {
> + scratch_buffer_free (&sbuf);
> + return l;
> + }
>
> /* Signal that we are going to add new objects. */
> struct r_debug *r = _dl_debug_update (nsid);
> @@ -1515,6 +1537,7 @@ cannot enable executable stack as shared object requires");
> _dl_audit_objopen (l, nsid);
> #endif
>
> + scratch_buffer_free (&sbuf);
> return l;
> }
> \f
> @@ -1598,6 +1621,8 @@ open_verify (const char *name, int fd,
> /* Initialize it to make the compiler happy. */
> const char *errstring = NULL;
> int errval = 0;
> + struct scratch_buffer sbuf;
> + scratch_buffer_init (&sbuf);
>
> #ifdef SHARED
> /* Give the auditing libraries a chance. */
> @@ -1660,6 +1685,7 @@ open_verify (const char *name, int fd,
> name = strdupa (realname);
> free (realname);
> }
> + scratch_buffer_free (&sbuf);
> __close_nocancel (fd);
> _dl_signal_error (errval, name, NULL, errstring);
> }
> @@ -1696,6 +1722,7 @@ open_verify (const char *name, int fd,
> 32-bit and 64-bit binaries can be run this might
> happen. */
> *found_other_class = true;
> + scratch_buffer_free (&sbuf);
> __close_nocancel (fd);
> __set_errno (ENOENT);
> return -1;
> @@ -1734,6 +1761,7 @@ open_verify (const char *name, int fd,
> }
> if (! __glibc_likely (elf_machine_matches_host (ehdr)))
> {
> + scratch_buffer_free (&sbuf);
> __close_nocancel (fd);
> __set_errno (ENOENT);
> return -1;
> @@ -1755,7 +1783,14 @@ open_verify (const char *name, int fd,
> phdr = (void *) (fbp->buf + ehdr->e_phoff);
> else
> {
> - phdr = alloca (maplength);
> + if (!scratch_buffer_set_array_size (&sbuf, 1, maplength))
> + {
> + errval = errno;
> + errstring = N_("cannot allocate memory");
> + goto lose;
> + }
> + phdr = sbuf.data;
> +
> if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength,
> ehdr->e_phoff) != maplength)
> {
> @@ -1769,6 +1804,7 @@ open_verify (const char *name, int fd,
> (phdr, ehdr->e_phnum, fbp->buf, fbp->len,
> loader, fd)))
> {
> + scratch_buffer_free (&sbuf);
> __close_nocancel (fd);
> __set_errno (ENOENT);
> return -1;
> @@ -1776,6 +1812,7 @@ open_verify (const char *name, int fd,
>
> }
>
> + scratch_buffer_free (&sbuf);
> return fd;
> }
> \f
> @@ -1796,13 +1833,18 @@ open_path (const char *name, size_t namelen, int mode,
> int fd = -1;
> const char *current_what = NULL;
> int any = 0;
> + struct scratch_buffer sbuf;
> + scratch_buffer_init (&sbuf);
>
> if (__glibc_unlikely (dirs == NULL))
> /* We're called before _dl_init_paths when loading the main executable
> given on the command line when rtld is run directly. */
> return -1;
>
> - buf = alloca (max_dirnamelen + max_capstrlen + namelen);
> + if (!scratch_buffer_set_array_size (&sbuf, 1,
> + max_dirnamelen + max_capstrlen + namelen))
> + return -1;
> + buf = sbuf.data;
> do
> {
> struct r_search_path_elem *this_dir = *dirs;
> @@ -1901,6 +1943,7 @@ open_path (const char *name, size_t namelen, int mode,
> if (*realname != NULL)
> {
> memcpy (*realname, buf, buflen);
> + scratch_buffer_free (&sbuf);
> return fd;
> }
> else
> @@ -1908,12 +1951,16 @@ open_path (const char *name, size_t namelen, int mode,
> /* No memory for the name, we certainly won't be able
> to load and link it. */
> __close_nocancel (fd);
> + scratch_buffer_free (&sbuf);
> return -1;
> }
> }
> if (here_any && (err = errno) != ENOENT && err != EACCES)
> - /* The file exists and is readable, but something went wrong. */
> - return -1;
> + {
> + /* The file exists and is readable, but something went wrong. */
> + scratch_buffer_free (&sbuf);
> + return -1;
> + }
>
> /* Remember whether we found anything. */
> any |= here_any;
> @@ -1934,6 +1981,7 @@ open_path (const char *name, size_t namelen, int mode,
> sps->dirs = (void *) -1;
> }
>
> + scratch_buffer_free (&sbuf);
> return -1;
> }
>
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] elf: dl-load: Get rid of alloca usage.
2023-10-02 13:24 [PATCH] elf: dl-load: Get rid of alloca usage Joe Simmons-Talbott
2023-10-10 19:07 ` Joe Simmons-Talbott
@ 2023-10-17 17:50 ` Adhemerval Zanella Netto
2023-10-18 13:31 ` Joe Simmons-Talbott
1 sibling, 1 reply; 4+ messages in thread
From: Adhemerval Zanella Netto @ 2023-10-17 17:50 UTC (permalink / raw)
To: libc-alpha, Joe Simmons-Talbott
On 02/10/23 10:24, Joe Simmons-Talbott wrote:
> Replace alloca usage with scratch_buffers. Change the sematics of
> is_trusted_path_normalize to return 1, 0, or -1 on error.
> ---
> elf/dl-load.c | 72 ++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 60 insertions(+), 12 deletions(-)
>
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 2923b1141d..c8e135b6e5 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -21,6 +21,7 @@
> #include <errno.h>
> #include <fcntl.h>
> #include <libintl.h>
> +#include <scratch_buffer.h>
> #include <stdbool.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -124,14 +125,21 @@ static const size_t system_dirs_len[] =
> };
> #define nsystem_dirs_len array_length (system_dirs_len)
>
> -static bool
> +static int
The main problem is _dl_dst_substitute, which calls is_trusted_path_normalize,
is not suppose to fail. The expand_dynamic_string_token does handle null,
but fillin_rpath will just ignore the entry. I think it should
_dl_signal_error (ENOMEM, ...).
Same for expand_dst macro, which does not expect to _dl_dst_substitute fail
with memory allocation.
So I think it would be better to split the patch to first only remove the
alloca from is_trusted_path_normalize, add memory allocation checks on
_dl_dst_substitute to return NULL if is_trusted_path_normalize fails to
allocate memory, and add NULL checks for _dl_dst_substitute callers as well.
> is_trusted_path_normalize (const char *path, size_t len)
> {
> if (len == 0)
> - return false;
> + return 0;
> +
> + struct scratch_buffer sbuf;
> + scratch_buffer_init (&sbuf);
> +
> + if (!scratch_buffer_set_array_size (&sbuf, 1, len + 2))
> + return -1;
>
> - char *npath = (char *) alloca (len + 2);
> + char *npath = sbuf.data;
> char *wnp = npath;
> +
> while (*path != '\0')
> {
> if (path[0] == '/')
> @@ -172,12 +180,12 @@ is_trusted_path_normalize (const char *path, size_t len)
> if (wnp - npath >= system_dirs_len[idx]
> && memcmp (trun, npath, system_dirs_len[idx]) == 0)
> /* Found it. */
> - return true;
> + return 1;
>
> trun += system_dirs_len[idx] + 1;
> }
>
> - return false;
> + return 0;
It misses the scratch_buffer_free for sbuf.
> }
>
> /* Given a substring starting at INPUT, just after the DST '$' start
> @@ -363,7 +371,7 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
> this way because it may be manipulated in some ways with hard
> links. */
> if (__glibc_unlikely (check_for_trusted)
> - && !is_trusted_path_normalize (result, wp - result))
> + && is_trusted_path_normalize (result, wp - result) != 1)
> {
> *result = '\0';
> return result;
> @@ -951,6 +959,8 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> /* Initialize to keep the compiler happy. */
> const char *errstring = NULL;
> int errval = 0;
> + struct scratch_buffer sbuf;
> + scratch_buffer_init (&sbuf);
>
> /* Get file information. To match the kernel behavior, do not fill
> in this information for the executable in case of an explicit
> @@ -982,6 +992,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> free ((void *) l->l_phdr);
> free (l);
> free (realname);
> + scratch_buffer_free (&sbuf);
> _dl_signal_error (errval, name, NULL, errstring);
> }
>
> @@ -998,6 +1009,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> free (realname);
> add_name_to_object (l, name);
>
> + scratch_buffer_free (&sbuf);
> return l;
> }
> }
> @@ -1029,6 +1041,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> /* Add the map for the mirrored object to the object list. */
> _dl_add_to_namespace_list (l, nsid);
>
> + scratch_buffer_free (&sbuf);
> return l;
> }
> #endif
> @@ -1039,6 +1052,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> loaded. So return now. */
> free (realname);
> __close_nocancel (fd);
> + scratch_buffer_free (&sbuf);
> return NULL;
> }
>
> @@ -1071,7 +1085,12 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> phdr = (void *) (fbp->buf + header->e_phoff);
> else
> {
> - phdr = alloca (maplength);
> + if (!scratch_buffer_set_array_size (&sbuf, 1, maplength))
> + {
> + errstring = N_("cannot allocate memory");
> + goto lose_errno;
> + }
> + phdr = sbuf.data;
> if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength,
> header->e_phoff) != maplength)
> {
> @@ -1485,7 +1504,10 @@ cannot enable executable stack as shared object requires");
>
> /* Skip auditing and debugger notification when called from 'sprof'. */
> if (mode & __RTLD_SPROF)
> - return l;
> + {
> + scratch_buffer_free (&sbuf);
> + return l;
> + }
>
> /* Signal that we are going to add new objects. */
> struct r_debug *r = _dl_debug_update (nsid);
> @@ -1515,6 +1537,7 @@ cannot enable executable stack as shared object requires");
> _dl_audit_objopen (l, nsid);
> #endif
>
> + scratch_buffer_free (&sbuf);
> return l;
> }
> \f
> @@ -1598,6 +1621,8 @@ open_verify (const char *name, int fd,
> /* Initialize it to make the compiler happy. */
> const char *errstring = NULL;
> int errval = 0;
> + struct scratch_buffer sbuf;
> + scratch_buffer_init (&sbuf);
>
> #ifdef SHARED
> /* Give the auditing libraries a chance. */
> @@ -1660,6 +1685,7 @@ open_verify (const char *name, int fd,
> name = strdupa (realname);
> free (realname);
> }
> + scratch_buffer_free (&sbuf);
> __close_nocancel (fd);
> _dl_signal_error (errval, name, NULL, errstring);
> }
> @@ -1696,6 +1722,7 @@ open_verify (const char *name, int fd,
> 32-bit and 64-bit binaries can be run this might
> happen. */
> *found_other_class = true;
> + scratch_buffer_free (&sbuf);
> __close_nocancel (fd);
> __set_errno (ENOENT);
> return -1;
> @@ -1734,6 +1761,7 @@ open_verify (const char *name, int fd,
> }
> if (! __glibc_likely (elf_machine_matches_host (ehdr)))
> {
> + scratch_buffer_free (&sbuf);
> __close_nocancel (fd);
> __set_errno (ENOENT);
> return -1;
> @@ -1755,7 +1783,14 @@ open_verify (const char *name, int fd,
> phdr = (void *) (fbp->buf + ehdr->e_phoff);
> else
> {
> - phdr = alloca (maplength);
> + if (!scratch_buffer_set_array_size (&sbuf, 1, maplength))
> + {
> + errval = errno;
> + errstring = N_("cannot allocate memory");
> + goto lose;
> + }
> + phdr = sbuf.data;
> +
> if ((size_t) __pread64_nocancel (fd, (void *) phdr, maplength,
> ehdr->e_phoff) != maplength)
> {
> @@ -1769,6 +1804,7 @@ open_verify (const char *name, int fd,
> (phdr, ehdr->e_phnum, fbp->buf, fbp->len,
> loader, fd)))
> {
> + scratch_buffer_free (&sbuf);
> __close_nocancel (fd);
> __set_errno (ENOENT);
> return -1;
> @@ -1776,6 +1812,7 @@ open_verify (const char *name, int fd,
>
> }
>
> + scratch_buffer_free (&sbuf);
> return fd;
> }
> \f
> @@ -1796,13 +1833,18 @@ open_path (const char *name, size_t namelen, int mode,
> int fd = -1;
> const char *current_what = NULL;
> int any = 0;
> + struct scratch_buffer sbuf;
> + scratch_buffer_init (&sbuf);
>
> if (__glibc_unlikely (dirs == NULL))
> /* We're called before _dl_init_paths when loading the main executable
> given on the command line when rtld is run directly. */
> return -1;
>
> - buf = alloca (max_dirnamelen + max_capstrlen + namelen);
> + if (!scratch_buffer_set_array_size (&sbuf, 1,
> + max_dirnamelen + max_capstrlen + namelen))
> + return -1;
> + buf = sbuf.data;
> do
> {
> struct r_search_path_elem *this_dir = *dirs;
> @@ -1901,6 +1943,7 @@ open_path (const char *name, size_t namelen, int mode,
> if (*realname != NULL)
> {
> memcpy (*realname, buf, buflen);
> + scratch_buffer_free (&sbuf);
> return fd;
> }
> else
> @@ -1908,12 +1951,16 @@ open_path (const char *name, size_t namelen, int mode,
> /* No memory for the name, we certainly won't be able
> to load and link it. */
> __close_nocancel (fd);
> + scratch_buffer_free (&sbuf);
> return -1;
> }
> }
> if (here_any && (err = errno) != ENOENT && err != EACCES)
> - /* The file exists and is readable, but something went wrong. */
> - return -1;
> + {
> + /* The file exists and is readable, but something went wrong. */
> + scratch_buffer_free (&sbuf);
> + return -1;
> + }
>
> /* Remember whether we found anything. */
> any |= here_any;
> @@ -1934,6 +1981,7 @@ open_path (const char *name, size_t namelen, int mode,
> sps->dirs = (void *) -1;
> }
>
> + scratch_buffer_free (&sbuf);
> return -1;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] elf: dl-load: Get rid of alloca usage.
2023-10-17 17:50 ` Adhemerval Zanella Netto
@ 2023-10-18 13:31 ` Joe Simmons-Talbott
0 siblings, 0 replies; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-10-18 13:31 UTC (permalink / raw)
To: Adhemerval Zanella Netto; +Cc: libc-alpha
On Tue, Oct 17, 2023 at 02:50:41PM -0300, Adhemerval Zanella Netto wrote:
>
>
> On 02/10/23 10:24, Joe Simmons-Talbott wrote:
> > Replace alloca usage with scratch_buffers. Change the sematics of
> > is_trusted_path_normalize to return 1, 0, or -1 on error.
> > ---
> > elf/dl-load.c | 72 ++++++++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 60 insertions(+), 12 deletions(-)
> >
> > diff --git a/elf/dl-load.c b/elf/dl-load.c
> > index 2923b1141d..c8e135b6e5 100644
> > --- a/elf/dl-load.c
> > +++ b/elf/dl-load.c
> > @@ -21,6 +21,7 @@
> > #include <errno.h>
> > #include <fcntl.h>
> > #include <libintl.h>
> > +#include <scratch_buffer.h>
> > #include <stdbool.h>
> > #include <stdlib.h>
> > #include <string.h>
> > @@ -124,14 +125,21 @@ static const size_t system_dirs_len[] =
> > };
> > #define nsystem_dirs_len array_length (system_dirs_len)
> >
> > -static bool
> > +static int
>
> The main problem is _dl_dst_substitute, which calls is_trusted_path_normalize,
> is not suppose to fail. The expand_dynamic_string_token does handle null,
> but fillin_rpath will just ignore the entry. I think it should
> _dl_signal_error (ENOMEM, ...).
>
> Same for expand_dst macro, which does not expect to _dl_dst_substitute fail
> with memory allocation.
>
> So I think it would be better to split the patch to first only remove the
> alloca from is_trusted_path_normalize, add memory allocation checks on
> _dl_dst_substitute to return NULL if is_trusted_path_normalize fails to
> allocate memory, and add NULL checks for _dl_dst_substitute callers as well.
I've posted a v2 which hopefully does as you've suggested. I also added
the scratch_buffer_free that was overlooked. Thanks for your review.
Thanks,
Joe
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-10-18 13:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-02 13:24 [PATCH] elf: dl-load: Get rid of alloca usage Joe Simmons-Talbott
2023-10-10 19:07 ` Joe Simmons-Talbott
2023-10-17 17:50 ` Adhemerval Zanella Netto
2023-10-18 13:31 ` Joe Simmons-Talbott
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).