Extend UnixDomainSocket::RecvMsg to return sender's PID

Extend UnixDomainSocket::RecvMsg to return sender's PID

BUG=357670
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266048

[mdempsky, 2014-04-18 01:11:30]

Here's my first stab at an API for this.  I've tested that the new tests work
when run as root on 3.2 (what I have readily available), but haven't had a
chance yet on a newer kernel.

[mdempsky, 2014-04-18 06:14:46]

Grr, so I've hit an impasse in trying to make use of this API safely, so I think
I'll need to go back to trying to use SCM_CREDENTIALS instead.

So hold off on reviewing this for now, because I need to redesign the API.

[mdempsky, 2014-04-18 23:23:25]

Updated the code to use SO_PASSCRED and SCM_CREDENTIALS instead of SO_PEERCRED. 
I'm not happy with splitting the unit tests like this, but it seems like the
most pragmatic fix at the moment.  (Long term, we should make SANDBOX_TEST
useful outside of sandbox_linux_unittests.)

[mdempsky, 2014-04-19 00:44:02]

(Also, followup CL is at https://codereview.chromium.org/240463005/.)

[mdempsky, 2014-04-23 21:47:07]

jln: Ping?  Any thoughts, or do you want to wait until I'm back in SFO tomorrow
to discuss?

[jln (very slow on Chromium), 2014-04-24 01:42:06]

On 2014/04/23 21:47:07, mdempsky wrote:
> jln: Ping?  Any thoughts, or do you want to wait until I'm back in SFO
tomorrow
> to discuss?

Sorry, still catching up. This will definitely be reviewed tomorrow!

[jln (very slow on Chromium), 2014-04-24 18:32:58]

Let's land the base/ change first and independently. I didn't review the
sandbox/ part yet, do you mind moving it to another CL?

lgtm for base/ changes.

https://chromiumcodereview.appspot.com/241863002/diff/140001/base/posix/unix_...
File base/posix/unix_domain_socket_linux.cc (right):

https://chromiumcodereview.appspot.com/241863002/diff/140001/base/posix/unix_...
base/posix/unix_domain_socket_linux.cc:109: const unsigned payload_len =
cmsg->cmsg_len - CMSG_LEN(0);
Ohh my, this looks like it's ripe for rewrite (in another CL).

https://chromiumcodereview.appspot.com/241863002/diff/140001/base/posix/unix_...
File base/posix/unix_domain_socket_linux.h (right):

https://chromiumcodereview.appspot.com/241863002/diff/140001/base/posix/unix_...
base/posix/unix_domain_socket_linux.h:23: static bool EnableReceiveProcessId(int
fd);
Perhaps make it clear that this should be used on the receiving end?

https://chromiumcodereview.appspot.com/241863002/diff/140001/base/posix/unix_...
base/posix/unix_domain_socket_linux.h:87: base::ProcessId* pid);
You didn't have to modify any caller? I was pretty sure RecvMsgWithFlags was
used (I think I added it).

https://chromiumcodereview.appspot.com/241863002/diff/140001/base/posix/unix_...
File base/posix/unix_domain_socket_linux_unittest.cc (right):

https://chromiumcodereview.appspot.com/241863002/diff/140001/base/posix/unix_...
base/posix/unix_domain_socket_linux_unittest.cc:81: // Additional tests are in
sandbox/linux/tests/unix_domain_socket_unittest.cc.
Remove this line, as base/ should not be tested by sandbox/

https://chromiumcodereview.appspot.com/241863002/diff/140001/base/posix/unix_...
base/posix/unix_domain_socket_linux_unittest.cc:94: char buf[sizeof(kHello) +
1];
Did you add the + 1 to be able to test that you only received sizeof(kHello)
bytes despite reading potentially more?

Maybe add a comment? As-is, one could think that you're adding space for a \0.

https://chromiumcodereview.appspot.com/241863002/diff/140001/sandbox/linux/te...
File sandbox/linux/tests/unix_domain_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/241863002/diff/140001/sandbox/linux/te...
sandbox/linux/tests/unix_domain_socket_unittest.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
No "(c)" in new files.

[mdempsky, 2014-04-24 19:00:34]

https://codereview.chromium.org/241863002/diff/140001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/241863002/diff/140001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.cc:109: const unsigned payload_len =
cmsg->cmsg_len - CMSG_LEN(0);
On 2014/04/24 18:32:58, jln wrote:
> Ohh my, this looks like it's ripe for rewrite (in another CL).

ACK

https://codereview.chromium.org/241863002/diff/140001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux.h (right):

https://codereview.chromium.org/241863002/diff/140001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.h:23: static bool EnableReceiveProcessId(int
fd);
On 2014/04/24 18:32:58, jln wrote:
> Perhaps make it clear that this should be used on the receiving end?

Done.

https://codereview.chromium.org/241863002/diff/140001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.h:87: base::ProcessId* pid);
On 2014/04/24 18:32:58, jln wrote:
> You didn't have to modify any caller? I was pretty sure RecvMsgWithFlags was
> used (I think I added it).

It has 'private' visibility, so it's only used within
unix_domain_socket_linux.cc, and I fixed up the one caller in there (in
SendRecvMsgWithFlags).

https://codereview.chromium.org/241863002/diff/140001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux_unittest.cc (right):

https://codereview.chromium.org/241863002/diff/140001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux_unittest.cc:81: // Additional tests are in
sandbox/linux/tests/unix_domain_socket_unittest.cc.
On 2014/04/24 18:32:58, jln wrote:
> Remove this line, as base/ should not be tested by sandbox/

Done.

