This is an archive of the discontinued LLVM Phabricator instance.

Redirect stdin, stdout and stderr to /dev/null when launching LLGS process.
ClosedPublic

Authored by ovyalov on Nov 3 2014, 5:01 PM.

Details

Summary

Closing std handles (but without redirecting them) allows kernel to "reuse" 0,1, 2 file handle ids for subsequent file handle open operations.
This issue may lead to junk data sent over TCP connection from lldb-gdbserver to lldb because stderr file handle might be assigned to the socket handle:

int fd = connection_up->GetReadObject()->GetWaitableHandle();
assert(fd == STDERR_FILENO); // llgs doesn't crash on such assertion without this code change in.

By redirecting std handles to /dev/null we prevent OS from such std handles reuse.

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 15742.Nov 3 2014, 5:01 PM
ovyalov retitled this revision from to Redirect stdin, stdout and stderr to /dev/null when launching LLGS process..
ovyalov updated this object.
ovyalov edited the test plan for this revision. (Show Details)
ovyalov added reviewers: apepper, sbest, clayborg.
ovyalov added a subscriber: Unknown Object (MLST).
clayborg requested changes to this revision.Nov 3 2014, 5:20 PM
clayborg edited edge metadata.

Can we remove:

launch_info.AppendCloseFileAction (STDIN_FILENO);
launch_info.AppendCloseFileAction (STDOUT_FILENO);
launch_info.AppendCloseFileAction (STDERR_FILENO);

As they are not needed if you are suppressing immediately afterward?

This revision now requires changes to proceed.Nov 3 2014, 5:20 PM

I left close actions because I found such comment for posix_spawn_file_actions_addopen (http://linux.die.net/man/3/posix_spawn_file_actions_addopen):

"If fildes was already an open file descriptor, it shall be closed before the new file is opened." - my understanding that open without prior close in such situation may lead to handle leakage.

Thank you.

Greg, please let me know whether it's okay to leave combination of close and suppress file actions here.

Thank you.

clayborg accepted this revision.Nov 4 2014, 3:09 PM
clayborg edited edge metadata.

It is ok as long as the test suite on MacOSX is still happy with it.

This revision is now accepted and ready to land.Nov 4 2014, 3:09 PM

I ran OSX test suite on my side w/ and w/o the patch - no regressions noticed.

Thank you!

Shawn, if the CL looks good to you could you submit it?

Thanks.

Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in r221324.