Change UnixDomainSocket::RecvMsg to return ScopedVector<base::ScopedFD>

Change UnixDomainSocket::RecvMsg to return ScopedVector<base::ScopedFD>

This is slightly suboptimal because ScopedVector forces each ScopedFD
to be individually heap allocated, but it's the simplest solution
until C++11 is available.

BUG=360274
NOTRY=true

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

[mdempsky, 2014-04-24 23:53:27]

After getting over my distaste of heap allocating individual 'int' objects,
using ScopedVector<base::ScopedFD> doesn't seem to be too bad.  Please take a
look and let me know if you think this CL looks stylistically appropriate for
Chromium, and I'll then find OWNERS to approve the individual parts.

The file I'm most unsure about stylistically is render_sandbox_host_linux.cc,
because it involves passing around "const std::vector<base::ScopedFD*>&"
references.  The others are relatively straight forward (aside from the
complicated logic about when to close/release descriptors around a fork() call).

[mdempsky, 2014-04-25 00:34:23]

On 2014/04/24 23:53:27, mdempsky wrote:
> The file I'm most unsure about stylistically is render_sandbox_host_linux.cc,

[FYI: as of patch set 4, this code now resides in sandbox_ipc_linux.{h,cc}
instead.]

[mdempsky, 2014-04-28 18:32:11]

