Ensure no RWX pages remain after mach_override_ptr

Ensure no RWX pages remain after mach_override_ptr

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

[Chris Evans, 2013-07-30 20:56:29]

You need to update third_party/mach_override/README.chromium

We should also send the modification upstream, but I support patching the local
copy until such time as a new upstream release fixes this (assuming it hasn't
already ;-)

For final review approval, you'll need someone from
third_party/mach_override/OWNERS (I see Avi is already listed).

[Avi (use Gerrit), 2013-07-30 20:58:14]

Mark was the one to do this work; throwing to him.

[Mark Mentovai, 2013-07-30 21:02:27]

Update README.chromium to reflect the local change. Also send this patch
upstream.

https://codereview.chromium.org/21208002/diff/1/third_party/mach_override/mac...
File third_party/mach_override/mach_override.c (right):

https://codereview.chromium.org/21208002/diff/1/third_party/mach_override/mac...
third_party/mach_override/mach_override.c:340: (vm_address_t)
originalFunctionPtr, 8, false,
The indentation here is a mess.

[ianbeer, 2013-07-30 21:49:39]

On 2013/07/30 21:02:27, Mark Mentovai wrote:
> Update README.chromium to reflect the local change. Also send this patch
> upstream.

I will send the patch upstream.

>
https://codereview.chromium.org/21208002/diff/1/third_party/mach_override/mac...
> third_party/mach_override/mach_override.c:340: (vm_address_t)
> originalFunctionPtr, 8, false,
> The indentation here is a mess.

The indentation in the whole file is quite messy. I've tried to match it more
closely now.

[Mark Mentovai, 2013-07-30 22:01:53]

https://codereview.chromium.org/21208002/diff/14001/third_party/mach_override...
File third_party/mach_override/mach_override.c (right):

https://codereview.chromium.org/21208002/diff/14001/third_party/mach_override...
third_party/mach_override/mach_override.c:340: (vm_address_t)
originalFunctionPtr, 8, false,
Assigning back into “err” after rewriting the original function’s code means
that if an error does occur here, the cleanup code below will free the
newly-written jump target.

Error handling in here is already a bit of a mess but you shouldn’t introduce
this new logic problem.

[ianbeer, 2013-08-07 15:08:26]

> Assigning back into “err” after rewriting the original function’s code means
> that if an error does occur here, the cleanup code below will free the
> newly-written jump target.
> 
> Error handling in here is already a bit of a mess but you shouldn’t introduce
> this new logic problem.

Fixed. Added the same logging to stderr present in the rest of the code.

[Mark Mentovai, 2013-08-07 15:16:39]

LGTM

[commit-bot: I haz the power, 2013-08-07 19:07:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ianbeer@chromium.org/21208002/22001

[commit-bot: I haz the power, 2013-08-07 19:22:47]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-09 08:59:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ianbeer@chromium.org/21208002/22001

[commit-bot: I haz the power, 2013-08-09 12:34:53]

Change committed as 216665

[Mark Mentovai, 2013-11-06 02:57:12]

This hasn’t been upstreamed to https://github.com/rentzsch/mach_override yet.
Please upstream it.

[ianbeer_google.com, 2013-11-06 03:10:30]

sorry about that, I'll get on it.


On Tue, Nov 5, 2013 at 6:57 PM, <mark@chromium.org> wrote:

> This hasn’t been upstreamed to https://github.com/rentzsch/mach_overrideyet.
> Please upstream it.
>
> https://codereview.chromium.org/21208002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.