Add unit test to check for broker FD leak

Add unit test to check for broker FD leak

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

[mdempsky, 2014-04-09 01:13:42]

Reworked the test to use a new SANDBOX_TEST_ALLOW_NOISE() macro and documented
why we need that for RecvMsgDescriptorLeak.  PTAL! :)

[jln (very slow on Chromium), 2014-04-09 02:25:47]

I'm sorry if this seems nit-picky, but I've been too burned by flaky tests in
the past. I think we should figure out the number of FDs in use more reliably.

https://codereview.chromium.org/229893002/diff/1/sandbox/linux/services/broke...
File sandbox/linux/services/broker_process.h (right):

https://codereview.chromium.org/229893002/diff/1/sandbox/linux/services/broke...
sandbox/linux/services/broker_process.h:63: int ipc_socketpair() const { return
ipc_socketpair_; }
Maybe add a comment saying that this should only be used by tests?

Another way to do it is to add it as protected, then subclass BrokerProcess as
BrokerProcessForTests and use that instead in the unit test.

https://codereview.chromium.org/229893002/diff/20001/sandbox/linux/services/b...
File sandbox/linux/services/broker_process_unittest.cc (right):

https://codereview.chromium.org/229893002/diff/20001/sandbox/linux/services/b...
sandbox/linux/services/broker_process_unittest.cc:442: const int min_fd =
dup(STDIN_FILENO);
Maybe just create a pipe or open cpuinfo instead? stdin could become problematic
if we do something funky to it in an extension of the test.

https://codereview.chromium.org/229893002/diff/20001/sandbox/linux/services/b...
sandbox/linux/services/broker_process_unittest.cc:442: const int min_fd =
dup(STDIN_FILENO);
While dup(2) will always give you the smallest fd available, it's not guaranteed
to equal the number of fds in use.

I don't think it's out of question that because of the complexity of the test
framework, a number of file descriptors get open and then closed before this
runs.

Unfortunately I think you may have no choice but count the number of entries in
/proc/self/fd/. You could also binary search this between 0 and
sysconf(OPEN_MAX) by trying to dup() a fd.

https://codereview.chromium.org/229893002/diff/20001/sandbox/linux/services/b...
sandbox/linux/services/broker_process_unittest.cc:446: // Lower our file
descriptor limit to just above the current limit so we can
You mean just above the current usage I think?

[jln (very slow on Chromium), 2014-04-09 02:32:09]

https://codereview.chromium.org/229893002/diff/20001/sandbox/linux/services/b...
File sandbox/linux/services/broker_process_unittest.cc (right):

https://codereview.chromium.org/229893002/diff/20001/sandbox/linux/services/b...
sandbox/linux/services/broker_process_unittest.cc:442: const int min_fd =
dup(STDIN_FILENO);
> You could also binary search this between 0 and
> sysconf(OPEN_MAX) by trying to dup() a fd.

I meant "by setting soft rlimit and trying to dup() a fd". Or you could just
start at 0 and increment since the number is almost certainly very small.

[mdempsky, 2014-04-09 23:29:40]

I reworked it a bit, and I think it should be way more robust.  It's not quite
what you suggested (I think simpler though), so let me know if you don't like
it.

Also, no worries about nitpicking. :)

https://codereview.chromium.org/229893002/diff/20001/sandbox/linux/services/b...
File sandbox/linux/services/broker_process_unittest.cc (right):

https://codereview.chromium.org/229893002/diff/20001/sandbox/linux/services/b...
sandbox/linux/services/broker_process_unittest.cc:442: const int min_fd =
dup(STDIN_FILENO);
On 2014/04/09 02:25:47, jln wrote:
> Maybe just create a pipe or open cpuinfo instead? stdin could become
problematic
> if we do something funky to it in an extension of the test.

Changed to use pipes.

https://codereview.chromium.org/229893002/diff/20001/sandbox/linux/services/b...
sandbox/linux/services/broker_process_unittest.cc:442: const int min_fd =
dup(STDIN_FILENO);
On 2014/04/09 02:25:47, jln wrote:
> While dup(2) will always give you the smallest fd available, it's not
guaranteed
> to equal the number of fds in use.

True.  I've reworked this to be more robust about the process having sparsely
allocated file descriptors.  Instead of using dup(0) and assuming the next
couple should be free too, it allocates two pipes and then closes them again.

I don't think I need to check for how many files are actually in use currently
because RLIMIT_NOFILE doesn't limit the number of open file descriptors a
process can have, it just limits the highest value of a new descriptor.  E.g.,
if I have 100 descriptors but none of them are <10, RLIMIT_NOFILE will still
allow me to allocate 10 more file descriptors.