https://codereview.chromium.org/241863002/diff/140001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux_unittest.cc:94: char buf[sizeof(kHello) +
1];
On 2014/04/24 18:32:58, jln wrote:
> Did you add the + 1 to be able to test that you only received sizeof(kHello)
> bytes despite reading potentially more?

Correct.

> Maybe add a comment? As-is, one could think that you're adding space for a \0.

Done.

https://codereview.chromium.org/241863002/diff/140001/sandbox/linux/tests/uni...
File sandbox/linux/tests/unix_domain_socket_unittest.cc (right):

https://codereview.chromium.org/241863002/diff/140001/sandbox/linux/tests/uni...
sandbox/linux/tests/unix_domain_socket_unittest.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/04/24 18:32:58, jln wrote:
> No "(c)" in new files.

Done.  (But this file will be moved to a separate CL.)

[jln (very slow on Chromium), 2014-04-24 19:23:55]

lgtm, this is ready for owner review.

[jln (very slow on Chromium), 2014-04-24 19:24:38]

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux.h (right):

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.h:13: #include
"base/process/process_handle.h"
You could forward declare base::ProcessId.

[mdempsky, 2014-04-24 20:23:30]

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux.h (right):

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.h:13: #include
"base/process/process_handle.h"
On 2014/04/24 19:24:38, jln wrote:
> You could forward declare base::ProcessId.

As discussed at lunch, base::ProcessId can't be forward declared because it's a
typedef, not a class.

[mdempsky, 2014-04-24 20:24:45]

willchan: PTAL for base OWNERS approval.

[willchan no longer on Chromium, 2014-04-24 20:44:04]

lgtm

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux.h (right):

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.h:13: #include
"base/process/process_handle.h"
On 2014/04/24 20:23:30, mdempsky wrote:
> On 2014/04/24 19:24:38, jln wrote:
> > You could forward declare base::ProcessId.
> 
> As discussed at lunch, base::ProcessId can't be forward declared because it's
a
> typedef, not a class.

I'm fine with including and not forward declaring, but since this is only be
added to unix_domain_socket_linux.h, why do we need the platform-agnostic type?
Does this code usage leak into platform-agnostic code?

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux_unittest.cc (right):

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux_unittest.cc:91: send_sock.get(), kHello,
sizeof(kHello), std::vector<int>()));
Generally we prefer arraysize to sizeof for arrays.

[mdempsky, 2014-04-24 20:59:13]

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux.h (right):

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.h:13: #include
"base/process/process_handle.h"
On 2014/04/24 20:44:04, willchan wrote:
> I'm fine with including and not forward declaring, but since this is only be
> added to unix_domain_socket_linux.h, why do we need the platform-agnostic
type?
> Does this code usage leak into platform-agnostic code?

I think it would be technically okay to use pid_t instead of base::ProcessId in
this API.  It just seemed like the predominant style of base/ was to still use
base::ProcessId instead of pid_t even for POSIX-specific functionality (e.g.,
base::GetParentProcessId).

Doesn't matter to me either way.  I'm happy to change it if you prefer.

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux_unittest.cc (right):

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux_unittest.cc:91: send_sock.get(), kHello,
sizeof(kHello), std::vector<int>()));
On 2014/04/24 20:44:04, willchan wrote:
> Generally we prefer arraysize to sizeof for arrays.

Noted, but the APIs used here take the number of bytes, not the number of array
elements.  In this case, they're the same because kHello is an array of chars
(and sizeof(char) == 1 by definition), but if we changed it to an array of
wchar_t, using arraysize() would be incorrect.

Do you still prefer that I change it to use arraysize() instead of sizeof()?

[willchan no longer on Chromium, 2014-04-24 21:09:56]

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux.h (right):

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.h:13: #include
"base/process/process_handle.h"
On 2014/04/24 20:59:14, mdempsky wrote:
> On 2014/04/24 20:44:04, willchan wrote:
> > I'm fine with including and not forward declaring, but since this is only be
> > added to unix_domain_socket_linux.h, why do we need the platform-agnostic
> type?
> > Does this code usage leak into platform-agnostic code?
> 
> I think it would be technically okay to use pid_t instead of base::ProcessId
in
> this API.  It just seemed like the predominant style of base/ was to still use
> base::ProcessId instead of pid_t even for POSIX-specific functionality (e.g.,
> base::GetParentProcessId).
> 
> Doesn't matter to me either way.  I'm happy to change it if you prefer.

I generally encourage using the cross-platform type to ease (reduce refactoring)
in future potential cross-platform usage except if there are costs. Avoiding an
#include is pretty minor, so I, like you, don't really care :) Just thought I'd
mention it in case one of you guys did.

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
File base/posix/unix_domain_socket_linux_unittest.cc (right):

https://codereview.chromium.org/241863002/diff/160001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux_unittest.cc:91: send_sock.get(), kHello,
sizeof(kHello), std::vector<int>()));
On 2014/04/24 20:59:14, mdempsky wrote:
> On 2014/04/24 20:44:04, willchan wrote:
> > Generally we prefer arraysize to sizeof for arrays.
> 
> Noted, but the APIs used here take the number of bytes, not the number of
array
> elements.  In this case, they're the same because kHello is an array of chars
> (and sizeof(char) == 1 by definition), but if we changed it to an array of
> wchar_t, using arraysize() would be incorrect.
> 
> Do you still prefer that I change it to use arraysize() instead of sizeof()?

Oops, I missed that. Fine as is.

[mdempsky, 2014-04-24 21:14:46]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:44:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mdempsky@chromium.org/241863002/160001

[commit-bot: I haz the power, 2014-04-24 23:42:46]

Change committed as 266048