[lldb-dev] [PATCH] Avoid leaking file descriptors.

[Piotr Rak]

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


2014-03-25 22:01 GMT+01:00 Todd Fiala <tfiala at google.com>:

> 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
>
>
> On Tue, Mar 25, 2014 at 1:43 PM, Todd Fiala <tfiala at google.com> wrote:
>
>> Looking at this now.
>>
>>
>> On Sat, Mar 22, 2014 at 8:43 AM, Piotr Rak <piotr.rak at gmail.com> wrote:
>>
>>> 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.
>>>
>>> _______________________________________________
>>> lldb-dev mailing list
>>> lldb-dev at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
>>>
>>>
>>
>>
>> --
>>  Todd Fiala | Software Engineer |  tfiala at google.com |  650-943-3180
>>
>
>
>
> --
> Todd Fiala | Software Engineer |  tfiala at google.com |  650-943-3180
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140325/810fb803/attachment.html>