[PATCH v2] elf: dl-load: Get rid of alloca usage.
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Thu Oct 19 13:33:55 GMT 2023
On 18/10/23 10:08, Joe Simmons-Talbott wrote:
> Replace alloca usage with scratch_buffers. Change the semantics of
> is_trusted_path_normalize to return 1, 0, or -1 on error. Change
> _dl_dst_substitute to return NULL on error and update callers to handle
> the NULL return value.
> ---
> Changes to v1:
> * _dl_dst_substitute returns NULL on failure.
> * update callers of _dl_dst_substitute to handle NULL return value.
> * Add missing scratch_buffer_free calls.
>
> elf/dl-deps.c | 5 +--
> elf/dl-load.c | 89 ++++++++++++++++++++++++++++++++++++++++++---------
> 2 files changed, 77 insertions(+), 17 deletions(-)
>
> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index 0549b4a4ff..c6f67c3e27 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -100,8 +100,9 @@ DST not allowed in SUID/SGID programs")); \
> __dst_cnt)); \
> \
> __result = _dl_dst_substitute (l, __str, __newp); \
> - \
> - if (*__result == '\0') \
> + if (__result == NULL) \
> + _dl_signal_error (0, __str, NULL, N_("Memory allocation failed")); \
We already have the 'Memory allocation failure' message, so it won't need to
add another translation.
> + else if (*__result == '\0') \
> { \
> /* The replacement for the DST is not known. We can't \
> processed. */ \
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 2923b1141d..23b3b80c88 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;
>
> - char *npath = (char *) alloca (len + 2);
> + struct scratch_buffer sbuf;
> + scratch_buffer_init (&sbuf);
> +
> + if (!scratch_buffer_set_array_size (&sbuf, 1, len + 2))
> + return -1;
> +
> + char *npath = sbuf.data;
> char *wnp = npath;
> +
> while (*path != '\0')
> {
> if (path[0] == '/')
> @@ -171,13 +179,17 @@ 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;
> + {
> + scratch_buffer_free (&sbuf);
> + /* Found it. */
> + return 1;
> + }
>
> trun += system_dirs_len[idx] + 1;
> }
>
> - return false;
> + scratch_buffer_free (&sbuf);
> + return 0;
> }
I think you can simplify this by using a temporary variable:
int r = 0;
for (size_t idx = 0; idx < nsystem_dirs_len; ++idx)
{
if (wnp - npath >= system_dirs_len[idx]
&& memcmp (trun, npath, system_dirs_len[idx]) == 0)
/* Found it. */
{
r = 1;
break;
}
trun += system_dirs_len[idx] + 1;
}
scratch_buffer_free (&sbuf);
>
> /* Given a substring starting at INPUT, just after the DST '$' start
> @@ -270,7 +282,7 @@ _dl_dst_count (const char *input)
> least equal to the value returned by DL_DST_REQUIRED. Note that it
> is possible for a DT_NEEDED, DT_AUXILIARY, and DT_FILTER entries to
> have colons, but we treat those as literal colons here, not as path
> - list delimiters. */
> + list delimiters. Returns NULL on failure. */
> char *
> _dl_dst_substitute (struct link_map *l, const char *input, char *result)
> {
> @@ -283,6 +295,7 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
> char *wp = result;
> const char *start = input;
> bool check_for_trusted = false;
> + int itpn;
>
> do
> {
> @@ -362,8 +375,14 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
> trusted to have designed this correctly. Only $ORIGIN is tested in
> 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))
> + itpn = is_trusted_path_normalize (result, wp - result);
> + if (itpn == -1)
> + {
> + _dl_signal_error (ENOMEM, NULL, NULL, N_("Failed to allocate memory"));
> + return NULL;
> + }
> +
> + if (__glibc_unlikely (check_for_trusted) && itpn)
> {
> *result = '\0';
> return result;
This is wrong, it just want to call is_trusted_path_normalize if check_for_trusted is
set. I think it should be:
if (__glibc_unlikely (check_for_trusted))
{
int r = is_trusted_path_normalize (result, wp - result);
if (r == -1)
return NULL;
else if (r == 0)
{
*result = '\0';
return result;
}
}
So we report back any allocation failure, and clear the result if the canonical path
is not related to system dirs.
> @@ -951,6 +970,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 +1003,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 +1020,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 +1052,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 +1063,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 +1096,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)
> {
Please move this alloca removal to another patch.
> @@ -1485,7 +1515,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 +1548,7 @@ cannot enable executable stack as shared object requires");
> _dl_audit_objopen (l, nsid);
> #endif
>
> + scratch_buffer_free (&sbuf);
> return l;
> }
>
> @@ -1598,6 +1632,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 +1696,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 +1733,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 +1772,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 +1794,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)
> {
Please move this alloca removal to another patch.
> @@ -1769,6 +1815,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 +1823,7 @@ open_verify (const char *name, int fd,
>
> }
>
> + scratch_buffer_free (&sbuf);
> return fd;
> }
>
> @@ -1796,13 +1844,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;
Please move this alloca removal to another patch.
> @@ -1901,6 +1954,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 +1962,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 +1992,7 @@ open_path (const char *name, size_t namelen, int mode,
> sps->dirs = (void *) -1;
> }
>
> + scratch_buffer_free (&sbuf);
> return -1;
> }
>
You are still missing the possible allocation failure on expand_dynamic_string_token,
since if _dl_dst_substitute returns NULL it requires to free the result:
Also, on expand_dynamic_string_token call at fillin_rpath, now that we have a possible
memory allocation failure I think it would be worth to add a _dl_signal_error if the
function returns NULL.
More information about the Libc-alpha
mailing list