Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
264923003:
Initial implementation of the Mac Bootstrap Sandbox. (Closed)
|
Created:
6 years, 7 months ago by Robert Sesek Modified:
6 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInitial implementation of the Mac Bootstrap Sandbox.
This adds the implementation for the second-layer sandbox, with tests, but
it does not wire it up for use in Chrome.
Design document:
https://docs.google.com/a/chromium.org/document/d/108sr6gBxqdrnzVPsb_4_JbDyW1V4-DRQUC4R8YvM40M/edit
BUG=367863
TEST=Covered by unit tests, manually verified on 10.6.8, 10.7.5, 10.8.5, and 10.9.2.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269937
Patch Set 1 #
Total comments: 86
Patch Set 2 : Address comments #Patch Set 3 : Rebase for new Mach utilities #
Total comments: 27
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Initialization changes #
Total comments: 5
Patch Set 6 : OWNERS #Patch Set 7 : Rebase for sandbox_export.h #
Messages
Total messages: 22 (0 generated)
> Affected files (+1156, -9 lines): Ha!
https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... File sandbox/mac/bootstrap_sandbox.cc (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:22: TASK_BOOTSTRAP_PORT, sandbox->server_->server_port()); There’s a global extern mach_port_t named bootstrap_port that is the cached bootstrap port, and it works in much the same way as the mach_task_self_ global. Things use it to communicate with the bootstrap server without having to go through task_get_special_port. Since you’re changing the task bootstrap port here, you should strongly consider setting bootstrap_port to the same port. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:38: DCHECK(IsPolicyValid(policy)); You’re CHECKy elsewhere, why’d you land on DCHECK for policy registration? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:50: return false; Based on the implementation here, it appears that you don’t need PrepareToForkWithPolicy at all, you can just pass the policy ID to FinishedFork. This check is the only thing that can fail, but returning failure here isn’t that much better than treating an unknown policy ID as “reject everything.” I don’t see a massive advantage to checking for the policy before forking. Taking the same lock twice for each fork in the parent process sucks. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:80: sandboxed_processes_.erase(handle); You were very CHECKy about things not being in the map when you were inserting them. Do you want to be CHECKy that they’re in the map before erasing them? (You can get an iterator, CHECK it, and then erase using the iterator to avoid doing the same map lookup twice.) https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:95: } You can just say “else return NULL” here and not have to initialize policy_id to -1. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:108: mach_task_self(), TASK_BOOTSTRAP_PORT, &real_bootstrap_port_)); When you task_get_special_port, you need to mach_port_deallocate the port you get when you’re done with it. Here, that’d be in the destructor. ScopedMachPort? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbox.h File sandbox/mac/bootstrap_sandbox.h (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:42: // PrepareToForkWithPolicy() and FinishedFork() must be non-nested and balanced. An alternative would be to make PrepareToForkWithPolicy return some handle that the caller would need to supply to FinishedFork, to not limit the nestiness and balanciness of things. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:45: // Creates a new sandbox manager. Returns NULL on failure. Isn’t it more normal to have a constructor and, for initialization work that can fail, an Initialize method? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:52: void RegisterSandboxPolicy(int sandbox_policy_id, This requires callers to come up with fixed and unique IDs, in addition to supplying the policy object? That seems a unnecessary. Why not have this function return the policy ID to the caller as an opaque handle kind of type? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:55: // Called in the parent prior to fork()ing a child. The policy registered The comments on this function and the two that follow say “called in the parent” which doesn’t make it clear that the client code is supposed to call this. It could also mean that your sandbox calls these functions. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:59: // Called in the parent after fork()ing a child. It records the |handle| Blank line before, otherwise PrepareToForkWithPolicy gets lost. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:60: // and associates it with the specified-above |sandbox_policy_id|. What’s the caller supposed to do if they call PrepareToForkWithPolicy and then fork, but the fork fails? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:84: // The |lock_| protects all proceeding variables. It’s spelled preceding, but the comment is wrong, because the lock is protecting all following variables. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:89: bool is_across_fork_; Blank line after this. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:89: bool is_across_fork_; A better name would be prepared_to_fork_—something that makes clearer that PrepareToForkWithPolicy was called but FinishedFork wasn’t. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... File sandbox/mac/launchd_interception_server.cc (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:43: "chromium.sandbox.LaunchdInterceptionServer", NULL); org.chromium.blah? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:43: "chromium.sandbox.LaunchdInterceptionServer", NULL); Hopefully nothing on this queue will result in a call to the bootstrap server. Let’s see… https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:49: if (mach_port_allocate(task, MACH_PORT_RIGHT_RECEIVE, &port) != Can we get away with not even having a receive right to this port? We don’t actually want to receive. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:62: const mach_msg_size_t kBufferSize = 2096; Does this include space for the trailer? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:69: kern_return_t kr; Don’t declare this ’til you use it. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:74: mach_msg_header_t *request, *reply; Don’t declare *reply ’til you use it. (And Chromium style wants the * over on the type name, which is easier to swallow when you’re not declaring multiple pointers in the same statement.) https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:76: kr = vm_allocate(task, reinterpret_cast<vm_address_t*>(&request), kBufferSize, If you’re vm_allocating, you might as well round kBufferSize up to the next page. You can say mach_vm_round_page(2096). https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:77: VM_MAKE_TAG(VM_MEMORY_MACH_MSG)|TRUE); Spaces around |. Same on line 85. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:87: LOG(ERROR) << "Failed to allocate reply buffer. Error #" << kr << ": " Leaks the request buffer. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:95: LOG(ERROR) << "Unable to receive message. Error #" << kr << ": " Leaks the request and reply buffers. Do you want bpoop’s scoped_mach_vm? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:117: vm_deallocate(task, reinterpret_cast<vm_address_t>(request), kBufferSize); The nice thing about writing your own server loop instead of letting libdispatch handle it for you is that you could allocate your message buffers once outside the loop and then keep recycling them for each message. Dunno if doing that and dedicating a thread is better for how we launch our children. I guess you could allocate the message buffers as class member variables instead of in this function and maybe reap the best of all worlds. That’d also resolve the confusion around leaks in the case of early returns. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:138: // means it is from the sandbox host proceess or an unsandboxed child. Do we have unsandboxed children? Is it fair to require a permissive policy to be registered for them? We could plug some potential holes by making the default policy be “deny.” https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:152: VLOG(2) << "Forwarding vproc swap message #" << request->msgh_id; I’m still afraid of these. If they’re required for checkin, maybe we should crack the messages and only pass the ones that we’ve decided are safe, or only allow them to be forwarded the first time we see one for a child, or something like that. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:165: compat_shim_.look_up2_get_request_name(request)); Can we make this interface use StringBuffer or something copy-less? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:215: NOTREACHED(); Send a reject message or the child will hang forever? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:224: << kr << ": " << mach_error_string(kr); All of these…I should really check in my mach_logging.cc. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... File sandbox/mac/launchd_interception_server.h (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.h:25: LaunchdInterceptionServer(const BootstrapSandbox* sandbox); explicit https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.h:29: bool Initialize(); Yeah, see, here you used Initialize instead of having a static Create method. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/os_compatibility.cc File sandbox/mac/os_compatibility.cc (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/os_compatibility... sandbox/mac/os_compatibility.cc:50: return request->servicename; This is dangerous without a length limitation. 10.9.0 launchd-842.1.4/src/job_types.defs says that this is a 128-char buffer. Here comes the StringPiece recommendation again. launchd may be sloppy about this but you don’t have to be. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/os_compatibility... sandbox/mac/os_compatibility.cc:78: } else if (base::mac::IsOSLion() || You’ll do better with the OrEarlier/OrLater calls because more of them can be optimized out. This would be IsOSLionOrLater && IsOSMavericksOrEarlier. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/os_compatibility.h File sandbox/mac/os_compatibility.h (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/os_compatibility... sandbox/mac/os_compatibility.h:25: // The msgh_id for swap_integer. Can you put some vertical whitespace in here so it’s easy to find each member? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/policy.cc File sandbox/mac/policy.cc (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/policy.cc#newcode29 sandbox/mac/policy.cc:29: result = other.result; Did the “*this = other” form work in the constructor but not in operator=? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/policy.h File sandbox/mac/policy.h (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/policy.h#newcode36 sandbox/mac/policy.h:36: Rule(); Do you need this? I don’t consider the only use being in a test to be a “need.” https://codereview.chromium.org/264923003/diff/1/sandbox/mac/policy.h#newcode44 sandbox/mac/policy.h:44: mach_port_t substitute_port; Comment on the ownership of this port, or it’s unclear whether the Rule assumes responsibility for deallocating it or something else. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/sandbox_mac.gypi File sandbox/mac/sandbox_mac.gypi (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/sandbox_mac.gypi... sandbox/mac/sandbox_mac.gypi:24: '../', No trailing /. Fix line 45 too. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/sandbox_mac.gypi... sandbox/mac/sandbox_mac.gypi:28: '$(SDKROOT)/usr/lib/libbsm.dylib', Just for audit_token_to_au32, hmph. Sucks, but OK. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/sandbox_mac.gypi... sandbox/mac/sandbox_mac.gypi:40: ':sandbox', Just say sandbox, no colon.
I'll have to wait to rebase to take advantage of mach_logging.h and scoped_mach_vm.h. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... File sandbox/mac/bootstrap_sandbox.cc (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:22: TASK_BOOTSTRAP_PORT, sandbox->server_->server_port()); On 2014/05/06 20:51:50, Mark Mentovai wrote: > There’s a global extern mach_port_t named bootstrap_port that is the cached > bootstrap port, and it works in much the same way as the mach_task_self_ global. > Things use it to communicate with the bootstrap server without having to go > through task_get_special_port. Since you’re changing the task bootstrap port > here, you should strongly consider setting bootstrap_port to the same port. Right, this is something that I considered but wasn't sure was necessary. Since it's not necessary to change the bootstrap port for sandboxing purposes in the parent, and it's only being done because of the concerns over concurrent access to task_get_special_port(), it seems like additional overhead for callers trying to talk to the bootstrap server via the extern global. I'm happy to update the global, too, but it seems OK to leave it as-is. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:38: DCHECK(IsPolicyValid(policy)); On 2014/05/06 20:51:50, Mark Mentovai wrote: > You’re CHECKy elsewhere, why’d you land on DCHECK for policy registration? Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:50: return false; On 2014/05/06 20:51:50, Mark Mentovai wrote: > Based on the implementation here, it appears that you don’t need > PrepareToForkWithPolicy at all, you can just pass the policy ID to FinishedFork. Not really, because the child could start executing before the parent calls FinishFork. How would you know what sandbox policy to apply to this new child? > This check is the only thing that can fail, but returning failure here isn’t > that much better than treating an unknown policy ID as “reject everything.” I > don’t see a massive advantage to checking for the policy before forking. I changed it to a CHECK. The point is to prevent developer error. I think a "reject everything" approach for an unknown policy may seem reasonable, but it's really just letting the programmer do the wrong thing and get away with it. > Taking the same lock twice for each fork in the parent process sucks. Per offline discussion, taking the lock twice sucks, but the other options are not much better. Other options (for posterity): 1. Acquire in Prepare() and Release in Finished(), but this means blocking servicing of the bootstrap port for the duration of LaunchProcess 2. Use atomicops to do this, but this makes PolicyForProcess() subtle and tricky, to the extent of it not being worth it. 3. Use a RWLock, but this still requires taking the lock multiple times if we do not want to create the situation of (1). Also, no RWLock implementation exists currently. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:80: sandboxed_processes_.erase(handle); On 2014/05/06 20:51:50, Mark Mentovai wrote: > You were very CHECKy about things not being in the map when you were inserting > them. Do you want to be CHECKy that they’re in the map before erasing them? (You > can get an iterator, CHECK it, and then erase using the iterator to avoid doing > the same map lookup twice.) Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:95: } On 2014/05/06 20:51:50, Mark Mentovai wrote: > You can just say “else return NULL” here and not have to initialize policy_id to > -1. This is because effective_policy_id_ could be -1. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:108: mach_task_self(), TASK_BOOTSTRAP_PORT, &real_bootstrap_port_)); On 2014/05/06 20:51:50, Mark Mentovai wrote: > When you task_get_special_port, you need to mach_port_deallocate the port you > get when you’re done with it. Here, that’d be in the destructor. ScopedMachPort? Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbox.h File sandbox/mac/bootstrap_sandbox.h (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:42: // PrepareToForkWithPolicy() and FinishedFork() must be non-nested and balanced. On 2014/05/06 20:51:50, Mark Mentovai wrote: > An alternative would be to make PrepareToForkWithPolicy return some handle that > the caller would need to supply to FinishedFork, to not limit the nestiness and > balanciness of things. I need to limit the nestiness and balanciness of things because you don't know what policy to apply if there are multiple simultaneous sandbox fork()s, until the PID is associated with a policy. And the child can and does start executing sometime in that window (base::LaunchProcess handles both the fork and the exec), making bootstrap requests. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:45: // Creates a new sandbox manager. Returns NULL on failure. On 2014/05/06 20:51:50, Mark Mentovai wrote: > Isn’t it more normal to have a constructor and, for initialization work that can > fail, an Initialize method? They're both acceptable in Chromium. I generally prefer this for public interfaces because it doesn't mean you can have an object in an inconsistent state. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:52: void RegisterSandboxPolicy(int sandbox_policy_id, On 2014/05/06 20:51:50, Mark Mentovai wrote: > This requires callers to come up with fixed and unique IDs, in addition to > supplying the policy object? That seems a unnecessary. Why not have this > function return the policy ID to the caller as an opaque handle kind of type? Yes, I chose this because we already have sandbox-type enums that we will likely want to re-use. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:55: // Called in the parent prior to fork()ing a child. The policy registered On 2014/05/06 20:51:50, Mark Mentovai wrote: > The comments on this function and the two that follow say “called in the > parent” which doesn’t make it clear that the client code is supposed to call > this. It could also mean that your sandbox calls these functions. Clarified this in the class-level comment. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:59: // Called in the parent after fork()ing a child. It records the |handle| On 2014/05/06 20:51:50, Mark Mentovai wrote: > Blank line before, otherwise PrepareToForkWithPolicy gets lost. Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:60: // and associates it with the specified-above |sandbox_policy_id|. On 2014/05/06 20:51:50, Mark Mentovai wrote: > What’s the caller supposed to do if they call PrepareToForkWithPolicy and then > fork, but the fork fails? Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:84: // The |lock_| protects all proceeding variables. On 2014/05/06 20:51:50, Mark Mentovai wrote: > It’s spelled preceding, but the comment is wrong, because the lock is protecting > all following variables. Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:89: bool is_across_fork_; On 2014/05/06 20:51:50, Mark Mentovai wrote: > Blank line after this. Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.h:89: bool is_across_fork_; On 2014/05/06 20:51:50, Mark Mentovai wrote: > A better name would be prepared_to_fork_—something that makes clearer that > PrepareToForkWithPolicy was called but FinishedFork wasn’t. Removed, see other comment. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... File sandbox/mac/launchd_interception_server.cc (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:43: "chromium.sandbox.LaunchdInterceptionServer", NULL); On 2014/05/06 20:51:50, Mark Mentovai wrote: > org.chromium.blah? Oops lost org. somehow. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:43: "chromium.sandbox.LaunchdInterceptionServer", NULL); On 2014/05/06 20:51:50, Mark Mentovai wrote: > Hopefully nothing on this queue will result in a call to the bootstrap server. > Let’s see… It shouldn't… https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:49: if (mach_port_allocate(task, MACH_PORT_RIGHT_RECEIVE, &port) != On 2014/05/06 20:51:50, Mark Mentovai wrote: > Can we get away with not even having a receive right to this port? We don’t > actually want to receive. Nope, can't do that. xnu-2422.1.72/osfmk/ipc/mach_port.c:mach_port_allocate_full() https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:62: const mach_msg_size_t kBufferSize = 2096; On 2014/05/06 20:51:50, Mark Mentovai wrote: > Does this include space for the trailer? Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:69: kern_return_t kr; On 2014/05/06 20:51:50, Mark Mentovai wrote: > Don’t declare this ’til you use it. Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:74: mach_msg_header_t *request, *reply; On 2014/05/06 20:51:50, Mark Mentovai wrote: > Don’t declare *reply ’til you use it. (And Chromium style wants the * over on > the type name, which is easier to swallow when you’re not declaring multiple > pointers in the same statement.) Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:76: kr = vm_allocate(task, reinterpret_cast<vm_address_t*>(&request), kBufferSize, On 2014/05/06 20:51:50, Mark Mentovai wrote: > If you’re vm_allocating, you might as well round kBufferSize up to the next > page. You can say mach_vm_round_page(2096). Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:77: VM_MAKE_TAG(VM_MEMORY_MACH_MSG)|TRUE); On 2014/05/06 20:51:50, Mark Mentovai wrote: > Spaces around |. Same on line 85. Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:87: LOG(ERROR) << "Failed to allocate reply buffer. Error #" << kr << ": " On 2014/05/06 20:51:50, Mark Mentovai wrote: > Leaks the request buffer. Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:95: LOG(ERROR) << "Unable to receive message. Error #" << kr << ": " On 2014/05/06 20:51:50, Mark Mentovai wrote: > Leaks the request and reply buffers. Do you want bpoop’s scoped_mach_vm? Please. Want to upstream it? https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:117: vm_deallocate(task, reinterpret_cast<vm_address_t>(request), kBufferSize); On 2014/05/06 20:51:50, Mark Mentovai wrote: > The nice thing about writing your own server loop instead of letting libdispatch > handle it for you is that you could allocate your message buffers once outside > the loop and then keep recycling them for each message. Dunno if doing that and > dedicating a thread is better for how we launch our children. I guess you could > allocate the message buffers as class member variables instead of in this > function and maybe reap the best of all worlds. That’d also resolve the > confusion around leaks in the case of early returns. I moved the buffers to be member variables, since the dispatch queue is guaranteed to execute blocks serially in FIFO order. FWIW I'm convinced that libdispatch is better if you want to be able to join the "thread" running the server, since doing that with mach_msg() means creating your own quit signaling mechanism. And being able to join the thread is necessary for properly writing tests. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:138: // means it is from the sandbox host proceess or an unsandboxed child. On 2014/05/06 20:51:50, Mark Mentovai wrote: > Do we have unsandboxed children? Is it fair to require a permissive policy to be > registered for them? We could plug some potential holes by making the default > policy be “deny.” We do have unsandboxed children. Besides certain plugins, we also launch processes (like ps) that are not sandboxed. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:152: VLOG(2) << "Forwarding vproc swap message #" << request->msgh_id; On 2014/05/06 20:51:50, Mark Mentovai wrote: > I’m still afraid of these. If they’re required for checkin, maybe we should > crack the messages and only pass the ones that we’ve decided are safe, or only > allow them to be forwarded the first time we see one for a child, or something > like that. I'm pretty confident these are safe. Because launchd does not spawn these processes, when they connect it allocates an anonymous job for them and all of the potentially dangerous commands are effectively no-ops. I'd be happy to crack these, but would like to do so in a followup CL :). I "left room" to do so. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:165: compat_shim_.look_up2_get_request_name(request)); On 2014/05/06 20:51:50, Mark Mentovai wrote: > Can we make this interface use StringBuffer or something copy-less? I'm assuming you meant StringPiece. But no because the map keys are std::string and it doesn't do implicit conversion between the two. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:215: NOTREACHED(); On 2014/05/06 20:51:50, Mark Mentovai wrote: > Send a reject message or the child will hang forever? I think NOTREACHED() means NOTREACHED(). https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.cc:224: << kr << ": " << mach_error_string(kr); On 2014/05/06 20:51:50, Mark Mentovai wrote: > All of these…I should really check in my mach_logging.cc. Yes, you should :). Would review for free. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... File sandbox/mac/launchd_interception_server.h (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.h:25: LaunchdInterceptionServer(const BootstrapSandbox* sandbox); On 2014/05/06 20:51:50, Mark Mentovai wrote: > explicit Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/launchd_intercep... sandbox/mac/launchd_interception_server.h:29: bool Initialize(); On 2014/05/06 20:51:50, Mark Mentovai wrote: > Yeah, see, here you used Initialize instead of having a static Create method. Yes, because this class is only instantiated by the Create method. If I liked big files, this and bootstrap_sandbox would be one in the same. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/os_compatibility.cc File sandbox/mac/os_compatibility.cc (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/os_compatibility... sandbox/mac/os_compatibility.cc:50: return request->servicename; On 2014/05/06 20:51:50, Mark Mentovai wrote: > This is dangerous without a length limitation. 10.9.0 > launchd-842.1.4/src/job_types.defs says that this is a 128-char buffer. Here > comes the StringPiece recommendation again. > > launchd may be sloppy about this but you don’t have to be. Good catch. There's even BOOTSTRAP_MAX_NAME_LEN. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/os_compatibility... sandbox/mac/os_compatibility.cc:78: } else if (base::mac::IsOSLion() || On 2014/05/06 20:51:50, Mark Mentovai wrote: > You’ll do better with the OrEarlier/OrLater calls because more of them can be > optimized out. This would be IsOSLionOrLater && IsOSMavericksOrEarlier. Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/os_compatibility.h File sandbox/mac/os_compatibility.h (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/os_compatibility... sandbox/mac/os_compatibility.h:25: // The msgh_id for swap_integer. On 2014/05/06 20:51:50, Mark Mentovai wrote: > Can you put some vertical whitespace in here so it’s easy to find each member? Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/policy.cc File sandbox/mac/policy.cc (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/policy.cc#newcode29 sandbox/mac/policy.cc:29: result = other.result; On 2014/05/06 20:51:50, Mark Mentovai wrote: > Did the “*this = other” form work in the constructor but not in operator=? Actually removed both because this is a struct. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/policy.h File sandbox/mac/policy.h (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/policy.h#newcode36 sandbox/mac/policy.h:36: Rule(); On 2014/05/06 20:51:50, Mark Mentovai wrote: > Do you need this? I don’t consider the only use being in a test to be a “need.” For STL containers, yes. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/policy.h#newcode44 sandbox/mac/policy.h:44: mach_port_t substitute_port; On 2014/05/06 20:51:50, Mark Mentovai wrote: > Comment on the ownership of this port, or it’s unclear whether the Rule assumes > responsibility for deallocating it or something else. Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/sandbox_mac.gypi File sandbox/mac/sandbox_mac.gypi (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/sandbox_mac.gypi... sandbox/mac/sandbox_mac.gypi:24: '../', On 2014/05/06 20:51:50, Mark Mentovai wrote: > No trailing /. Fix line 45 too. Done. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/sandbox_mac.gypi... sandbox/mac/sandbox_mac.gypi:28: '$(SDKROOT)/usr/lib/libbsm.dylib', On 2014/05/06 20:51:50, Mark Mentovai wrote: > Just for audit_token_to_au32, hmph. Sucks, but OK. Yeah, it's already linked into Chromium Framework, so it's fine. https://codereview.chromium.org/264923003/diff/1/sandbox/mac/sandbox_mac.gypi... sandbox/mac/sandbox_mac.gypi:40: ':sandbox', On 2014/05/06 20:51:50, Mark Mentovai wrote: > Just say sandbox, no colon. Done.
On 2014/05/08 20:58:11, rsesek wrote: > I'll have to wait to rebase to take advantage of mach_logging.h and > scoped_mach_vm.h. Done.
https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... File sandbox/mac/bootstrap_sandbox.cc (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:22: TASK_BOOTSTRAP_PORT, sandbox->server_->server_port()); rsesek wrote: > On 2014/05/06 20:51:50, Mark Mentovai wrote: > > There’s a global extern mach_port_t named bootstrap_port that is the cached > > bootstrap port, and it works in much the same way as the mach_task_self_ > global. > > Things use it to communicate with the bootstrap server without having to go > > through task_get_special_port. Since you’re changing the task bootstrap port > > here, you should strongly consider setting bootstrap_port to the same port. > > Right, this is something that I considered but wasn't sure was necessary. Since > it's not necessary to change the bootstrap port for sandboxing purposes in the > parent, and it's only being done because of the concerns over concurrent access > to task_get_special_port(), it seems like additional overhead for callers trying > to talk to the bootstrap server via the extern global. > > I'm happy to update the global, too, but it seems OK to leave it as-is. OK, then you should mention in a comment that you’ve thought about it and aren’t touching it, and that that you also don’t envision anything wrong with touching it. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/bootstrap_sa... File sandbox/mac/bootstrap_sandbox.h (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/bootstrap_sa... sandbox/mac/bootstrap_sandbox.h:68: // If fork() failed and a new child was not created, pass kNullProcessHandle. Do you want to leave a TODO somewhere (.cc file?) about being able to handle this without a long-lived lock and threadiness restriction if a better process launchy thing was available? https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... File sandbox/mac/launchd_interception_server.cc (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:54: const int kMachMsgMemory = VM_MAKE_TAG(VM_MEMORY_MACH_MSG) | TRUE; You don’t really mean TRUE, you mean VM_FLAGS_ANYWHERE. And this shouldn’t be named kMachMsgMemory, but maybe kMachMsgMemoryFlags or kMachMsgMemoryAllocFlags. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:62: request_buffer_.reset(buffer, kBufferSize); Maybe you should have local scopers for this and .swap them into the member variables at the end of Initialize. Then, you’d free everything in case anything else fails in Initialize. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:72: // TODO(rsesek): Specify DISPATCH_QUEUE_SERIAL, in the 10.7 SDK. This scared me into thinking that serial as a behavior wasn’t available on 10.6. That didn’t make sense, though, so I checked the header and I see that what you’ve done is fine. But can you make this comment a little less scary by explaining what I assume you mean, like “use the new name when the SDK allows it, but it’s just defined to NULL so this still gives the right behavior.” https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:228: // TODO(rsesek): Crack the message and ensure that the swap is only being Cool. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/os_compatibi... File sandbox/mac/os_compatibility.cc (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/os_compatibi... sandbox/mac/os_compatibility.cc:64: name[name_length] = '\0'; Not necessary, std::string guarantees NUL-termination. Also not really legal, you’re not supposed to write past [.length() - 1].
Semi-drive-by. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/bootstrap_sa... File sandbox/mac/bootstrap_sandbox.h (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/bootstrap_sa... sandbox/mac/bootstrap_sandbox.h:64: void PrepareToForkWithPolicy(int sandbox_policy_id); The comment talks about returning a bool, but this function returns nothing. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... File sandbox/mac/launchd_interception_server.cc (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:149: // means it is from the sandbox host proceess or an unsandboxed child. typo: process https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... File sandbox/mac/launchd_interception_server.h (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.h:89: // The compatiblity shim that handles differences in message header IDs and typo: compatibility https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/os_compatibi... File sandbox/mac/os_compatibility.cc (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/os_compatibi... sandbox/mac/os_compatibility.cc:46: // TODO(rsesek): Libc provides strnlen() staring in 10.7. typo: starting https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/os_compatibi... sandbox/mac/os_compatibility.cc:100: &LaunchdLookUp2GetRequestName<look_up2_request_10_7>; In the past when we blindly patched CFAllocator in unknown systems, it caused crashes. So we made a policy decision to not blindly patch things that are known to change across system versions, knowing that it means that we take a slight security hit until we can get our hands on an OS seed to verify. See CanGetContextForCFAllocator. I'm surprised that we aren't going to do the same here. --- Also, from experience, doing a LOG(ERROR) like you do above is a mistake. If things _aren't_ compatible, you'll get crash reports in which it is obvious what the problem is. If things _are_ compatible, you will have every yokel who gets their hands on an OS seed log bugs against you for every weird thing they see because you have made yourself a target. Don't do it. :) https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/policy.h File sandbox/mac/policy.h (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/policy.h#new... sandbox/mac/policy.h:27: POLICY_SUBSTITUE_PORT, typo: SUBSTITUTE https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/policy.h#new... sandbox/mac/policy.h:33: // |substitute_port| must be NULL. If result is POLICY_SUBSTITUE_PORT, then Fix the constant names on this line and the line above once you remove the typos in the constant name.
https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... File sandbox/mac/bootstrap_sandbox.cc (right): https://codereview.chromium.org/264923003/diff/1/sandbox/mac/bootstrap_sandbo... sandbox/mac/bootstrap_sandbox.cc:22: TASK_BOOTSTRAP_PORT, sandbox->server_->server_port()); On 2014/05/09 20:11:19, Mark Mentovai wrote: > rsesek wrote: > > On 2014/05/06 20:51:50, Mark Mentovai wrote: > > > There’s a global extern mach_port_t named bootstrap_port that is the cached > > > bootstrap port, and it works in much the same way as the mach_task_self_ > > global. > > > Things use it to communicate with the bootstrap server without having to go > > > through task_get_special_port. Since you’re changing the task bootstrap port > > > here, you should strongly consider setting bootstrap_port to the same port. > > > > Right, this is something that I considered but wasn't sure was necessary. > Since > > it's not necessary to change the bootstrap port for sandboxing purposes in the > > parent, and it's only being done because of the concerns over concurrent > access > > to task_get_special_port(), it seems like additional overhead for callers > trying > > to talk to the bootstrap server via the extern global. > > > > I'm happy to update the global, too, but it seems OK to leave it as-is. > > OK, then you should mention in a comment that you’ve thought about it and aren’t > touching it, and that that you also don’t envision anything wrong with touching > it. Done. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/bootstrap_sa... File sandbox/mac/bootstrap_sandbox.h (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/bootstrap_sa... sandbox/mac/bootstrap_sandbox.h:64: void PrepareToForkWithPolicy(int sandbox_policy_id); On 2014/05/09 21:02:06, Avi wrote: > The comment talks about returning a bool, but this function returns nothing. Done. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/bootstrap_sa... sandbox/mac/bootstrap_sandbox.h:68: // If fork() failed and a new child was not created, pass kNullProcessHandle. On 2014/05/09 20:11:19, Mark Mentovai wrote: > Do you want to leave a TODO somewhere (.cc file?) about being able to handle > this without a long-lived lock and threadiness restriction if a better process > launchy thing was available? Sure, but I feel like it'll be a long-time coming. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... File sandbox/mac/launchd_interception_server.cc (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:54: const int kMachMsgMemory = VM_MAKE_TAG(VM_MEMORY_MACH_MSG) | TRUE; On 2014/05/09 20:11:19, Mark Mentovai wrote: > You don’t really mean TRUE, you mean VM_FLAGS_ANYWHERE. And this shouldn’t be > named kMachMsgMemory, but maybe kMachMsgMemoryFlags or kMachMsgMemoryAllocFlags. Done. I just copied this from some Apple code. Go figure. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:62: request_buffer_.reset(buffer, kBufferSize); On 2014/05/09 20:11:19, Mark Mentovai wrote: > Maybe you should have local scopers for this and .swap them into the member > variables at the end of Initialize. Then, you’d free everything in case anything > else fails in Initialize. If Initialize fails, anything properly allocated will still be destroyed when the object gets torn down. I don't feel strongly about doing this. Do you? https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:72: // TODO(rsesek): Specify DISPATCH_QUEUE_SERIAL, in the 10.7 SDK. On 2014/05/09 20:11:19, Mark Mentovai wrote: > This scared me into thinking that serial as a behavior wasn’t available on 10.6. > That didn’t make sense, though, so I checked the header and I see that what > you’ve done is fine. But can you make this comment a little less scary by > explaining what I assume you mean, like “use the new name when the SDK allows > it, but it’s just defined to NULL so this still gives the right behavior.” Done. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:149: // means it is from the sandbox host proceess or an unsandboxed child. On 2014/05/09 21:02:06, Avi wrote: > typo: process Done. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... File sandbox/mac/launchd_interception_server.h (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.h:89: // The compatiblity shim that handles differences in message header IDs and On 2014/05/09 21:02:06, Avi wrote: > typo: compatibility Done. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/os_compatibi... File sandbox/mac/os_compatibility.cc (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/os_compatibi... sandbox/mac/os_compatibility.cc:46: // TODO(rsesek): Libc provides strnlen() staring in 10.7. On 2014/05/09 21:02:06, Avi wrote: > typo: starting Done. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/os_compatibi... sandbox/mac/os_compatibility.cc:64: name[name_length] = '\0'; On 2014/05/09 20:11:19, Mark Mentovai wrote: > Not necessary, std::string guarantees NUL-termination. > > Also not really legal, you’re not supposed to write past [.length() - 1]. Done. For posterity, C++ § 21.4.5. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/os_compatibi... sandbox/mac/os_compatibility.cc:100: &LaunchdLookUp2GetRequestName<look_up2_request_10_7>; On 2014/05/09 21:02:06, Avi wrote: > In the past when we blindly patched CFAllocator in unknown systems, it caused > crashes. So we made a policy decision to not blindly patch things that are known > to change across system versions, knowing that it means that we take a slight > security hit until we can get our hands on an OS seed to verify. > > See CanGetContextForCFAllocator. > > I'm surprised that we aren't going to do the same here. > Launchd hasn't changed these messages since 10.7, so they appear much more stable than the allocator internals. The user of the sandbox could decide to not turn it on for FutureCat; that way the //sandbox/mac code doesn't have to know about whether or not it should be turned on. > Also, from experience, doing a LOG(ERROR) like you do above is a mistake. If > things _aren't_ compatible, you'll get crash reports in which it is obvious what > the problem is. If things _are_ compatible, you will have every yokel who gets > their hands on an OS seed log bugs against you for every weird thing they see > because you have made yourself a target. > > Don't do it. :) Oh, right. Switched to DLOG so that I'll see it in debug mode at least. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/policy.h File sandbox/mac/policy.h (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/policy.h#new... sandbox/mac/policy.h:27: POLICY_SUBSTITUE_PORT, On 2014/05/09 21:02:06, Avi wrote: > typo: SUBSTITUTE Ooof. Done. https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/policy.h#new... sandbox/mac/policy.h:33: // |substitute_port| must be NULL. If result is POLICY_SUBSTITUE_PORT, then On 2014/05/09 21:02:06, Avi wrote: > Fix the constant names on this line and the line above once you remove the typos > in the constant name. Done.
https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... File sandbox/mac/launchd_interception_server.cc (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:62: request_buffer_.reset(buffer, kBufferSize); rsesek wrote: > On 2014/05/09 20:11:19, Mark Mentovai wrote: > > Maybe you should have local scopers for this and .swap them into the member > > variables at the end of Initialize. Then, you’d free everything in case > anything > > else fails in Initialize. > > If Initialize fails, anything properly allocated will still be destroyed when > the object gets torn down. I don't feel strongly about doing this. Do you? Only a tiny bit, even if the object’s expected to be torn down. It’s allocated memory that’s sitting around taking up space without being used. Speaking of tearing things down, is dispatch_release safe to call on NULL? (In the destructor.) Speaking of THAT, you never initialize your dispatch sources to anything, so if Initialize failed and returned early, you’ll be dispatch_releasing garbage in the destructor. https://codereview.chromium.org/264923003/diff/80001/sandbox/mac/launchd_inte... File sandbox/mac/launchd_interception_server.cc (right): https://codereview.chromium.org/264923003/diff/80001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:31: dispatch_release(server_source_); Here, dangerous. https://codereview.chromium.org/264923003/diff/80001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:82: // Allocate the dummy sandbox port. Shouldn’t you do this before you start listening for messages?
https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... File sandbox/mac/launchd_interception_server.cc (right): https://codereview.chromium.org/264923003/diff/60001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:62: request_buffer_.reset(buffer, kBufferSize); On 2014/05/09 22:20:52, Mark Mentovai wrote: > rsesek wrote: > > On 2014/05/09 20:11:19, Mark Mentovai wrote: > > > Maybe you should have local scopers for this and .swap them into the member > > > variables at the end of Initialize. Then, you’d free everything in case > > anything > > > else fails in Initialize. > > > > If Initialize fails, anything properly allocated will still be destroyed when > > the object gets torn down. I don't feel strongly about doing this. Do you? > > Only a tiny bit, even if the object’s expected to be torn down. It’s allocated > memory that’s sitting around taking up space without being used. Well, in Create() it explicitly is destroyed if initialize fails. Nothing will leak. > Speaking of tearing things down, is dispatch_release safe to call on NULL? (In > the destructor.) It's not documented as such, so let's assume no. > Speaking of THAT, you never initialize your dispatch sources to anything, so if > Initialize failed and returned early, you’ll be dispatch_releasing garbage in > the destructor. Good catch. The compiler didn't complain about it being uninitialized for whatever reason. https://codereview.chromium.org/264923003/diff/80001/sandbox/mac/launchd_inte... File sandbox/mac/launchd_interception_server.cc (right): https://codereview.chromium.org/264923003/diff/80001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:31: dispatch_release(server_source_); On 2014/05/09 22:20:53, Mark Mentovai wrote: > Here, dangerous. Done. https://codereview.chromium.org/264923003/diff/80001/sandbox/mac/launchd_inte... sandbox/mac/launchd_interception_server.cc:82: // Allocate the dummy sandbox port. On 2014/05/09 22:20:53, Mark Mentovai wrote: > Shouldn’t you do this before you start listening for messages? Done.
LGTM https://codereview.chromium.org/264923003/diff/100001/sandbox/mac/launchd_int... File sandbox/mac/launchd_interception_server.cc (right): https://codereview.chromium.org/264923003/diff/100001/sandbox/mac/launchd_int... sandbox/mac/launchd_interception_server.cc:33: if (server_source_) Good call. The source code shows that dispatch_release doesn’t NULL-check, so this is necessary. The compiler had no way to know whether you were doing anything dangerous.
Thanks for the review. +jln for sandbox OWNERS
Rubberstamp lgtm Do you want to add yourself and another owner (maybe Mark?) to sandbox/mac/OWNERS? https://codereview.chromium.org/264923003/diff/100001/sandbox/mac/bootstrap_s... File sandbox/mac/bootstrap_sandbox.h (right): https://codereview.chromium.org/264923003/diff/100001/sandbox/mac/bootstrap_s... sandbox/mac/bootstrap_sandbox.h:36: // Clients that which to use the sandbox must inform it of the creation and nit: s/which/wish/ https://codereview.chromium.org/264923003/diff/100001/sandbox/mac/sandbox_mac... File sandbox/mac/sandbox_mac.gypi (right): https://codereview.chromium.org/264923003/diff/100001/sandbox/mac/sandbox_mac... sandbox/mac/sandbox_mac.gypi:11: 'bootstrap_sandbox.cc', How come you didn't need to create a SANDBOX_EXPORT macro to change the visibility for component builds? Presumably that's simply because it's not used yet?
On 2014/05/09 23:10:10, jln wrote: > Rubberstamp lgtm > > Do you want to add yourself and another owner (maybe Mark?) to > sandbox/mac/OWNERS? Thanks, done to both.
https://codereview.chromium.org/264923003/diff/100001/sandbox/mac/bootstrap_s... File sandbox/mac/bootstrap_sandbox.h (right): https://codereview.chromium.org/264923003/diff/100001/sandbox/mac/bootstrap_s... sandbox/mac/bootstrap_sandbox.h:36: // Clients that which to use the sandbox must inform it of the creation and On 2014/05/09 23:10:10, jln wrote: > nit: s/which/wish/ Done. https://codereview.chromium.org/264923003/diff/100001/sandbox/mac/sandbox_mac... File sandbox/mac/sandbox_mac.gypi (right): https://codereview.chromium.org/264923003/diff/100001/sandbox/mac/sandbox_mac... sandbox/mac/sandbox_mac.gypi:11: 'bootstrap_sandbox.cc', On 2014/05/09 23:10:10, jln wrote: > How come you didn't need to create a SANDBOX_EXPORT macro to change the > visibility for component builds? > > Presumably that's simply because it's not used yet? Yes, this doesn't make use of it. I was thinking of maybe moving the one in //sandbox/linux up a level so that it doesn't have to be duplicated with Mac. Will wait until this actually needs to be used, though.
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/264923003/120001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
The CQ bit was unchecked by rsesek@chromium.org
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/264923003/130001
Message was sent while issue was closed.
Change committed as 269937 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
