This is an archive of the discontinued LLVM Phabricator instance.

Use socketpair on all Unix platforms
ClosedPublic

Authored by DemiMarie on May 15 2017, 2:25 PM.

Details

Summary

Using TCP sockets is insecure against local attackers, and possibly against remote attackers too (some vulnerabilities may allow tricking a browser to make a request to localhost). Use socketpair (which is immune to such attacks) on all Unix platforms.

Diff Detail

Repository
rL LLVM

Event Timeline

DemiMarie created this revision.May 15 2017, 2:25 PM
DemiMarie set the repository for this revision to rL LLVM.Jul 12 2017, 9:30 AM
DemiMarie removed a subscriber: lldb-commits.
DemiMarie added a subscriber: lldb-commits.
clayborg edited the summary of this revision. (Show Details)Jul 12 2017, 9:46 AM
clayborg added a reviewer: labath.
clayborg edited edge metadata.Jul 12 2017, 9:52 AM

Looks fine to me, lets make sure Pavel is OK with this. On MacOS socketpair is also much faster. Please run the following command while stopped at a breakpoint with and without your change:

(lldb) process plugin packet speed-test

It will send and receive packets using a variety of sizes and report the number of packets per second.

labath requested changes to this revision.Jul 12 2017, 9:57 AM

Have you tried that this actually works? (It doesn't work for me)

As far as I can tell, you will have to teach lldb-server to understand the --fd argument before you can flip this switch.

This revision now requires changes to proceed.Jul 12 2017, 9:57 AM

Ah yes, I thought there was something missing...

DemiMarie updated this revision to Diff 106708.Jul 14 2017, 2:17 PM
DemiMarie edited edge metadata.

Make --fd an alias for --pipe

Ah yes, I thought there was something missing...

I just made --fd an alias for --pipe.

clayborg accepted this revision.Jul 14 2017, 2:32 PM

Let Pavel try this and verify and this is good to go.

Any news on any speed changes? Please post the output of:

(lldb) process plugin packet speed-test

Both without and with this change. Nice to track any perf improvements

labath requested changes to this revision.Jul 18 2017, 2:38 AM

This is completely wrong. The --pipe argument is used to write the port that lldb-server listens on. Making --fd an alias to that will not help.

Please make sure you test your change before submitting it for review. You should at least make sure you are able to debug a hello world program with your modifications.

This revision now requires changes to proceed.Jul 18 2017, 2:38 AM
clayborg requested changes to this revision.Jul 18 2017, 8:21 AM

Before any changes are submitted, we assume you are getting a clean test suite run.

Before any changes are submitted, we assume you are getting a clean test suite run.

On my system, I get build failures in clang.

Before any changes are submitted, we assume you are getting a clean test suite run.

On my system, I get build failures in clang.

I suggest you write an email to cfe-dev http://lists.llvm.org/mailman/listinfo/cfe-dev and ask for help (or just google around). It's kinda hard to develop a feature without even being able to build the code :)

I am not sure how you expect to submit any patches then. Patches must be tested prior to submission.

DemiMarie updated this revision to Diff 107805.Jul 22 2017, 5:57 PM
DemiMarie edited edge metadata.

Fix connection when there is no host:port

DemiMarie updated this revision to Diff 107806.Jul 22 2017, 5:58 PM

Get rid of silly and bogus #error

clayborg requested changes to this revision.Jul 24 2017, 9:58 AM

Please init fd

tools/lldb-server/lldb-gdbserver.cpp
388 ↗(On Diff #107806)

This must be initialized to -1 since only the 'F' case will store to this. Pretty much all of the time random memory would be used for this and incorrectly try to use a random integer as a file descriptor.

This revision now requires changes to proceed.Jul 24 2017, 9:58 AM
DemiMarie updated this revision to Diff 108094.Jul 25 2017, 8:50 AM
DemiMarie edited edge metadata.

Initalize connection_fd

Otherwise undefined behavior ensues whenever --fd is not passed to
lldb-server -g.

So where did the other changes go where we use "--fd" for non Apple builds? Did those changes get lost? They will be needed.

Are you able to run the test suite now?

DemiMarie updated this revision to Diff 108214.Jul 25 2017, 9:37 PM

Actually use socketpair :)

DemiMarie added a comment.EditedJul 25 2017, 9:38 PM

So where did the other changes go where we use "--fd" for non Apple builds? Did those changes get lost? They will be needed.

Are you able to run the test suite now?

I did. Not all tests passed, but none of the failures appeared to be related to this change, and the compiled lldb can debug /usr/bin/cat.

clayborg accepted this revision.Jul 27 2017, 11:57 AM

I'm ok as long as Pavel is ok. Please wait for the OK from him as well.

I am not able to test this right now. Eugene, can you take a look?

tools/lldb-server/lldb-gdbserver.cpp
241 ↗(On Diff #108214)

you should set the O_CLOEXEC flag on the file descriptor, to avoid leakage.

DemiMarie updated this revision to Diff 108796.Jul 29 2017, 9:51 AM

Set FD_CLOEXEC on the communication fd

DemiMarie marked 2 inline comments as done.Jul 29 2017, 9:52 AM
eugene accepted this revision.Jul 31 2017, 9:32 PM

LGTM. Test run on Linux is clear. I also see a bit of perf bump.

Before checking in please remove outdated comment from ProcessGDBRemote::LaunchAndConnectToDebugserver.

// Use a socketpair on Apple for now until other platforms can verify it
// works and is fast enough
clayborg accepted this revision.Aug 7 2017, 7:54 AM

Remove the outdated comment and we are good to go.

DemiMarie updated this revision to Diff 110636.Aug 10 2017, 2:59 PM

Delete misleading comment

DemiMarie updated this revision to Diff 110642.Aug 10 2017, 3:45 PM

Fix misleading comment for real

Does this need any further action on my part?

clayborg accepted this revision.Sep 18 2017, 8:18 AM

This should be good to go. Watch the buildbots for failures.

Pavel & Eugene haven't marked it accepted yet, but from the comments is looks like they are both okay with the change. So it looks to me like everything is done but checking the code in.

If you have checkin privileges, then just check it in and as Greg says keep an eye on the bots. If you don't then speak up and one of us will check it in for you.

eugene accepted this revision.Sep 18 2017, 11:06 AM

I did mark it Accepted.

Pavel & Eugene haven't marked it accepted yet, but from the comments is looks like they are both okay with the change. So it looks to me like everything is done but checking the code in.

If you have checkin privileges, then just check it in and as Greg says keep an eye on the bots. If you don't then speak up and one of us will check it in for you.

I don’t have checkin privileges. Would one of you mind checking the code in?

This revision was automatically updated to reflect the committed changes.