diff mbox series

shm_open/unlink: fix errno if namelen >= NAME_MAX

Message ID 87lfg6zdl5.wl-chenli@uniontech.com
State New
Headers show
Series shm_open/unlink: fix errno if namelen >= NAME_MAX | expand

Commit Message

Chen Li Oct. 16, 2020, 10:09 a.m. UTC
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.
---
 sysdeps/posix/shm-directory.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Adhemerval Zanella Oct. 26, 2020, 7:44 p.m. UTC | #1
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;					      \
> 
>
Florian Weimer Oct. 27, 2020, 10:27 a.m. UTC | #2
* Adhemerval Zanella via Libc-alpha:

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

As this is generic code, I don't think we can assume PATH_MAX is defined
(Hurd does not have it AFAIK).

Thanks,
Florian
Adhemerval Zanella Oct. 27, 2020, 11:53 a.m. UTC | #3
On 27/10/2020 07:27, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> I think it would be good to refactor the code to use a PATH_MAX variable
>> instead, something like:
> 
> As this is generic code, I don't think we can assume PATH_MAX is defined
> (Hurd does not have it AFAIK).

Although Hurd uses the POSIX implementation, I am not sure if it is fully
functional.  It does not support sem [1] [2].

I would like to avoid creating a unbounded path, at least on Linux with
BZ#25383 expected fix it would highly unlikely we will even need paths
larger than PATH_MAX (and current implementation does imposes the NAME_MAX
limit).

So I think it would be better to make Linux uses at maximum PATH_MAX, or 
even NAME_MAX plus "/dev/shm/".

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=25524
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=25521
diff mbox series

Patch

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;					      \