dbus: revamp fd passing for i/o restrictions

dbus: revamp fd passing support for i/o restrictions
    
Encapsulate file descriptor validity checking and status in the companion FileDescriptor class so callers can do descriptor checking in a context where i/o is allowed.
    
Update the debug daemon client support to validate the pipe descriptors in a worker thread so it is not done on the UI thread.

BUG=126142
TEST=new unit tests + collect trace data on chrome os and verify no assert is triggered

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136331

[Jorge Lucangeli Obes, 2012-05-04 22:31:10]

lgtm

[satorux1, 2012-05-04 22:31:22]

If it's not too much work, I think it'd be nicer to fix this problem rather than
working around by ThreadRestrictions::ScopedAllowIO

http://codereview.chromium.org/10382021/diff/1/dbus/message.cc
File dbus/message.cc (right):

http://codereview.chromium.org/10382021/diff/1/dbus/message.cc#newcode45
dbus/message.cc:45: bool prev = base::ThreadRestrictions::SetIOAllowed(true);
ThreadRestrictions::ScopedAllowIO would be simpler.

http://codereview.chromium.org/10382021/diff/1/dbus/message.cc#newcode707
dbus/message.cc:707: if (!CheckFileDescriptor(fd)) {
Maybe this wasn't the right place to validate this. What about instead letting
the caller to to validate this, and we check it here:

if (!value.is_validated())
  LOG(FATAL) << "The caller should validate before passing a file descriptor";

[Sam Leffler, 2012-05-04 22:38:17]

On 2012/05/04 22:31:22, satorux1 wrote:
> If it's not too much work, I think it'd be nicer to fix this problem rather
than
> working around by ThreadRestrictions::ScopedAllowIO

This is the result of trying to fix the problem correctly (and subsequent
discussions).  To satisfy the i/o restriction we must bounce the fstat request
to the File thread but the request/check is made in dbus and the caller expects
the message to be ready to send on return so we making this transparent appears
hard.  If you have a suggestion how to do this (or similar) would appreciate it.

[Sam Leffler, 2012-05-04 22:45:42]

On 2012/05/04 22:38:17, Sam Leffler wrote:
> On 2012/05/04 22:31:22, satorux1 wrote:
> > If it's not too much work, I think it'd be nicer to fix this problem rather
> than
> > working around by ThreadRestrictions::ScopedAllowIO
> 
> This is the result of trying to fix the problem correctly (and subsequent
> discussions).  To satisfy the i/o restriction we must bounce the fstat request
> to the File thread but the request/check is made in dbus and the caller
expects
> the message to be ready to send on return so we making this transparent
appears
> hard.  If you have a suggestion how to do this (or similar) would appreciate
it.

Think I see what you're suggesting; let me try that approach.

[Sam Leffler, 2012-05-08 17:52:43]

PTAL; no more hacks thanks to satoru's excellent suggestions

[Jorge Lucangeli Obes, 2012-05-08 18:42:22]

lgtm

nit: the CL description is outdated now that the solution is conceptually
correct and not a hack.

[Sam Leffler, 2012-05-08 18:54:41]

fixed, thank you

[Sam Leffler, 2012-05-08 19:05:44]

Hold off on this cl; just noticed there more to do (fd usage in GetDebugLogs)

[Sam Leffler, 2012-05-08 22:24:04]

PTAL

[satorux1, 2012-05-09 22:12:09]

LGTM, but please change CHECK to DCHECK before checking this in. Other comments
are style nits.

http://codereview.chromium.org/10382021/diff/6007/chromeos/dbus/debug_daemon_...
File chromeos/dbus/debug_daemon_client.cc (right):

http://codereview.chromium.org/10382021/diff/6007/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:157: dbus::FileDescriptor *file_descriptor
= new dbus::FileDescriptor(file);
nit: in Chrome, we usually add * to a type than a variable:

dbus::FileDescriptor* file_descriptor

http://codereview.chromium.org/10382021/diff/6007/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:293: callback) {
nit: the following is probably a bit more common in Chrome:

void OnCheckValidityRequestStopSystem(
    dbus::FileDescriptor* file_descriptor,
    const StopSystemTracingCallback& callback) {

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc
File dbus/file_descriptor.cc (right):

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:18: CHECK(valid_);
We usually use DCHECK(). IIRC, CHECK is used only when we want to catch a crash
in shipping binaries, which we rarely do.

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:24: CHECK(valid_);  // NB: check first so owner_ is
unchanged if this triggers
ditto.

[Jorge Lucangeli Obes, 2012-05-09 23:08:39]

lgtm

[satorux1, 2012-05-10 07:16:56]

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc
File dbus/file_descriptor.cc (right):

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:16: // Retrieves value as an int without affecting
ownership.
the comment looks identical to the one in .h, if so, please remove.

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:18: CHECK(valid_);
On 2012/05/09 22:12:09, satorux1 wrote:
> We usually use DCHECK(). IIRC, CHECK is used only when we want to catch a
crash
> in shipping binaries, which we rarely do.

I think I was wrong. If valid_ is false, this is security violation, so so
crashing the browser by CHECK() would make more sense than DCHECK.

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:22: // Takes the value and ownership.
the comment looks identical to the one in .h, if so, please remove.

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:31: // be made on the File thread.
move this comment to .h file? The users of this function want to know about the
information.

on the File thread -> something like "on a thread with file IO allowed"

[Sam Leffler, 2012-05-10 15:59:31]

pushing

http://codereview.chromium.org/10382021/diff/6007/chromeos/dbus/debug_daemon_...
File chromeos/dbus/debug_daemon_client.cc (right):

http://codereview.chromium.org/10382021/diff/6007/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:157: dbus::FileDescriptor *file_descriptor
= new dbus::FileDescriptor(file);
On 2012/05/09 22:12:09, satorux1 wrote:
> nit: in Chrome, we usually add * to a type than a variable:
> 
> dbus::FileDescriptor* file_descriptor

Done.

http://codereview.chromium.org/10382021/diff/6007/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:293: callback) {
On 2012/05/09 22:12:09, satorux1 wrote:
> nit: the following is probably a bit more common in Chrome:
> 
> void OnCheckValidityRequestStopSystem(
>     dbus::FileDescriptor* file_descriptor,
>     const StopSystemTracingCallback& callback) {

Done.

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc
File dbus/file_descriptor.cc (right):

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:16: // Retrieves value as an int without affecting
ownership.
On 2012/05/10 07:16:56, satorux1 wrote:
> the comment looks identical to the one in .h, if so, please remove.

Done.

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:18: CHECK(valid_);
Correct, this preserves the previous behaviour that generated an assert check in
release binaries due to potential security violation.

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:22: // Takes the value and ownership.
On 2012/05/10 07:16:56, satorux1 wrote:
> the comment looks identical to the one in .h, if so, please remove.

Done.

http://codereview.chromium.org/10382021/diff/6007/dbus/file_descriptor.cc#new...
dbus/file_descriptor.cc:31: // be made on the File thread.
On 2012/05/10 07:16:56, satorux1 wrote:
> move this comment to .h file? The users of this function want to know about
the
> information.
> 
> on the File thread -> something like "on a thread with file IO allowed"

Done.

[commit-bot: I haz the power, 2012-05-10 15:59:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sleffler@chromium.org/10382021/3003

[commit-bot: I haz the power, 2012-05-10 17:41:15]

Change committed as 136331