Adaptively use temp dir instead of /dev/shm for executable shmem file on Linux

Adaptively use temp dir instead of /dev/shm for executable shmem file on Linux

On some Linux systems, files from /dev/shm cannot have PROT_EXEC applied to
their mappings.  This depends on picayune setup details that vary between
distributions and kernels and could vary between installations.  So just
use an empirical test of whether it works or not, and fall back to using
generic temporary space instead of /dev/shm for the executable case if needed.

BUG= http://code.google.com/p/chromium/issues/detail?id=103377
TEST= SharedMemory.AnonymousExecutable with /dev/shm mounted noexec

R=mark@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113228

[Mark Mentovai, 2011-12-05 23:44:46]

http://codereview.chromium.org/8800025/diff/1/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/8800025/diff/1/base/file_util_posix.cc#newcode937
base/file_util_posix.cc:937: int fd =
CreateAndOpenFdForTemporaryFile(FilePath("/dev/shm"), &path);
A ScopedFDClose would be a good idea here.

http://codereview.chromium.org/8800025/diff/1/base/file_util_posix.cc#newcode964
base/file_util_posix.cc:964: if (!executable ||
It might be better to do:

  DevShmExecutable dev_shm_executable;
  if (executable) {
    static DevShmExecutable s_dev_shm_executable = DetermineDevShmExecutable();
    dev_shm_executable = s_dev_shm_executable;
  }
  if (!executable || dev_shm_executable == DevShmExecutableYes) {
    // ...
  }

This gets rid of g_dev_shm_executable (no big deal) but also gets rid of the
DevShmExecutableUnknown state and eliminates your own tracking of that state.
You’re using the static to your advantage.

I’m assuming that you’re not worried about threading concerns here, but the
patch you proposed also seemed not to be worried about threading.

[Roland McGrath, 2011-12-06 00:18:15]

On 2011/12/05 23:44:46, Mark Mentovai wrote:
> http://codereview.chromium.org/8800025/diff/1/base/file_util_posix.cc
> File base/file_util_posix.cc (right):
> 
>
http://codereview.chromium.org/8800025/diff/1/base/file_util_posix.cc#newcode937
> base/file_util_posix.cc:937: int fd =
> CreateAndOpenFdForTemporaryFile(FilePath("/dev/shm"), &path);
> A ScopedFDClose would be a good idea here.

Done.

>
http://codereview.chromium.org/8800025/diff/1/base/file_util_posix.cc#newcode964
> base/file_util_posix.cc:964: if (!executable ||
> It might be better to do:
> 
>   DevShmExecutable dev_shm_executable;
>   if (executable) {
>     static DevShmExecutable s_dev_shm_executable =
DetermineDevShmExecutable();
>     dev_shm_executable = s_dev_shm_executable;
>   }
>   if (!executable || dev_shm_executable == DevShmExecutableYes) {
>     // ...
>   }
>
> This gets rid of g_dev_shm_executable (no big deal) but also gets rid of the
> DevShmExecutableUnknown state and eliminates your own tracking of that state.
> You’re using the static to your advantage.

Done.  I had thought there was some style admonition against using
initialized statics, but I must have misremembered.

> I’m assuming that you’re not worried about threading concerns here, but the
> patch you proposed also seemed not to be worried about threading.

The first version included a comment about how a race would be harmless.
But the version using the static in fact has implicit synchronization,
because all initialized statics do.  (It compiles into a call to
pthread_once.)


Thanks,
Roland

[Roland McGrath, 2011-12-06 01:22:21]

PTAL.  It would be nice if I could manage to land this before the branch. 
Though it's certainly not the end of the world (it's only to fix Gentoo users
running NaCl).

[Mark Mentovai, 2011-12-06 17:55:13]

LGTM