brettw/awong: Ping?  Would either of you mind reviewing for overall style and
then approving for base?  (jln can approve for the content and sandbox changes,
and I'll ask mseaborn or someone to approve for components/nacl.)

[awong, 2014-04-28 18:48:16]

I can look at style, but doing a correctness review is slightly beyond me. Is
someone else doing that?

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:138: fds->reserve(wire_fds_len);
Is resize() or reserve() appropriate here? Is it possible for fds to be too
large initially?

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:177: base::ScopedFD
send_sock(raw_socks[1]);
Why isn't send_sock just inside the scope below?

[brettw, 2014-04-28 19:53:43]

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:177: base::ScopedFD
send_sock(raw_socks[1]);
On 2014/04/28 18:48:16, awong wrote:
> Why isn't send_sock just inside the scope below?

Actually, is there a reason for the scope below? Any why do we need to close the
send_sock then instead of when the function exits? I think the old code did this
because it had to do it explicitly and doing it later meant more work.

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:182: if (!SendMsg(fd, request.data(),
request.size(), send_fds)) {
No {}

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:204: if (result_fd) {
No {}

[mdempsky, 2014-04-28 20:16:58]

On 2014/04/28 18:48:16, awong wrote:
> I can look at style, but doing a correctness review is slightly beyond me. Is
> someone else doing that?

I'll volunteer jln to do a correctness review. :)

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:138: fds->reserve(wire_fds_len);
On 2014/04/28 18:48:16, awong wrote:
> Is resize() or reserve() appropriate here? Is it possible for fds to be too
> large initially?

We call fds->clear() at the very beginning, so the size should be zero.  We need
to use reserve() now instead of resize() because my CL changes the code to use
push_back() instead of just memcpy()'ing directly into the allocated memory.

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:177: base::ScopedFD
send_sock(raw_socks[1]);
On 2014/04/28 18:48:16, awong wrote:
> Why isn't send_sock just inside the scope below?

It could be, but I feel like it's less error-prone to create the base::ScopedFD
as close to the FD allocation as possible.  E.g., I don't want someone to
accidentally put in some code between here and the scope below that 'return's,
and then we leak this descriptor.

That said, I can move it below if you prefer.

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:177: base::ScopedFD
send_sock(raw_socks[1]);
On 2014/04/28 19:53:44, brettw wrote:
> Actually, is there a reason for the scope below?

Not a particularly good one, but it allows send_fds to free up its memory after
we don't need it anymore.  (Also, since it contains a bare FD that will no
longer be valid once send_sock is reset, it seemed like best practice to make
sure it's inaccessible sooner.)

> Any why do we need to close the
> send_sock then instead of when the function exits? I think the old code did
this
> because it had to do it explicitly and doing it later meant more work.

We need to close the sending end of the socket in our process, otherwise if the
peer process dies before sending us a response, we'll hang in RecvMsgWithFlags()
instead of seeing EOF.  I've added a comment to explain this.

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:182: if (!SendMsg(fd, request.data(),
request.size(), send_fds)) {
On 2014/04/28 19:53:44, brettw wrote:
> No {}

Done.

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:204: if (result_fd) {
On 2014/04/28 19:53:44, brettw wrote:
> No {}

Done.

[awong, 2014-04-28 20:25:17]

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:138: fds->reserve(wire_fds_len);
On 2014/04/28 20:16:58, mdempsky wrote:
> On 2014/04/28 18:48:16, awong wrote:
> > Is resize() or reserve() appropriate here? Is it possible for fds to be too
> > large initially?
> 
> We call fds->clear() at the very beginning, so the size should be zero.  We
need
> to use reserve() now instead of resize() because my CL changes the code to use
> push_back() instead of just memcpy()'ing directly into the allocated memory.

Can we CHECK/DCHECK that fds->empty() then? This makes me nervous about someone
in the future messing with the vector between the clear and this block of code.

For that matter...is this premature opt?

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:177: base::ScopedFD
send_sock(raw_socks[1]);
Yeah...seems good enough. If I was being 100% strict ivory tower, I'd tell you
to pull socketpair() out into a little helper function that only returned 2
ScopedFD via output parameter such that raw_socks would never be in this scope.

Actually...given the amount of lifetime management you are doing, that's
probably worth the extra static function.

On 2014/04/28 20:16:58, mdempsky wrote:
> On 2014/04/28 18:48:16, awong wrote:
> > Why isn't send_sock just inside the scope below?
> 
> It could be, but I feel like it's less error-prone to create the
base::ScopedFD
> as close to the FD allocation as possible.  E.g., I don't want someone to
> accidentally put in some code between here and the scope below that 'return's,
> and then we leak this descriptor.
> 
> That said, I can move it below if you prefer.

[mdempsky, 2014-04-28 21:11:53]

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:138: fds->reserve(wire_fds_len);
On 2014/04/28 20:25:18, awong wrote:
> Can we CHECK/DCHECK that fds->empty() then? This makes me nervous about
someone
> in the future messing with the vector between the clear and this block of
code.

I added a DCHECK(fds->empty()).  The structure of the code is such that I think
it's unlikely to need more sanity checks than that.

> For that matter...is this premature opt?

Maybe?  I just added it to mimic the existing code structure.  I don't feel
strongly about it, and don't mind removing it if you think it's bad practice.

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:177: base::ScopedFD
send_sock(raw_socks[1]);
On 2014/04/28 20:25:18, awong wrote:
> Yeah...seems good enough. If I was being 100% strict ivory tower, I'd tell you
> to pull socketpair() out into a little helper function that only returned 2
> ScopedFD via output parameter such that raw_socks would never be in this
scope.
> 
> Actually...given the amount of lifetime management you are doing, that's
> probably worth the extra static function.

Acknowledged, but I'd like to tackle this in a followup CL if you don't mind,
because I'd like this to be a public API that I can use in other files too.

[awong, 2014-04-28 21:22:28]

LGTM from my side with 1 nits and 1 random comment.

Passing off to jln for correctness review and brettw for any further base style
comments.

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/258543006/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:138: fds->reserve(wire_fds_len);
On 2014/04/28 21:11:54, mdempsky wrote:
> On 2014/04/28 20:25:18, awong wrote:
> > Can we CHECK/DCHECK that fds->empty() then? This makes me nervous about
> someone
> > in the future messing with the vector between the clear and this block of
> code.
> 
> I added a DCHECK(fds->empty()).  The structure of the code is such that I
think
> it's unlikely to need more sanity checks than that.
> 
> > For that matter...is this premature opt?
> 
> Maybe?  I just added it to mimic the existing code structure.  I don't feel
> strongly about it, and don't mind removing it if you think it's bad practice.

I think the resize() was just to guarantee the memcpy could work. I call
premature opt on this, but also don't care enough now that you have the DCHECK.

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

https://codereview.chromium.org/258543006/diff/130001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.cc:166: static bool
CreateSocketPair(base::ScopedFD* one, base::ScopedFD* two) {
nit: move to top of the file.

[mdempsky, 2014-04-28 21:24:43]

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

https://codereview.chromium.org/258543006/diff/130001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.cc:166: static bool
CreateSocketPair(base::ScopedFD* one, base::ScopedFD* two) {
On 2014/04/28 21:22:28, awong wrote:
> nit: move to top of the file.

Done.

[brettw, 2014-04-28 21:57:01]

lgtm

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

https://codereview.chromium.org/258543006/diff/170001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.cc:150: DCHECK(fds->empty());
Weighing in on this, I think we should remove both the check and the reserve.

[mdempsky, 2014-04-28 22:01:52]

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

https://codereview.chromium.org/258543006/diff/170001/base/posix/unix_domain_...
base/posix/unix_domain_socket_linux.cc:150: DCHECK(fds->empty());
On 2014/04/28 21:57:02, brettw wrote:
> Weighing in on this, I think we should remove both the check and the reserve.

Done.

[brettw, 2014-04-28 22:20:06]

base lgtm

[mdempsky, 2014-04-28 23:12:43]

awong/brettw: Thanks for the review!

jln: Please review content/ and sandbox/ for OWNERS approval.  Also, please
review base/ for correctness.

mseaborn: Please review components/nacl/ for OWNERS approval.

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

Looks good in general, but I think there is a small leak in NaCl.

https://chromiumcodereview.appspot.com/258543006/diff/190001/components/nacl/...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://chromiumcodereview.appspot.com/258543006/diff/190001/components/nacl/...
components/nacl/loader/nacl_helper_linux.cc:196: 
We can't rely on the ScopedVector to release
child_fds[content::ZygoteForkDelegate::kDummyFDIndex] as this function will
never return.

More importantly, this is also true for kParentFDIndex.

Please reset() these FDs explicitly.

https://chromiumcodereview.appspot.com/258543006/diff/190001/content/zygote/z...
File content/zygote/zygote_linux.cc (right):

https://chromiumcodereview.appspot.com/258543006/diff/190001/content/zygote/z...
content/zygote/zygote_linux.cc:489: close(kZygoteSocketPairFd);  // Our socket
from the browser.
This is not passed in fds I suppose? Do you mind adding a PCHECK(0 ==
IGNORE_EINTR()) as a tag-along fix? (Or feel free to defer to another CL).

https://chromiumcodereview.appspot.com/258543006/diff/190001/content/zygote/z...
content/zygote/zygote_linux.cc:492: for (ScopedVector<base::ScopedFD>::iterator
i = fds.begin(); i != fds.end();
Wouldn't it be cleaner to iterate on "mapping" instead?

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

https://chromiumcodereview.appspot.com/258543006/diff/190001/components/nacl/...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://chromiumcodereview.appspot.com/258543006/diff/190001/components/nacl/...
components/nacl/loader/nacl_helper_linux.cc:199: child_fds.clear();
I had missed this one, so there is no leak.

[mdempsky, 2014-04-29 00:10:33]

https://codereview.chromium.org/258543006/diff/190001/components/nacl/loader/...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/258543006/diff/190001/components/nacl/loader/...
components/nacl/loader/nacl_helper_linux.cc:196: 
On 2014/04/28 23:48:55, jln wrote:
> Please reset() these FDs explicitly.

As we discussed and you pointed out below, child_fds.clear() already frees the
other descriptors.

https://codereview.chromium.org/258543006/diff/190001/content/zygote/zygote_l...
File content/zygote/zygote_linux.cc (right):

https://codereview.chromium.org/258543006/diff/190001/content/zygote/zygote_l...
content/zygote/zygote_linux.cc:489: close(kZygoteSocketPairFd);  // Our socket
from the browser.
On 2014/04/28 23:48:55, jln wrote:
> This is not passed in fds I suppose?

Correct.  There's no ScopedFD that owns this socket to my knowledge, so we still
need to manually close it. =/

> Do you mind adding a PCHECK(0 == IGNORE_EINTR()) as a
> tag-along fix?

Done.

https://codereview.chromium.org/258543006/diff/190001/content/zygote/zygote_l...
content/zygote/zygote_linux.cc:492: for (ScopedVector<base::ScopedFD>::iterator
i = fds.begin(); i != fds.end();
On 2014/04/28 23:48:55, jln wrote:
> Wouldn't it be cleaner to iterate on "mapping" instead?

Probably, but as discussed, |mapping| just has the bare file descriptors, and
there's no clean way to correlate the elements of |mapping| back to |fds|, so
this is the best option currently available I think. :(

[jln (very slow on Chromium), 2014-04-29 00:16:52]

lgtm, thanks for this nice cleanup!

[Mark Seaborn, 2014-04-30 18:55:44]

LGTM for components/nacl

[mdempsky, 2014-04-30 19:03:37]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-04-30 19:04:26]

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

[mdempsky, 2014-04-30 21:53:51]

The CQ bit was unchecked by mdempsky@chromium.org

[mdempsky, 2014-04-30 21:53:57]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-04-30 21:55:30]

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

[commit-bot: I haz the power, 2014-04-30 21:57:12]

Change committed as 267350