Re: [PATCH] shm_open/unlink: fix errno if namelen >= NAME_MAX

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 16/10/2020 07:09, Chen Li wrote:
> 
> According to linux's manpage and posix's doc, errno should be
> set to ENAMETOOLONG if the path exceeds the maximuz length:
> 
> linux man page(http://man7.org/linux/man-pages/man3/shm_open.3.html)
> 
> ```
> ENAMETOOLONG
>    The length of name exceeds PATH_MAX.
> ```
> 
> posix doc(https://pubs.opengroup.org/onlinepubs/009695399/functions/shm_open.html):
> 
> ```
> [ENAMETOOLONG]
>    The length of the name argument exceeds {PATH_MAX} or a pathname component is longer than {NAME_MAX}.
> ```
> glibc doesn't handle ENAMETOOLONG correctly previously. When the path
> exceeds the maximum value, errno was set to EINVAL instead, which
> doesn't conform the man page and posix standard.
> 
> This patch removes the NAME_MAX check in SHM_GET_NAME and leaves this
> check to open syscall, which should handle maximunize length correctly
> inside various filesystem implementations.

Although it fixes the errno value for large filenames, it also allows a
possible unbounded stack allocation since the resulting path will be
issued with alloca.

I think it would be good to refactor the code to use a PATH_MAX variable
instead, something like:

  _Bool
  shm_get_name (const char *prefix, char *shm_name, size shm_path_max)
  {
    size_t shm_dirlen;
    const char *shm_dir = __shm_directory (&shm_dirlen);
    if (shm_dir == NULL)
      {
        __set_errno (ENOSYS);
        return false;
      }
    while (name[0] == '/')
      ++name;
    size_t namelen = strlen (name) + 1;
    if (namelen == 1 || strchr (name, '/') != NULL)
      {
        __set_errno (EINVAL);
        result false;
      }
    if (shm_dirlen + namelen > shm_path_max)
      {
        __set_errno (ENAMETOOLONG);
        result false;
      }
    __mempcpy (__memcpy (shm_name, shm_dir, shm_dirlen),
               name, namelen);
    return true;
  }

It would be good to move shm_get_name to its own implementation file as
well (since it is used on both shm_open and shm_unlink).

So on sysdeps/posix/shm_open.c:

  int
  shm_open (const char *name, int oflag, mode_t mode)
  {
    char shm_path[PATH_MAX];
    if (! shm_get_name (shm_path, sizeof shm_path))
      return SEM_FAILED;
    [...]
  }

> ---
>  sysdeps/posix/shm-directory.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/posix/shm-directory.h b/sysdeps/posix/shm-directory.h
> index c7979ebb72..5a1aab2c14 100644
> --- a/sysdeps/posix/shm-directory.h
> +++ b/sysdeps/posix/shm-directory.h
> @@ -53,7 +53,7 @@ extern const char *__shm_directory (size_t *len);
>      ++name;								      \
>    size_t namelen = strlen (name) + 1;					      \
>    /* Validate the filename.  */						      \
> -  if (namelen == 1 || namelen >= NAME_MAX || strchr (name, '/') != NULL)      \
> +  if (namelen == 1 || strchr (name, '/') != NULL)      \
>      {									      \
>        __set_errno (errno_for_invalid);					      \
>        return retval_for_invalid;					      \
> 
>