[v2] elf: dl-load: Get rid of alloca usage.
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-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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(-)
Comments
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.
On Thu, Oct 19, 2023 at 10:33:55AM -0300, Adhemerval Zanella Netto wrote:
>
>
> 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.
>
I've posted a v3 patch that makes all of the changes you've suggested.
Thanks for the review.
Thanks,
Joe
> > + 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.
>
@@ -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")); \
+ else if (*__result == '\0') \
{ \
/* The replacement for the DST is not known. We can't \
processed. */ \
@@ -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;
}
/* 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;
@@ -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)
{
@@ -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)
{
@@ -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;
@@ -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;
}