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

[OndÅej BÃlka <neleai at seznam dot cz>]

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.  */
+      if (errno != ENOSYS)
+        error (EXIT_FAILURE, 0, "failed to open shared memory object: shm_open");
       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.  */
+      if (errno != ENOSYS)
+        error (EXIT_FAILURE, 0, "failed to open shared memory object: shm_open");
+
       perror ("failed to create a shared memory object: shm_open");
       return 0;
     }
@@ -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");
 
 
 #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;
-- 
1.8.4.rc3