Use the new ScopedMachVM class and the MACH_LOG family of logging macros

Use the new ScopedMachVM class and the MACH_LOG family of logging macros
where it makes sense to do so in existing code.

This migration shook out a couple of bugs in mach_logging.h carried over
from mac_logging.h (fixed there too).

R=rsesek@chromium.org, sky@chromium.org

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

[Mark Mentovai, 2014-05-09 19:06:20]

https://codereview.chromium.org/278923002/diff/60001/content/browser/mach_bro...
File content/browser/mach_broker_mac.mm (right):

https://codereview.chromium.org/278923002/diff/60001/content/browser/mach_bro...
content/browser/mach_broker_mac.mm:98: while ((kr = mach_msg(&msg.header,
options, 0, sizeof(msg), server_port_,
It occurs to me that this sucks. A compromised child can send an
intentionally-large message to this port to force this mach_msg to return
MACH_RCV_TOO_LARGE, which will terminate this loop and thread.

Do you know how the libdispatch-based Mach message receiver deals with this kind
of thing?

[Robert Sesek, 2014-05-09 20:40:00]

https://codereview.chromium.org/278923002/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory_mac.cc (right):

https://codereview.chromium.org/278923002/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_mac.cc:48: memory_.reset(address,
mach_vm_round_page(size_));
You don't want to round the size in the initializer list?

https://codereview.chromium.org/278923002/diff/60001/base/process/memory_mac.mm
File base/process/memory_mac.mm (right):

https://codereview.chromium.org/278923002/diff/60001/base/process/memory_mac....
base/process/memory_mac.mm:228: mach_port_deallocate(mach_task_self(), unused);
Technically this _can_ fail. But it's not likely.

https://codereview.chromium.org/278923002/diff/60001/content/browser/mach_bro...
File content/browser/mach_broker_mac.mm (right):

https://codereview.chromium.org/278923002/diff/60001/content/browser/mach_bro...
content/browser/mach_broker_mac.mm:98: while ((kr = mach_msg(&msg.header,
options, 0, sizeof(msg), server_port_,
On 2014/05/09 19:06:20, Mark Mentovai wrote:
> It occurs to me that this sucks. A compromised child can send an
> intentionally-large message to this port to force this mach_msg to return
> MACH_RCV_TOO_LARGE, which will terminate this loop and thread.
> 
> Do you know how the libdispatch-based Mach message receiver deals with this
kind
> of thing?

Switching this to libdispatch would be better. Dispatch just has an
EVFILT_MACHPORT and calls out to the event handler block. If the message failed
to be received, then returning out of the block would be fine and the server
would keep servicing its port.

I have an item on my todo list to switch this from a thread to a queue. Then it
can actually be joinable (if we want).

https://codereview.chromium.org/278923002/diff/60001/content/browser/mach_bro...
content/browser/mach_broker_mac.mm:100: KERN_SUCCESS) {
nit? should this be indented +4

[Mark Mentovai, 2014-05-09 21:09:34]

Maybe I should move bpoop to use the libdispatch handler too, except then I’d
have to find something for the main thread to do. Maybe not.

https://codereview.chromium.org/278923002/diff/60001/base/process/memory_mac.mm
File base/process/memory_mac.mm (right):

https://codereview.chromium.org/278923002/diff/60001/base/process/memory_mac....
base/process/memory_mac.mm:228: mach_port_deallocate(mach_task_self(), unused);
rsesek wrote:
> Technically this _can_ fail. But it's not likely.

Oh, maybe I should have said something here? The kernel doesn’t actually give
you back a port object when you call mach_vm_region. It always gives you a null
port. The deallocate is there to make the code not look like a leaky hog, but
it’s really a no-op.

https://codereview.chromium.org/278923002/diff/60001/content/browser/mach_bro...
File content/browser/mach_broker_mac.mm (right):

https://codereview.chromium.org/278923002/diff/60001/content/browser/mach_bro...
content/browser/mach_broker_mac.mm:100: KERN_SUCCESS) {
rsesek wrote:
> nit? should this be indented +4

I don’t think so, but I struggled with this too. I made it tall instead of wide,
how ’bout that?

[Robert Sesek, 2014-05-09 21:14:17]

LGTM

On 2014/05/09 21:09:34, Mark Mentovai wrote:
> Maybe I should move bpoop to use the libdispatch handler too, except then I’d
> have to find something for the main thread to do. Maybe not.

dispatch_main();

> 
>
https://codereview.chromium.org/278923002/diff/60001/base/process/memory_mac.mm
> File base/process/memory_mac.mm (right):
> 
>
https://codereview.chromium.org/278923002/diff/60001/base/process/memory_mac....
> base/process/memory_mac.mm:228: mach_port_deallocate(mach_task_self(),
unused);
> rsesek wrote:
> > Technically this _can_ fail. But it's not likely.
> 
> Oh, maybe I should have said something here? The kernel doesn’t actually give
> you back a port object when you call mach_vm_region. It always gives you a
null
> port. The deallocate is there to make the code not look like a leaky hog, but
> it’s really a no-op.

Ah, a comment may be helpful.


>
https://codereview.chromium.org/278923002/diff/60001/content/browser/mach_bro...
> File content/browser/mach_broker_mac.mm (right):
> 
>
https://codereview.chromium.org/278923002/diff/60001/content/browser/mach_bro...
> content/browser/mach_broker_mac.mm:100: KERN_SUCCESS) {
> rsesek wrote:
> > nit? should this be indented +4
> 
> I don’t think so, but I struggled with this too. I made it tall instead of
wide,
> how ’bout that?

LGTM

[Mark Mentovai, 2014-05-09 21:28:43]

I added a comment there and in process_metrics_mac.cc, the other spot with a
vm_region call.

[Mark Mentovai, 2014-05-09 22:08:44]

sky for content/browser

[sky, 2014-05-09 22:34:38]

LGTM

[Mark Mentovai, 2014-05-10 02:12:55]

Committed patchset #5 manually as r269483 (presubmit successful).

[Mark Mentovai, 2014-05-10 04:10:36]

This broke iOS and a few attempts at fixing it later, I just backed the whole
thing out at r269516. I’ll pick this up again next week.

[Mark Mentovai, 2014-05-10 19:14:02]

This one should be better on iOS. You can look at the diffs from patch set 6 to
see the changes.

I went with vm_deallocate everywhere, since mach_vm_deallocate (and the rest of
<mach/mach_vm.h>) isn’t available on iOS at all, and for solely dealing with
things in-process (which we’re doing, we use mach_task_self() everywhere) the
native-width types are fine. Probably won’t cause problems with bpoop either
because I don’t ever need to vm_deallocate anything in another task, but if I do
wind up needing a scoper that does mach_vm_deallocate there, I’ll find a way
(templates?) to work it back in.

I also had to un-exclude the new .cc files on iOS and hide the bootstrap stuff
on iOS.

Sucks that iOS isn’t in the default trybot suite for *mac* or *.mm files.

[Robert Sesek, 2014-05-12 15:47:05]

Diffs from PS6 to PS8 LGTM.

[Mark Mentovai, 2014-05-12 18:50:10]

Committed patchset #9 manually as r269829 (presubmit successful).