Re: [PATCH v2 1/2][BZ #16274] Fix shm_open.

["Carlos O'Donell" <carlos at redhat dot com>]

On 11/29/2013 01:02 PM, OndÅej BÃlka wrote:
> On Thu, Nov 28, 2013 at 09:06:46PM -0500, Carlos O'Donell wrote:
>> 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.
>>
>>> @@ -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?
>>
>> In summary:
>>
>> * Fix the test.
>> * Document the implementation-defined choices we've just made.
>>
>> Post v2 please.
>>
>> Cheers,
>> Carlos.
> 
> A fix with testcase is here. I changed detection of skipping test to
> checking if errno is ENOSYS. I send documentation in separate patch.
> 
> 	[BZ #16274]
> 	* sysdeps/unix/sysv/linux/shm_open.c (shm_open): Correctly
> 	handle filename validation.
> 	* rt/tst-shm.c (do_test, do_open: Do not skip a test when
> 	shm_open fails.
> 
> ---
>  rt/tst-shm.c                       | 31 ++++++++++++++++---------------
>  sysdeps/unix/sysv/linux/shm_open.c |  4 ++--
>  2 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/rt/tst-shm.c b/rt/tst-shm.c
> index cb4b1ee..7f21d4e 100644
> --- a/rt/tst-shm.c
> +++ b/rt/tst-shm.c
> @@ -41,12 +41,12 @@ do_open (void)
>    int fd;
>  
>    /* Create the shared memory object.  */
> -  fd = shm_open ("/shm-test", O_RDWR, 0600);
> +  fd = shm_open ("/glibc-shm-test", O_RDWR, 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.  */
> +      /* If shm_open is unimplemented we skip a test.  */

s/a test/the test/g

> +      if (errno != ENOSYS)

Shouldn't this be "if (errno == ENOSYS)"? Since an unimplemented function is
the only reason to skip the test and return 0.

> +        error (EXIT_FAILURE, 0, "failed to open shared memory object: shm_open");

Suggest: "Failed to open shared memory object: shm_open unimplemented. Test skipped." wrapped appropriately.

>        perror ("failed to open shared memory object: shm_open");
>        return -1;
>      }
> @@ -143,12 +143,13 @@ do_test (void)
>  
>  
>    /* Create the shared memory object.  */
> -  fd = shm_open ("/shm-test", O_RDWR | O_CREAT | O_TRUNC | O_EXCL, 0600);
> +  fd = shm_open ("/glibc-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.  */
> +      /* If shm_open is unimplemented we skip a test.  */

s/a test/the test/g

> +      if (errno != ENOSYS)

Same question here, shouldn't this be "errno == ENOSYS"?

> +        error (EXIT_FAILURE, 0, "failed to open shared memory object: shm_open");
> +

Suggest: "Failed to open shared memory object: shm_open unimplemented. Test skipped." wrapped appropriately.

>        perror ("failed to create a shared memory object: shm_open");
>        return 0;

Shouldn't this be `return -1;' ?

We want to exit with 0 if the error was ENOSYS otherwise -1?

Maybe it's just Friday and I'm confused though...

>      }
> @@ -160,18 +161,18 @@ do_test (void)
>           shared memory itself.  */
>        perror ("failed to size of shared memory object: ftruncate");
>        close (fd);
> -      shm_unlink ("/shm-test");
> +      shm_unlink ("/glibc-shm-test");
>        return 0;
>      }
>  
>    if (fstat64 (fd, &st) == -1)
>      {
> -      shm_unlink ("/shm-test");
> +      shm_unlink ("/glibc-shm-test");
>        error (EXIT_FAILURE, 0, "initial stat failed");
>      }
>    if (st.st_size != 4000)
>      {
> -      shm_unlink ("/shm-test");
> +      shm_unlink ("/glibc-shm-test");
>        error (EXIT_FAILURE, 0, "initial size not correct");
>      }
>  
> @@ -184,7 +185,7 @@ do_test (void)
>        /* Couldn't create a second process.  */
>        perror ("fork");
>        close (fd);
> -      shm_unlink ("/shm-test");
> +      shm_unlink ("/glibc-shm-test");
>        return 0;
>      }
>  
> @@ -199,7 +200,7 @@ do_test (void)
>        kill (pid1, SIGTERM);
>        waitpid (pid1, &ignore, 0);
>        close (fd);
> -      shm_unlink ("/shm-test");
> +      shm_unlink ("/glibc-shm-test");
>        return 0;
>      }
>  
> @@ -208,14 +209,14 @@ do_test (void)
>    waitpid (pid2, &status2, 0);
>  
>    /* Now we can unlink the shared object.  */
> -  shm_unlink ("/shm-test");
> +  shm_unlink ("/glibc-shm-test");
>  
>    return (!WIFEXITED (status1) || WEXITSTATUS (status1) != 0
>  	  || !WIFEXITED (status2) || WEXITSTATUS (status2) != 0);
>  }
>  #define TEST_FUNCTION do_test ()
>  
> -#define CLEANUP_HANDLER shm_unlink ("/shm-test");
> +#define CLEANUP_HANDLER shm_unlink ("/glibc-shm-test");

OK.

>  
>  
>  #include "../test-skeleton.c"
> 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)

OK.

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

OK.

>      {
>        __set_errno (ENOENT);
>        return -1;
> 

Cheers,
Carlos.