Add O_CLOEXEC to open calls.

[adam-azarchs]

This prevents file descriptors from leaking to child processes.

When compiled for older (pre-2.6.23) kernels which lack support for O_CLOEXEC, there is no change in behavior. With newer kernels, child processes will no longer inherit leveldb's file handles, which reduces the changes of accidentally corrupting the database.

Fixes #623

[pwnall]

This change seems reasonable to me. Can you please add tests?

[adam-azarchs]

Added a test as requested. Sorry about being lazy about it before. Before:

==== Test EnvPosixTest.TestCloseOnExec
Unexpected open file 3
File descriptor 3 is /tmp/leveldbtest-5046/close_on_exec_seq.txt
/mnt/home/adam.azarchs/leveldb/util/env_posix_test.cc:104: failed: 0 == 1

After:

==== Test EnvPosixTest.TestCloseOnExec
==== PASSED 2 tests

[adam-azarchs]

Hm, seems that while it passes fine for ./env_posix_test, it fails when running through make (as it does in travis). Probably because make is leaking file descriptors to it as well. Will need to iterate on this a bit more.

[adam-azarchs]

Fixed the error. It does seem somewhat concerning that travis considers the build to have passed even if a unit test fails like that, however.

[adam-azarchs]

Sorry for the noise earlier. Been spoiled by for Go too long I guess. The PR is now stable. Please take a look and let me know if further changes are desired.

[adam-azarchs]

Are there any further changes you'd like to see on this PR?

[adam-azarchs]

Rebased to master to resolve conflicts.

[pwnall]

@adam-azarchs Thank you for your work, and sorry for the slow response! This looks good, and I'll look at adopting it next time I have a chunk of time for LevelDB. I may try to use CMake for the O_CLOEXEC feature detection, but everything else looks good to me.

[adam-azarchs]

I've rebased to master and modified things to use cmake for the feature detection. Admittedly I don't use cmake often so I'm cargo culting that bit, but it seems to work.

[pwnall]

@adam-azarchs Thank you very much for the revisions!

I expect us to get the Windows CI green over the next couple of days. I plan to look into this PR right after the CI becomes green.

[adam-azarchs]

This seems to have broken the travis build on mac. I don't have a mac to test on so it's a bit difficult for me to resolve this. It looks like those errors were being inadvertently ignored by travis, previously. I'm guessing the issue is that macs don't have a /proc/self/exe?

[pwnall]

Macs don't have procfs, indeed. So, no /proc/self/exe or /proc/self/fd.

The bigger question is -- why did this PR pass CI in the first place. I think it's fair to say we were all relying on the Travis results here.

I'm trying to see if I can figure out a quick fix for this issue.

[pwnall]

(continued from the previous comment, I accidentally hit Enter)

https://travis-ci.org/google/leveldb/jobs/497244562#L2789 shows that the test failed on Mac, as expected, but the failure didn't bubble up to the Travis status.

Good news: the travis config used by the PR had a bug that was fixed in 808e59e

So, I'll assume that the CI configuration problem was fixed, and we only have to deal with the broken test. This is a good example of the importance of getting that CI config right 😄

[hebasto]

#cmakedefine01 always defines the HAVE_O_CLOEXEC macro:

Input file lines of the form #cmakedefine01 VAR will be replaced with either #define VAR 1 or #define VAR 0 similarly.

Therefore, it looks wrong to use #if defined(HAVE_O_CLOEXEC) in the code:

leveldb/util/env_posix.cc

Line 52 in 75fceae

#if defined(HAVE_O_CLOEXEC)

and

leveldb/util/env_posix_test.cc

Line 62 in 75fceae

#if defined(HAVE_O_CLOEXEC)

UPDATE: The latter has been fixed in 27dc99f.

UPDATE 2: Have just noticed that a fix has been suggested in #1028.