diff mbox series

rt: fix shm_open not set ENAMETOOLONG when name exceeds {_POSIX_PATH_MAX}

Message ID 20230307083229.629411-1-abushwangs@gmail.com
State Superseded
Headers
Series rt: fix shm_open not set ENAMETOOLONG when name exceeds {_POSIX_PATH_MAX} |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

abushwang March 7, 2023, 8:32 a.m. UTC 
  according to man-pages-posix-2017, shm_open() function may fail if the length
of the name argument exceeds {_POSIX_PATH_MAX} and set ENAMETOOLONG

Signed-off-by: abushwang <abushwangs@gmail.com>
---
 posix/shm-directory.c | 5 ++++-
 rt/shm_open.c         | 5 +++--
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

Xi Ruoyao March 7, 2023, 8:54 a.m. UTC | #1 
On Tue, 2023-03-07 at 16:32 +0800, abushwang via Libc-alpha wrote:
> according to man-pages-posix-2017, shm_open() function may fail if the length
> of the name argument exceeds {_POSIX_PATH_MAX} and set ENAMETOOLONG

It's "may" fail, not "shall" fail.  POSIX 2017 says "may" means:

may

Describes a feature or behavior that is optional for an implementation
that conforms to POSIX.1-2017. An application should not rely on the
existence of the feature or behavior. An application that relies on such
a feature or behavior cannot be assured to be portable across conforming
implementations.

We should not break existing programs just for a "may" clause.

/* snip */

>  int
>  __shm_get_name (struct shmdir_name *result, const char *name, bool
> sem_prefix)
> @@ -50,13 +51,15 @@ __shm_get_name (struct shmdir_name *result, const
> char *name, bool sem_prefix)
>    while (name[0] == '/')
>      ++name;
>    namelen = strlen (name);
> +  if (namelen > NAME_MAX)
> +    return ENAMETOOLONG;
>  
>    if (sem_prefix)
>      alloc_buffer_copy_bytes (&buffer, "sem.", strlen ("sem."));
>    alloc_buffer_copy_bytes (&buffer, name, namelen + 1);
>    if (namelen == 0 || memchr (name, '/', namelen) != NULL
>        || alloc_buffer_has_failed (&buffer))

If you really want ENAMETOOLONG I guess you can check if namelen >
NAME_MAX here (as a very long namelen would have caused an allocation
failure).

By the way if namelen <= NAME_MAX but alloc_buffer_has_failed (due to a
high memory usage) should we say ENOSPC instead of EINVAL?  POSIX 2017
says:

[ENOSPC]
There is insufficient space for the creation of the new shared memory object.

> -    return -1;
> +    return EINVAL;
>    return 0;
>  }
abushwang March 7, 2023, 9:46 a.m. UTC | #2 
I have re-send this patch according to your suggest.

Thinks
abushwang

Xi Ruoyao <xry111@xry111.site> 于2023年3月7日周二 16:54写道：

> On Tue, 2023-03-07 at 16:32 +0800, abushwang via Libc-alpha wrote:
> > according to man-pages-posix-2017, shm_open() function may fail if the
> length
> > of the name argument exceeds {_POSIX_PATH_MAX} and set ENAMETOOLONG
>
> It's "may" fail, not "shall" fail.  POSIX 2017 says "may" means:
>
> may
>
> Describes a feature or behavior that is optional for an implementation
> that conforms to POSIX.1-2017. An application should not rely on the
> existence of the feature or behavior. An application that relies on such
> a feature or behavior cannot be assured to be portable across conforming
> implementations.
>
> We should not break existing programs just for a "may" clause.
>
> /* snip */
>
> >  int
> >  __shm_get_name (struct shmdir_name *result, const char *name, bool
> > sem_prefix)
> > @@ -50,13 +51,15 @@ __shm_get_name (struct shmdir_name *result, const
> > char *name, bool sem_prefix)
> >    while (name[0] == '/')
> >      ++name;
> >    namelen = strlen (name);
> > +  if (namelen > NAME_MAX)
> > +    return ENAMETOOLONG;
> >
> >    if (sem_prefix)
> >      alloc_buffer_copy_bytes (&buffer, "sem.", strlen ("sem."));
> >    alloc_buffer_copy_bytes (&buffer, name, namelen + 1);
> >    if (namelen == 0 || memchr (name, '/', namelen) != NULL
> >        || alloc_buffer_has_failed (&buffer))
>
> If you really want ENAMETOOLONG I guess you can check if namelen >
> NAME_MAX here (as a very long namelen would have caused an allocation
> failure).
>
> By the way if namelen <= NAME_MAX but alloc_buffer_has_failed (due to a
> high memory usage) should we say ENOSPC instead of EINVAL?  POSIX 2017
> says:
>
> [ENOSPC]
> There is insufficient space for the creation of the new shared memory
> object.
>
> > -    return -1;
> > +    return EINVAL;
> >    return 0;
> >  }
>
> --
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University
>
abushwang March 7, 2023, 9:46 a.m. UTC | #3 
ENOSPC mean insufficient shared memory object, alloc_buffer_has_failed mean
insufficient memory for name, maybe they are different.

