This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][BZ #16274] Fix shm_open.


On 11/28/2013 07:40 PM, OndÅej BÃlka wrote:
> Hi,
> 
> during second revision of patch I added a explicit null check but for
> some reason got these backwards.
> 
> As result of testsuite stayed same I assumed it was ok but did not
> expected that actual test is:
> 
>   /* Create the shared memory object.  */
>   fd = shm_open ("/shm-test", O_RDWR | O_CREAT | O_TRUNC | O_EXCL,
> 0600);
>   if (fd == -1)
>     {
>       /* We don't regard this as a bug.  Simply don't run the test.  It
> could
>          means there is no such implementation or the object is already
> in
>          use in which case we don't want to disturb.  */
>       perror ("failed to create a shared memory object: shm_open");
>       return 0;
>     }
> 
> which succeeded.

Please immediately change the test to be something else e.g. "/glibc-shm-test"
and remove the `return 0;' case.and repost v2 of the patch.

I don't want this test case to bite us again, it's silly not to expect that
you have some control over the name of the shared memory file.

> 
> 	[BZ #16274]
> 	sysdeps/unix/sysv/linux/shm_open.c (shm_open): Fix implementation.

That's not an appropriate description of the change.

> 
> diff --git a/sysdeps/unix/sysv/linux/shm_open.c b/sysdeps/unix/sysv/linux/shm_open.c
> index 482b49c..7bb2874 100644
> --- a/sysdeps/unix/sysv/linux/shm_open.c
> +++ b/sysdeps/unix/sysv/linux/shm_open.c
> @@ -151,7 +151,7 @@ shm_open (const char *name, int oflag, mode_t mode)
>    namelen = strlen (name);
>  
>    /* Validate the filename.  */
> -  if (name[0] == '\0' || namelen > NAME_MAX || strchr (name, '/') == NULL)
> +  if (name[0] == '\0' || namelen > NAME_MAX || strchr (name, '/') != NULL)

POSIX says:
If name begins with the slash character, then processes calling shm_open() 
with the same value of name refer to the same shared memory object, as long 
as that name has not been removed. If name does not begin with the slash 
character, the effect is implementation-defined. The interpretation of slash 
characters other than the leading slash character in name is 
implementation-defined.

So we are going to say:
- glibc's implementation allows a name without a starting slash e.g. "foo"
- glibc's implementation does not allow non-leading slashes e.g. "foo/bar"
  - Even though Linux allows mkdir on /dev/shm e.g.
    mkdir /dev/shm/foo; shm_open "foo/bar"

Could you please add two stubs for shm_open, and shm_unlink in
manual/memory.texi and include our implementation-defined details there?

>      {
>        __set_errno (EINVAL);
>        return -1;
> @@ -241,7 +241,7 @@ shm_unlink (const char *name)
>    namelen = strlen (name);
>  
>    /* Validate the filename.  */
> -  if (name[0] == '\0' || namelen > NAME_MAX || strchr (name, '/') == NULL)
> +  if (name[0] == '\0' || namelen > NAME_MAX || strchr (name, '/') != NULL)
>      {
>        __set_errno (ENOENT);
>        return -1;
> 

In summary:

* Fix the test.
* Document the implementation-defined choices we've just made.

Post v2 please.

Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]