[Mac] Fix ProcessUtilTest.FDRemapping for 10.9 and later.

[Mac] Fix ProcessUtilTest.FDRemapping for 10.9 and later.

OSX 10.9 introduced a kernel facility which allows tagging file
descriptors to prevent various operations unless the caller knows the
key.  libdispatch uses this facility to tag a file descriptor against
close() or dup(), and this test does both.  Unfortunately, the failure
case is to raise a kernel exception and hang the child process rather
than return an error code.

This change addresses this by attempting to set a guard.  If successful,
the guard is removed and the caller can close the file descriptor as
normal.  If unsuccessful, either the file descriptor is not valid, or it
is guarded and cannot be closed.  Either way, it is skipped.  The test
still succeeds by counting the file descriptors which can be closed.

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

[Scott Hess - ex-Googler, 2014-07-24 17:58:17]

Another option which works is to check the fstat() results for FIFO, which will
be true for this injected fd and kChildPipe.

Per #8 on the bug, I think this is spinning up as part of NSApp initialization,
since the main thread is clearly touching libdispatch calls when the guarding
thread is doing setup.  That might be incorrect, so it is possible that there is
some means by which we could prevent libdispatch from spinning up the subthread
for this subprocess.

I am nervous about whether this can affect us in the wild, but AFAICT the cases
we see in the wild are probably people closing the wrong file descriptor.  We
should probably handle that case in some way (other than hanging), but as far as
I can tell from code-reading, there isn't a clean way to handle that which will
allow this code to work correctly.  Once the guard fires the process is burned,
one way or another.  [Dirty way would be to use a separate thread to provoke the
exceptions, but to hang in the handler, which I think will allow some other
thread to proceed to EPERM.  Heinous.]

[Mark Mentovai, 2014-07-25 13:23:44]

I guess this is OK…in a test…

https://codereview.chromium.org/414503005/diff/1/base/process/process_util_un...
File base/process/process_util_unittest.cc (right):

https://codereview.chromium.org/414503005/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:514: bool is_fd_unguarded(int fd) {
Naming.

https://codereview.chromium.org/414503005/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:520: int original_fdflags = 0;
The kernel calls these unsigned ints, might as well do the same.

https://codereview.chromium.org/414503005/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:528: int ret =
HANDLE_EINTR(my_change_fdguard_np(
I don’t think HANDLE_EINTR is really needed around the guard operations.

https://codereview.chromium.org/414503005/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:554: if (!is_fd_unguarded(i))
This is kind of a double negative, and considering that this is the only place
the function is called, it makes sense to negate the name itself.

[Scott Hess - ex-Googler, 2014-07-28 19:58:03]

On 2014/07/25 13:23:44, Mark Mentovai wrote:
> I guess this is OK…in a test…

Not really, but disabling seems less OK...

https://codereview.chromium.org/414503005/diff/1/base/process/process_util_un...
File base/process/process_util_unittest.cc (right):

https://codereview.chromium.org/414503005/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:514: bool is_fd_unguarded(int fd) {
On 2014/07/25 13:23:43, Mark Mentovai wrote:
> Naming.

Done.

I also changed my_guard_fdguard_np -> guard_fdguard_np because it really doesn't
need a real namespace _and_ namespace-by-convention.

https://codereview.chromium.org/414503005/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:520: int original_fdflags = 0;
On 2014/07/25 13:23:43, Mark Mentovai wrote:
> The kernel calls these unsigned ints, might as well do the same.

The guard flags are unsigned, but the fdflags are signed:

extern int change_fdguard_np(int fd, const guardid_t *guard, u_int guardflags,
	const guardid_t *nguard, u_int nguardflags, int *fdflagsp);

https://codereview.chromium.org/414503005/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:528: int ret =
HANDLE_EINTR(my_change_fdguard_np(
On 2014/07/25 13:23:43, Mark Mentovai wrote:
> I don’t think HANDLE_EINTR is really needed around the guard operations.

Done.

https://codereview.chromium.org/414503005/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:554: if (!is_fd_unguarded(i))
On 2014/07/25 13:23:43, Mark Mentovai wrote:
> This is kind of a double negative, and considering that this is the only place
> the function is called, it makes sense to negate the name itself.

Changed to CanGuardFd().  The reason for this sense of test is because if an
undocumented syscall fails, it's unclear whether a strong assertion can be made
to the specific reasons for the failure, so I want to act on the positive case
only.

I could rewrite the test below to account for this case, since EBADF is already
handled, but leaving the cross-platform code alone seems better.

[Mark Mentovai, 2014-07-28 20:15:03]

LGTM

Well, it’s not so much that it looks “good,” but you get the idea…

[Scott Hess - ex-Googler, 2014-07-28 20:15:54]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2014-07-28 20:35:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shess@chromium.org/414503005/20001

[commit-bot: I haz the power, 2014-07-29 00:20:17]

Change committed as 286037