[PATCH] Avoid leaking file descriptors.

Hi,

I’ve noticed that we leak file descriptors, since we don’t use O_CLOEXEC when opening files.
I’ve changed lldb_utility::PseudoTerminal, debugserver version of it and lldb_private::File to set it while opening by default.

This can be disabled by defining:
LLDB_DISABLE_O_CLOEXEC and LLDB_DISABLE_DUPFD_CLOEXEC, currently not integrated with any filesystem.
It also should compile if O_CLOEXEC and F_DUPFD_CLOEXEC aren’t defined, and give compilation warning.

Also added new File open option:
eOpenOptionInheritOnExecPosixFD that makes it fallback to old behavior, however in whole code base it was not required, and could be removed…

I went pretty carefully through fork()/exec() and posix_spawn() codepaths to ensure that everything is OK, and it looked it is, but might have missed something.
None of them required changes, from what I have noticed, since use dup2 or posix_spawnattr_add*.

If any of your out of tree code needs to be adjusted, PseudoTerminal::Fork looks like good example how to pass fd without leaking.

I’ve tested linux amd64 and haven’t noticed any regressions comparing to trunk, but some tests fail in non-deterministic way, so could mask it…

Most notably:

TestStepNoDebug.py
TestCommandRegex.py

Managed to make them fail with and without the patch running in separate.

I’ve possibly broke debugserver when WITH_SPRINGBOARD is defined for launch code path.
I don’t know SBSLaunchApplicationForDebugging semantics, and have no idea how to fix it, beside disabling it for PseudoTerminal if this option is defined.
Those changes are not strictly required for debugserver yet I have made them for completeness.

Both raw diff and git patch are attached.

Please test attached, and report back if any problems are noticed.

Cheers,
/Piotr

PS.
@Greg, sorry that it took longer than I thought it will.

Avoid-leaking-file-descriptors-to-child-processes.diff (21.1 KB)

0001-Avoid-leaking-file-descriptors-to-child-processes.patch (22.4 KB)

Looking at this now.

btw on this one:

TestStepNoDebug.py

I disabled the tests in that file that were failing intermittently for me, early today.

I haven’t been seeing this one at all:

TestCommandRegex.py

Regarding the patch, I ran the full test suite twice without any errors on Ubuntu 12.04 x86_64. The changes look good here. I see quite a bit of code for handling Windows properly - have you verified it works okay there, Piotr?

Greg - could you give this a whirl on MacOSX and make sure it looks good to you? Also see Piotr’s springboard notes above.

Thanks,
Todd

It seems weird to me that TestStepNoDebug.py should be failing intermittently on Linux. It is a pretty straight-forward single-threaded test, so if it is going to fail, I would expect it to fail altogether.

What does it look like when it fails?

Jim

Hi,

No I did not. Windows/Mac/FreeBSD is not tested or known to even compile.
FreeBSD should be pretty safe (on par with linux) and and I’ve really tried be careful, and went through possibly all places where we exec/posix_spawn.
Windows is least of concern here, as long it compiles, but some sanity testing there would be nice too.

/Piotr

Hey Jim,

I’ll need to bring it up again. This morning I had to disable more than one test that failed intermittently, on items that look relatively significant to be failing.

I’ll put some eyes on it shortly here.

+lldb-dev I am sorry

Huh, that's failing doing "set a breakpoint and run to it". The set a breakpoint part works, I've already checked that, but we haven't done anything but "LaunchSimple" and we fail at "did I hit this breakpoint".

That's pretty bad, but I'm surprised you aren't seeing a similar failure from lots of tests...

Jim

My feeling was when I looked at it bit, that it is related to some thing I’ve seen about symbols earlier. In some cases we got some strange symbol (address pointing to location where it fails set breakpoint), but have not nailed it down to anything specific, and did not have time to dig deeper yet. It looks like edge case, rather like something totally broken though.

And indeed, this one doesn’t look very nice.