abush wang <abushwangs@gmail.com> 于2023年3月7日周二 17:46写道：

> I have re-send this patch according to your suggest.
>
> Thinks
> abushwang
>
> Xi Ruoyao <xry111@xry111.site> 于2023年3月7日周二 16:54写道：
>
>> On Tue, 2023-03-07 at 16:32 +0800, abushwang via Libc-alpha wrote:
>> > according to man-pages-posix-2017, shm_open() function may fail if the
>> length
>> > of the name argument exceeds {_POSIX_PATH_MAX} and set ENAMETOOLONG
>>
>> It's "may" fail, not "shall" fail.  POSIX 2017 says "may" means:
>>
>> may
>>
>> Describes a feature or behavior that is optional for an implementation
>> that conforms to POSIX.1-2017. An application should not rely on the
>> existence of the feature or behavior. An application that relies on such
>> a feature or behavior cannot be assured to be portable across conforming
>> implementations.
>>
>> We should not break existing programs just for a "may" clause.
>>
>> /* snip */
>>
>> >  int
>> >  __shm_get_name (struct shmdir_name *result, const char *name, bool
>> > sem_prefix)
>> > @@ -50,13 +51,15 @@ __shm_get_name (struct shmdir_name *result, const
>> > char *name, bool sem_prefix)
>> >    while (name[0] == '/')
>> >      ++name;
>> >    namelen = strlen (name);
>> > +  if (namelen > NAME_MAX)
>> > +    return ENAMETOOLONG;
>> >
>> >    if (sem_prefix)
>> >      alloc_buffer_copy_bytes (&buffer, "sem.", strlen ("sem."));
>> >    alloc_buffer_copy_bytes (&buffer, name, namelen + 1);
>> >    if (namelen == 0 || memchr (name, '/', namelen) != NULL
>> >        || alloc_buffer_has_failed (&buffer))
>>
>> If you really want ENAMETOOLONG I guess you can check if namelen >
>> NAME_MAX here (as a very long namelen would have caused an allocation
>> failure).
>>
>> By the way if namelen <= NAME_MAX but alloc_buffer_has_failed (due to a
>> high memory usage) should we say ENOSPC instead of EINVAL?  POSIX 2017
>> says:
>>
>> [ENOSPC]
>> There is insufficient space for the creation of the new shared memory
>> object.
>>
>> > -    return -1;
>> > +    return EINVAL;
>> >    return 0;
>> >  }
>>
>> --
>> Xi Ruoyao <xry111@xry111.site>
>> School of Aerospace Science and Technology, Xidian University
>>
>
diff mbox series

Patch

diff --git a/posix/shm-directory.c b/posix/shm-directory.c
index 86d9fd8e4f..93aaeb60a0 100644
--- a/posix/shm-directory.c
+++ b/posix/shm-directory.c
@@ -25,6 +25,7 @@ 
 #include <string.h>
 #include <sys/mman.h>
 #include <fcntl.h>
+#include <errno.h>
 
 int
 __shm_get_name (struct shmdir_name *result, const char *name, bool sem_prefix)
@@ -50,13 +51,15 @@  __shm_get_name (struct shmdir_name *result, const char *name, bool sem_prefix)
   while (name[0] == '/')
     ++name;
   namelen = strlen (name);
+  if (namelen > NAME_MAX)
+    return ENAMETOOLONG;
 
   if (sem_prefix)
     alloc_buffer_copy_bytes (&buffer, "sem.", strlen ("sem."));
   alloc_buffer_copy_bytes (&buffer, name, namelen + 1);
   if (namelen == 0 || memchr (name, '/', namelen) != NULL
       || alloc_buffer_has_failed (&buffer))
-    return -1;
+    return EINVAL;
   return 0;
 }
 libc_hidden_def (__shm_get_name)
diff --git a/rt/shm_open.c b/rt/shm_open.c
index 6c1f4d604f..fc1dc96bb4 100644
--- a/rt/shm_open.c
+++ b/rt/shm_open.c
@@ -30,9 +30,10 @@  int
 __shm_open (const char *name, int oflag, mode_t mode)
 {
   struct shmdir_name dirname;
-  if (__shm_get_name (&dirname, name, false) != 0)
+  int ret =__shm_get_name (&dirname, name, false);
+  if (ret != 0)
     {
-      __set_errno (EINVAL);
+      __set_errno (ret);
       return -1;
     }