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

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

On 12/02/2013 06:04 PM, OndÅej BÃlka wrote:
> On Fri, Nov 29, 2013 at 02:48:31PM -0500, Carlos O'Donell wrote:
>>> +      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...
>>
> It was double negation, which is correct but bit confusing.
> 
> Here is new version, a do_open function uses a existing shared object so
> I simply inlined it with appropriate error. 

Thanks.
 
> 
> 	[BZ #16274]
> 	* sysdeps/unix/sysv/linux/shm_open.c (shm_open): Correctly
> 	handle filename validation.                               
> 	* rt/tst-shm.c (do_test): Do not skip a test when
> 	shm_open fails.
> 	(do_open): Delete.
> 
> 
> diff --git a/rt/tst-shm.c b/rt/tst-shm.c
> index cb4b1ee..83ad586 100644
> --- a/rt/tst-shm.c
> +++ b/rt/tst-shm.c
> @@ -34,34 +34,17 @@
>  /* We want to see output immediately.  */
>  #define STDOUT_UNBUFFERED
>  
> -
> -static int
> -do_open (void)
> -{
> -  int fd;
> -
> -  /* Create the shared memory object.  */
> -  fd = shm_open ("/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.  */
> -      perror ("failed to open shared memory object: shm_open");
> -      return -1;
> -    }
> -
> -  return fd;
> -}
> -
> -
>  static void
>  worker (int write_now)
>  {
>    struct timespec ts;
>    struct stat64 st;
>    int i;
> -  int fd = do_open ();
> +  int fd = shm_open ("/glibc-shm-test", O_RDWR, 0600);
> +
> +  if (fd == -1)
> +    error (EXIT_FAILURE, 0, "failed to open shared memory object: shm_open");
> +
>    char *mem;
>  
>    if (fd == -1)
> @@ -143,14 +126,17 @@ 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.  */
> -      perror ("failed to create a shared memory object: shm_open");
> -      return 0;
> +      /* If shm_open is unimplemented we skip the test.  */
> +      if (errno == ENOSYS)
> +        {
> +	  perror ("shm_open unimplemented.  Test skipped.");
> +          return 0;
> +        }
> +      else
> +        error (EXIT_FAILURE, 0, "failed to create shared memory object: shm_open");

Shouldn't this be s/0/-1/g? It's a real failure?

>      }
>  
>    /* Size the object.  We make it 4000 bytes long.  */
> @@ -160,18 +146,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 +170,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 +185,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 +194,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");
>  
>  
>  #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)
>      {
>        __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;
> 

Cheers,
Carlos.