Posix: fix named SHM mappings permissions.

base/memory/shared_memory_unittest.cc

Index: base/memory/shared_memory_unittest.cc
diff --git a/base/memory/shared_memory_unittest.cc b/base/memory/shared_memory_unittest.cc
index 89db5112f620855d24b99e19b0d27b106f36ea75..de7497c927b72f6fa7de43d9e1597912afd91e69 100644
--- a/base/memory/shared_memory_unittest.cc
+++ b/base/memory/shared_memory_unittest.cc
@@ -8,6 +8,8 @@
 #endif
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/shared_memory.h"
+#include "base/rand_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/sys_info.h"
 #include "base/test/multiprocess_test.h"
 #include "base/threading/platform_thread.h"
@@ -21,6 +23,9 @@
 
 #if defined(OS_POSIX)
 #include <sys/mman.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
 #endif
 
 static const int kNumThreads = 5;
@@ -401,7 +406,60 @@ TEST(SharedMemoryTest, AnonymousExecutable) {
   EXPECT_EQ(0, mprotect(shared_memory.memory(), shared_memory.requested_size(),
                         PROT_READ | PROT_EXEC));
 }
-#endif
+
+// Create a shared memory object, check its permissions.
+TEST(SharedMemoryTest, FilePermissionsAnonymous) {
+  const uint32 kTestSize = 1 << 8;
+
+  SharedMemory shared_memory;
+  SharedMemoryCreateOptions options;
+  options.size = kTestSize;
+  // Set a permissive umask.
+  mode_t old_umask = umask(S_IWGRP | S_IWOTH);

[Mark Mentovai, 2013/07/02 14:31:54]

Can you restore the old umask with a scoper, in case these EXPECTs ever become
ASSERTs? Same in the next test.

[jln (very slow on Chromium), 2013/07/02 18:49:08]

I ended up created a class for this, since it's less verbose than using the
ScopedClosureRunner.

+
+  EXPECT_TRUE(shared_memory.Create(options));
+
+  int shm_fd = shared_memory.handle().fd;
+  struct stat shm_stat;
+  EXPECT_EQ(0, fstat(shm_fd, &shm_stat));
+  // Neither the group, nor others should be able to read the shared memory
+  // file.
+  EXPECT_FALSE(shm_stat.st_mode & S_IRWXO);
+  EXPECT_FALSE(shm_stat.st_mode & S_IRWXG);
+
+  // Restore umask.
+  umask(old_umask);
+}
+
+// Create a shared memory object, check its permissions.
+TEST(SharedMemoryTest, FilePermissionsNamed) {
+  const uint32 kTestSize = 1 << 8;
+
+  SharedMemory shared_memory;
+  SharedMemoryCreateOptions options;
+  options.size = kTestSize;
+  std::string shared_mem_name =
+      "shared_perm_test-" + Uint64ToString(RandUint64());

[Mark Mentovai, 2013/07/02 14:31:54]

Random numbers lead to flake. 64 bits are probably “random enough” but why play
around with this? Is there something better you can use? Something you can use
in addition?

[jln (very slow on Chromium), 2013/07/02 18:49:08]

I think this is the best way to do it. We're creating a file in a "temp"
directory and for the coverage of this particular test, we need that file to not
exist.

Paradoxically, using a random file name like this leads to a much more
predictable run than playing around with incremental file names or figuring out
whether or not a test-specific name is already used.

Especially if you take into account factors such as:
- Another instance of the test running concurrently on the machine.
- Different user ids creating files in the temporary directory at the same time.

I was tempted to check if the path already existed, but then the test would need
to have knowledge of the construction of the temporary path.

Let me know what you think!

[Mark Mentovai, 2013/07/02 19:52:56]

Julien Tinnes wrote:
You’re right, I don’t think you should be poking around on the FS.

I think you should at least add the pid to the random number, or the pid,
time(), and a random number.

The other alternative is to use whatever our mkdtemp equivalent is to get your
own unique temporary directory, and then stick the file in there. But that’s
probably cumbersome because we don’t have full control of the entire path here.

[jln (very slow on Chromium), 2013/07/02 21:20:02]

I added the PID.

+  options.name = &shared_mem_name;
+  // Set a permissive umask.
+  mode_t old_umask = umask(S_IWGRP | S_IWOTH);
+
+  EXPECT_TRUE(shared_memory.Create(options));
+  // Clean-up the backing file immediately, we don't need it.
+  EXPECT_TRUE(shared_memory.Delete(shared_mem_name));
+
+  int shm_fd = shared_memory.handle().fd;
+  struct stat shm_stat;
+  EXPECT_EQ(0, fstat(shm_fd, &shm_stat));

[Mark Mentovai, 2013/07/02 14:31:54]

If the backing file is gone, what are you testing?

[jln (very slow on Chromium), 2013/07/02 18:49:08]

The link is gone, but the inode is still there and will only be deleted by the
test once it's closed (which in this case will happen once shared_memory is
destroyed).
I clarified the comment above when deleting the link.

[Mark Mentovai, 2013/07/02 19:52:56]

Julien Tinnes wrote:
Right, but I mean: why are the permissions relevant at that point? What
interface can someone use to get an FD to the backing file if they didn’t have
one already if we goofed and the permission bits are wrong? Is it /proc/*/fd or
something?

[jln (very slow on Chromium), 2013/07/02 21:20:02]

We're checking that the file was created with the right permissions. I'm
deleting the file name as early as possible because it's easier and more
readable than to add a scoper, but this shouldn't affect the code that follows
Delete() in any way.

Let me know if I can make something more clear.

+  // Neither the group, nor others should be able to read the shared memory
+  // file.
+  EXPECT_FALSE(shm_stat.st_mode & S_IRWXO);
+  EXPECT_FALSE(shm_stat.st_mode & S_IRWXG);
+  // Restore umask.
+  umask(old_umask);
+}
+
+#endif  // defined(OS_POSIX)
 
 // Map() will return addresses which are aligned to the platform page size, this
 // varies from platform to platform though.  Since we'd like to advertise a