So since there aren't any other threads, allocating three extra fds and then
setting RLIMIT_NOFILE to 1 greater than the largest of them ensures I'll still
be able to allocate those three.

I added a comment to help explain this, but let me know if you think more
details would be helpful.

https://codereview.chromium.org/229893002/diff/40001/sandbox/linux/services/b...
File sandbox/linux/services/broker_process.h (right):

https://codereview.chromium.org/229893002/diff/40001/sandbox/linux/services/b...
sandbox/linux/services/broker_process.h:102: friend void
TEST_RecvMsgDescriptorLeak(void*);
I considered making ipc_socketpair() into a protected method and subclassing
BrokerProcess like you suggested, but I thought this was much more concise and
avoided needing to make BrokerProcess's destructor virtual.

I'm not a fan of having to hard-code the test's macro generated name though (and
I couldn't find any precedent for it in Chromium), but at least it will be
caught at compile-time if things change. :/

[jln (very slow on Chromium), 2014-04-10 20:21:15]

https://chromiumcodereview.appspot.com/229893002/diff/60001/sandbox/linux/ser...
File sandbox/linux/services/broker_process_unittest.cc (right):

https://chromiumcodereview.appspot.com/229893002/diff/60001/sandbox/linux/ser...
sandbox/linux/services/broker_process_unittest.cc:449: // Find the four lowest
available file descriptors.
Replace with "Find four available file descriptors" instead? They're not
guaranteed to be the lowest.

https://chromiumcodereview.appspot.com/229893002/diff/60001/sandbox/linux/ser...
sandbox/linux/services/broker_process_unittest.cc:455: for (size_t i = 1; i <
arraysize(available_fds); i++) {
Didn't you forget to close available_fds[0]?

Make it a ScopedFD maybe?

https://chromiumcodereview.appspot.com/229893002/diff/60001/sandbox/linux/ser...
sandbox/linux/services/broker_process_unittest.cc:463: const rlim_t fd_limit =
kFdLimit

https://chromiumcodereview.appspot.com/229893002/diff/60001/sandbox/linux/ser...
sandbox/linux/services/broker_process_unittest.cc:483: // The broker process
should only have a couple spare file descriptors
Sending kFdLimit descriptors is guaranteed to work, but I don't think you can
guarantee that only a couple spare fd are available.

[mdempsky, 2014-04-10 20:35:50]

https://codereview.chromium.org/229893002/diff/60001/sandbox/linux/services/b...
File sandbox/linux/services/broker_process_unittest.cc (right):

https://codereview.chromium.org/229893002/diff/60001/sandbox/linux/services/b...
sandbox/linux/services/broker_process_unittest.cc:449: // Find the four lowest
available file descriptors.
On 2014/04/10 20:21:15, jln wrote:
> Replace with "Find four available file descriptors" instead? They're not
> guaranteed to be the lowest.

As discussed, POSIX requires pipe() to return the two lowest file descriptors
(http://pubs.opengroup.org/onlinepubs/9699919799/functions/pipe.html), so I left
the comment as is.

https://codereview.chromium.org/229893002/diff/60001/sandbox/linux/services/b...
sandbox/linux/services/broker_process_unittest.cc:455: for (size_t i = 1; i <
arraysize(available_fds); i++) {
On 2014/04/10 20:21:15, jln wrote:
> Didn't you forget to close available_fds[0]?
> 
> Make it a ScopedFD maybe?

Done.

https://codereview.chromium.org/229893002/diff/60001/sandbox/linux/services/b...
sandbox/linux/services/broker_process_unittest.cc:463: const rlim_t fd_limit =
On 2014/04/10 20:21:15, jln wrote:
> kFdLimit

As discussed, this isn't a "compile-time constant", so I think kNaming it would
be misleading.

https://codereview.chromium.org/229893002/diff/60001/sandbox/linux/services/b...
sandbox/linux/services/broker_process_unittest.cc:483: // The broker process
should only have a couple spare file descriptors
On 2014/04/10 20:21:15, jln wrote:
> Sending kFdLimit descriptors is guaranteed to work, but I don't think you can
> guarantee that only a couple spare fd are available.

Yep, that's what I was trying to convey with the comment: because of the earlier
setrlimit logic, the broker "should" only have a couple spare fds, but instead
of relying on that I just guarantee that we'd flood the descriptor table.  Let
me know if you think I should revise the comment.

[jln (very slow on Chromium), 2014-04-10 20:44:46]

lgtm, thanks Matthew!

[mdempsky, 2014-04-10 20:47:27]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-04-10 20:47:37]

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

[commit-bot: I haz the power, 2014-04-11 00:44:21]

Change committed as 263147