[PATCH] posix: execvpe: fix UMR with file > NAME_MAX [BZ #33627]

[Collin Funk]

Pádraig Brady <P@draigBrady.com> writes:

> * posix/execvpe.c (__execvpe_common): Since strnlen doesn't inspect
> beyond NAME_MAX and NAME_MAX does not cover the NUL, we need
> to explicitly check for the NUL.  I.e. the existing check for,
> file_len-1 > NAME_MAX, was never true.  This check is required
> so that we're guaranteed that file_len includes the NUL, as we
> depend on that in the following memcpy to properly terminate
> the file buffer passed to execve().  Otherwise that call will trigger
> UMR when inspecting the passed file, which can be seen with valgrind.
> Note returning ENAMETOOLONG early here for FILE names > NAME_MAX
> will also avoid redundant processing of ENAMETOOLONG on each entry
> in $PATH, after the change in [BZ #33626] is applied.
> ---
>  posix/execvpe.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/posix/execvpe.c b/posix/execvpe.c
> index 0fb09b9631..433b5bb0b3 100644
> --- a/posix/execvpe.c
> +++ b/posix/execvpe.c
> @@ -98,8 +98,9 @@ __execvpe_common (const char *file, char *const argv[], char *const envp[],
>    size_t file_len = __strnlen (file, NAME_MAX) + 1;
>    size_t path_len = __strnlen (path, PATH_MAX - 1) + 1;
>  
> -  /* NAME_MAX does not include the terminating null character.  */
> -  if ((file_len - 1 > NAME_MAX)
> +  /* NAME_MAX does not include the terminating NUL character.
> +     The following check ensures FILE is NUL terminated.  */
> +  if ((file_len - 1 == NAME_MAX && file[NAME_MAX] != '\0')
>        || !__libc_alloca_cutoff (path_len + file_len + 1))
>      {
>        errno = ENAMETOOLONG;

Nice catch. Thanks!

Reviewed-by: Collin Funk <collin.funk1@gmail.com>

Collin