shm_open/unlink: fix errno if namelen >= NAME_MAX

  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(-)
  

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

* 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
  

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
  

On  Mon Oct 26 19:44:44 GMT 2020,
Adhemerval Zanella  wrote:

> 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)

So you prefer function over macro here? If so, shm_dir, shm_dirlen, shm_name
should all be passed as args too, because unlike c macro, function is not
dynamical scope. Given such long arg list(errno_for_invalid, retval_for_invalid,
prefix, shm_dir, shm_dirlen, shm_name), I'm not sure weathre it confrom glibc's
coding style, and is refactoring to split into more small functions a good idea? 


Thank,
Chen Li
  

On 03/11/2020 11:45, Chen Li wrote:
> 
> On  Mon Oct 26 19:44:44 GMT 2020,
> Adhemerval Zanella  wrote:
> 
>> 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)
> 
> So you prefer function over macro here? If so, shm_dir, shm_dirlen, shm_name
> should all be passed as args too, because unlike c macro, function is not
> dynamical scope. Given such long arg list(errno_for_invalid, retval_for_invalid,
> prefix, shm_dir, shm_dirlen, shm_name), I'm not sure weathre it confrom glibc's
> coding style, and is refactoring to split into more small functions a good idea? 

Florian has pointed me out a better solution would be use
use his fix [1] for BZ#25383 [2] which refactor the POSIX sem/shm
code to use a static buffer instead.

Then the ENAMETOOLONG could be returned by '__shm_get_name'
instead.

Florian, do you plan to send your patch for review?

[1] https://sourceware.org/bugzilla/attachment.cgi?id=12921
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=25383
  

* Adhemerval Zanella:

> Florian has pointed me out a better solution would be use
> use his fix [1] for BZ#25383 [2] which refactor the POSIX sem/shm
> code to use a static buffer instead.
>
> Then the ENAMETOOLONG could be returned by '__shm_get_name'
> instead.
>
> Florian, do you plan to send your patch for review?

I'm working on the change you requested (eliminate the new struct
type).  No ETA yet.  Hopefully still this month.

Thanks,
Florian
  

On 03/11/2020 13:49, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Florian has pointed me out a better solution would be use
>> use his fix [1] for BZ#25383 [2] which refactor the POSIX sem/shm
>> code to use a static buffer instead.
>>
>> Then the ENAMETOOLONG could be returned by '__shm_get_name'
>> instead.
>>
>> Florian, do you plan to send your patch for review?
> 
> I'm working on the change you requested (eliminate the new struct
> type).  No ETA yet.  Hopefully still this month.

Thanks, I think the NAME_MAX errno fix should be simpler on top of
your patch.