Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
721553002:
sandbox: Extend BrokerPolicy to support file creation (Closed)
Descriptionsandbox: Extend Broker to support improved file permissions
Added BrokerFilePermission class which is used to specify whitelists for the Broker
BUG=432369
TEST=sandbox_linux_unittest, chrome on Ubuntu 14.04 and 14.10
Committed: https://crrev.com/ad78f42ca0a83d590ff9f32050adfcb277fb34d6
Cr-Commit-Position: refs/heads/master@{#305894}
Patch Set 1 #Patch Set 2 : Added unlink #Patch Set 3 : minor fix #
Total comments: 1
Patch Set 4 : Added new FilePermissionClass #Patch Set 5 : rebase #Patch Set 6 : Remove DRI3 fixes #Patch Set 7 : Adding comments #
Total comments: 22
Patch Set 8 : refactor the class #Patch Set 9 : make constructor private #
Total comments: 5
Patch Set 10 : Remove serialization #
Total comments: 32
Patch Set 11 : review fixups #
Total comments: 17
Patch Set 12 : jorge's comments #
Total comments: 5
Patch Set 13 : lame #
Total comments: 7
Patch Set 14 : rebase #
Total comments: 13
Patch Set 15 : jln review changes #
Total comments: 12
Patch Set 16 : fixing androids lack of string.back #Patch Set 17 : nit #Patch Set 18 : fix component build #
Total comments: 26
Patch Set 19 : codereview2 #
Total comments: 5
Patch Set 20 : nits #Messages
Total messages: 41 (2 generated)
leecam@chromium.org changed reviewers: + jln@chromium.org, jorgelo@chromium.org, mdempsky@chromium.org
This is still a work in progress but thought I'd put it up for see what people think... I'll split the changes to BrokerPolicy out from the changes to the gpu policy to fix DRI3 into separate CLs but it all here to take a look at now. I've tried this on 14.10 and it does fix the issue.
On 2014/11/12 01:47:39, leecam wrote: > This is still a work in progress but thought I'd put it up for see what people > think... > > I'll split the changes to BrokerPolicy out from the changes to the gpu policy to > fix DRI3 into separate CLs but it all here to take a look at now. > > I've tried this on 14.10 and it does fix the issue. You reverted the env variable when testing right?
https://codereview.chromium.org/721553002/diff/40001/sandbox/linux/syscall_br... File sandbox/linux/syscall_broker/broker_policy.h (right): https://codereview.chromium.org/721553002/diff/40001/sandbox/linux/syscall_br... sandbox/linux/syscall_broker/broker_policy.h:17: struct BrokerPermission { I thought about this a little last night. I think this should be extracted to a separate file and the different permissions should be made into classes: BrokerPermission (which composes a string or a base::FilePath). | BrokerPermissionReadOnly... That way we don't need macros and we let polymorphism take care of the if statements.
On 2014/11/12 17:29:42, Jorge Lucangeli Obes wrote: > https://codereview.chromium.org/721553002/diff/40001/sandbox/linux/syscall_br... > File sandbox/linux/syscall_broker/broker_policy.h (right): > > https://codereview.chromium.org/721553002/diff/40001/sandbox/linux/syscall_br... > sandbox/linux/syscall_broker/broker_policy.h:17: struct BrokerPermission { > I thought about this a little last night. I think this should be extracted to a > separate file and the different permissions should be made into classes: > > BrokerPermission (which composes a string or a base::FilePath). > | > BrokerPermissionReadOnly... > > That way we don't need macros and we let polymorphism take care of the if > statements. Yep tested without the env patch :) Just moving the permission into a separate class - watch this space...
On 2014/11/12 18:36:14, leecam wrote: > On 2014/11/12 17:29:42, Jorge Lucangeli Obes wrote: > > > https://codereview.chromium.org/721553002/diff/40001/sandbox/linux/syscall_br... > > File sandbox/linux/syscall_broker/broker_policy.h (right): > > > > > https://codereview.chromium.org/721553002/diff/40001/sandbox/linux/syscall_br... > > sandbox/linux/syscall_broker/broker_policy.h:17: struct BrokerPermission { > > I thought about this a little last night. I think this should be extracted to > a > > separate file and the different permissions should be made into classes: > > > > BrokerPermission (which composes a string or a base::FilePath). > > | > > BrokerPermissionReadOnly... > > > > That way we don't need macros and we let polymorphism take care of the if > > statements. > > Yep tested without the env patch :) > > Just moving the permission into a separate class - watch this space... Still OOO, but took a look in a few seconds: - Please, be very careful about async signal safety. Adopt the convention of existing code and comment when the functions are async signal safe etc. Anything touched by async safe function should ideally be at least const. - Be a little more careful about the style guide: DISALLOW_COPY_AND_ASSIGN etc - sandbox/linux/syscall_broker/broker_file_permission.h looks really odd. I don't think we need all these classes. You can simply define an enum passed to the constructor parameter to handle and symbolize all these cases instead. - A lot of things, both member variables and methods should be marked as const - If you think you could migrate away from std::string to more async-signal safe types here, let's do it, otherwise we can do that in another CL. But we absolutely need to cleanup this code from dependencies that are not guaranteed aysnc sig safe.
https://engdoc.corp.google.com/eng/doc/devguide/cpp/primer.shtml is a good document to read up in some of the conventions such as DISALLOW_COPY_AND_ASSIGN. https://chromiumcodereview.appspot.com/721553002/diff/110001/content/common/s... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/110001/content/common/s... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:62: void AddArmMaliGpuWhitelist(std::vector<BrokerFilePermission>* permissions) { Does it make sense to split this CL in two and first add the BrokerFilePermission class, and then change the policies? It's already a big CL. https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:57: /* Note: This prefix match will allow any path under the whitelisted Use C++ comments here: // https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:222: CHECK(allow_create); If we use an enum, you can get rid of check. https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:223: // Recursive paths must have a trailing slash It might be useful to add these comments as error messages in the CHECK. https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:16: class BrokerFilePermission { Maybe Julien has a point here. How would this look if you did: enum BrokerPermissionType { kReadOnly, KReadWrite, ... class BrokerPermission { (maybe remove "File" to make things shorter). public BrokerPermission(std::string path, enum BrokerPermissionType type) { switch type: case kReadOnly: this.allow_read = True etc. It looks like keeping the boolean values makes the open and access checks easier. Moreover, if we don't let callers set the boolean values directly, we can avoid the sanity checks in the constructor. https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:47: private: Don't forget DISALLOW_COPY_AND_ASSIGN(BrokerPermission); (or whatever name you choose). https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_host.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_host.cc:68: requested_filename.c_str(), flags, &file_to_open, &unlink_after_open); When do you think we're gonna need this? I thought we were going to allow unlink() in the broker.
Adding some high-level comments. I'm back and available if you want to discuss this in a 1:1, on IRC or via e-mail. Let me know if I can help! Thanks for taking this on. https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/110001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:57: }; To add to my previous remark: don't forget that classes should not be copy-able in general. Since you want to store this in a vector, this must be copy-able. To resolve this: I suggest that you make BrokerFilePermission a passive struct. The CheckOpen / CheckAccess method look like they could adequately belong to BrokerPolicy, as private method (that take a const BrokerFilePermission& as one of their arguments) BrokerFilePermission could even be a subtype of class BrokerPolicy. One may even want to typedef std::vector<BrokerFilePermission> as "BrokerPolicyHolder" or something along these lines. Another advantage of this: to move the gpu process to a new process type or to a thread in the browser, we'll need to send the policy via IPC and serialize it. By doing this, you'll make serialization trivial
So Jorge wants a class and jln wants a struct...who wins? :) I'm easy but I think I prefer the class with its own check methods. https://codereview.chromium.org/721553002/diff/110001/content/common/sandbox_... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/721553002/diff/110001/content/common/sandbox_... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:62: void AddArmMaliGpuWhitelist(std::vector<BrokerFilePermission>* permissions) { On 2014/11/14 18:48:06, Jorge Lucangeli Obes wrote: > Does it make sense to split this CL in two and first add the > BrokerFilePermission class, and then change the policies? It's already a big CL. Not changing the behavior of the policies here but I have changed the BrokerProcess API so I need to fix up all the users of it. I have another CL which changes the policy to make DRI3 work https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:57: /* Note: This prefix match will allow any path under the whitelisted On 2014/11/14 18:48:07, Jorge Lucangeli Obes wrote: > Use C++ comments here: // Done. https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:222: CHECK(allow_create); On 2014/11/14 18:48:07, Jorge Lucangeli Obes wrote: > If we use an enum, you can get rid of check. Agreed, can't get rid of the path checks though... https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:223: // Recursive paths must have a trailing slash On 2014/11/14 18:48:07, Jorge Lucangeli Obes wrote: > It might be useful to add these comments as error messages in the CHECK. Will do! https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:16: class BrokerFilePermission { On 2014/11/14 18:48:07, Jorge Lucangeli Obes wrote: > Maybe Julien has a point here. How would this look if you did: > > enum BrokerPermissionType { > kReadOnly, > KReadWrite, > ... > > class BrokerPermission { (maybe remove "File" to make things shorter). > public BrokerPermission(std::string path, > enum BrokerPermissionType type) { > switch type: > case kReadOnly: > this.allow_read = True > > etc. > > It looks like keeping the boolean values makes the open and access checks > easier. Moreover, if we don't let callers set the boolean values directly, we > can avoid the sanity checks in the constructor. As we discussed offline, happy to change this to an enum but the sanity checks are not just on the boolean values but on the path too. So we'd still need the sanity checks in the constructor. We's also lose the ability to extend the API. e.g Say we added recursion depth. Then we could add uint32_t depth to BrokerFilePermission and then only change the interface on the 'recursive' classes e.g BrokerFilePermissionReadOnlyRecursive(std::string path, uint32_t depth). In the enum case we'd now have to add a redundant depth param to the constructor. But am happy to change it... https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:47: private: On 2014/11/14 18:48:07, Jorge Lucangeli Obes wrote: > Don't forget DISALLOW_COPY_AND_ASSIGN(BrokerPermission); (or whatever name you > choose). Can't use DISALLOW_COPY_AND_ASSIGN as it is copied into a vector. https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:57: }; On 2014/11/18 01:24:46, jln wrote: > To add to my previous remark: don't forget that classes should not be copy-able > in general. > > Since you want to store this in a vector, this must be copy-able. > > To resolve this: > > I suggest that you make BrokerFilePermission a passive struct. The CheckOpen / > CheckAccess method look like they could adequately belong to BrokerPolicy, as > private method (that take a const BrokerFilePermission& as one of their > arguments) > > BrokerFilePermission could even be a subtype of class BrokerPolicy. > > One may even want to typedef std::vector<BrokerFilePermission> as > "BrokerPolicyHolder" or something along these lines. > > Another advantage of this: to move the gpu process to a new process type or to a > thread in the browser, we'll need to send the policy via IPC and serialize it. > By doing this, you'll make serialization trivial Hah this was a struct in the first CL but changed it to class as per earlier review comments. BrokerFilePermission is only a string and a few bools (or one enum) so its pretty easy to serialize as a class or struct but I get the point! Happy to change this back though I do like the Check* methods as part of the BrokerFilePermission class. The Broker is a syscall_broker not an open_syscall_broker so I think its nice to architect the 'open' specific stuff into a class, so the constructor can validate it (which is bit less clean in a passive struct). https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_host.cc (right): https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_host.cc:68: requested_filename.c_str(), flags, &file_to_open, &unlink_after_open); On 2014/11/14 18:48:07, Jorge Lucangeli Obes wrote: > When do you think we're gonna need this? I thought we were going to allow > unlink() in the broker. We do allow unlink but we dont have an IPC for the client to unlink any file. We just have an API that will unlink a file after its openned/created by the BrokerHost.
Oh also on a less semantic note, can someone review my validation logic (the ValidatePath function for example) as a mistake there breaks the security of the sandbox and it needs to be bulletproof. Cheers!
On 2014/11/18 21:45:12, leecam wrote: > Oh also on a less semantic note, can someone review my validation logic (the > ValidatePath function for example) as a mistake there breaks the security of the > sandbox and it needs to be bulletproof. Make sure you add a bunch of unittests for these as well! :)
On 2014/11/18 21:48:50, jln wrote: > On 2014/11/18 21:45:12, leecam wrote: > > Oh also on a less semantic note, can someone review my validation logic (the > > ValidatePath function for example) as a mistake there breaks the security of > the > > sandbox and it needs to be bulletproof. > > Make sure you add a bunch of unittests for these as well! :) Yeah ack. I've added a few extra and I'm writing a few more at the mo.
On 2014/11/18 21:40:54, leecam wrote: > So Jorge wants a class and jln wants a struct...who wins? :) > > I'm easy but I think I prefer the class with its own check methods. I'll do another pass. I could be convinced that we should build a copy-able class. But in this case, we would need to add the usual checks: make it final to prevent risks of "object slicing" (and of course remove all your inherited class as mentioned earlier, there should be better ways to make this readable (like passing an enum to the constructor). Also please make the copy constructor explicit, with "= default". .... but, we need to move into a world where we don't use std::vector anyways. As mentioned, we can't use the STL here, it has been a huge headache in regard to async signal safety. I mentioned before that we could construct the policy by using the STL, but then we have to "lock" it to a read-only simple structure (like an array of BrokerFilePermission). In that case, it could simplify things so have a BrokerFilePermissionHolder struct (which would be embedded by the BrokerFilePermission class). These are changes that we can do later, but please keep them in mind in case it makes sense to do them now: there is no future for STL in async signal safe code, it needs to go!
On 2014/11/18 21:57:08, jln wrote: > On 2014/11/18 21:40:54, leecam wrote: > > So Jorge wants a class and jln wants a struct...who wins? :) > > > > I'm easy but I think I prefer the class with its own check methods. > > I'll do another pass. I could be convinced that we should build a copy-able > class. But in this case, we would need to add the usual checks: make it final to > prevent risks of "object slicing" (and of course remove all your inherited class > as mentioned earlier, there should be better ways to make this readable (like > passing an enum to the constructor). Yeah happy to do the enum...its a bit hard to extend the API (as mentioned above) but I don't think we are going to add much more. > > Also please make the copy constructor explicit, with "= default". > > .... but, we need to move into a world where we don't use std::vector anyways. > As mentioned, we can't use the STL here, it has been a huge headache in regard > to async signal safety. > > I mentioned before that we could construct the policy by using the STL, but then > we have to "lock" it to a read-only simple structure (like an array of > BrokerFilePermission). In that case, it could simplify things so have a > BrokerFilePermissionHolder struct (which would be embedded by the > BrokerFilePermission class). Yeah agreed, I do only use the STL vector for storage. In the constructor the backing store is cast to a simple array and that is whats used in the async code, so we don't use any STL functions that might screw with memory under the covers. > > These are changes that we can do later, but please keep them in mind in case it > makes sense to do them now: there is no future for STL in async signal safe > code, it needs to go!
Looking good. https://codereview.chromium.org/721553002/diff/110001/content/common/sandbox_... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://codereview.chromium.org/721553002/diff/110001/content/common/sandbox_... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:35: using sandbox::syscall_broker::BrokerFilePermission; Nit: Sort. (Technically I think all of the sandbox::syscall_broker::* lines should go below sandbox::bpf_dsl, since we seem to usually sort by simple ASCII.) https://codereview.chromium.org/721553002/diff/110001/content/common/sandbox_... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:276: for (const auto& perm : permissions_extra) { (Could use "permissions.insert(permissions.end(), permissions_extra.begin(), permissions_extra.end())", but as-is is okay by me too and perhaps easier to understand.) https://codereview.chromium.org/721553002/diff/110001/content/common/sandbox_... File content/common/sandbox_linux/bpf_gpu_policy_linux.h (right): https://codereview.chromium.org/721553002/diff/110001/content/common/sandbox_... content/common/sandbox_linux/bpf_gpu_policy_linux.h:17: class BrokerFilePermission; nit: Sort. https://codereview.chromium.org/721553002/diff/110001/content/common/sandbox_... content/common/sandbox_linux/bpf_gpu_policy_linux.h:38: // |read_whitelist_extra| and |write_whitelist_extra| are lists of file nit: Update comment. https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:47: private: On 2014/11/18 21:40:54, leecam wrote: > Can't use DISALLOW_COPY_AND_ASSIGN as it is copied into a vector. In that case, the style guide requires you to write: BrokerFilePermission(const BrokerFilePermission&) = default; BrokerFilePermission operator=(const BrokerFilePermission&) = default; to explicitly declare it copyable/assignable. :-/ https://codereview.chromium.org/721553002/diff/110001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:59: class BrokerFilePermissionReadOnly : public BrokerFilePermission { Do we gain anything by making these subclasses instead of just making the constructors into functions? E.g., BrokerFilePermission BrokerFilePermissionReadOnly(std::string path); BrokerFilePermission BrokerFilePermissionWriteOnly(std::string path); ... I might go further and suggest: class FilePermission { // "Broker" is redundant with syscall_broker namespace static FilePermission ReadOnly(std::string path); static FilePermission WriteOnly(std::string path); ... private: FilePermission(...); // private constructor so users have to call static builder method above }; and then call sites look like: FilePermission::ReadOnly("/dev/foo")
So I refactored the class to mdempsky's suggestion. PTAL and let me know if we are happy with this API. If we agree then I'll finish off the test cases (dont want to rewrite all the tests again if we change the API). I also added some calls to handle serializing the class.
On 2014/11/19 00:27:23, leecam wrote: > So I refactored the class to mdempsky's suggestion. PTAL and let me know if we > are happy with this API. I'm happy with it. :)
I think the API makes sense. https://codereview.chromium.org/721553002/diff/150001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://codereview.chromium.org/721553002/diff/150001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:52: // Construct a permission from a serilized pickle buffer. "serialized" More generally though, I don't think we need serialization right now, but if you've already done it I guess we can include it in this CL.
https://codereview.chromium.org/721553002/diff/150001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://codereview.chromium.org/721553002/diff/150001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:52: // Construct a permission from a serilized pickle buffer. On 2014/11/19 00:36:55, Jorge Lucangeli Obes wrote: > "serialized" > > More generally though, I don't think we need serialization right now, but if > you've already done it I guess we can include it in this CL. Please, let's remove it from this CL. We should discuss this separately, but when we need serialization, it has to be done with Chrome IPC, not a new, ad-hoc IPC mechanism. The current ad-hoc IPC mechanism should be limited to what we need it for: async signal safe IPCs.
On 2014/11/19 00:44:24, jln wrote: > https://codereview.chromium.org/721553002/diff/150001/sandbox/linux/syscall_b... > File sandbox/linux/syscall_broker/broker_file_permission.h (right): > > https://codereview.chromium.org/721553002/diff/150001/sandbox/linux/syscall_b... > sandbox/linux/syscall_broker/broker_file_permission.h:52: // Construct a > permission from a serilized pickle buffer. > On 2014/11/19 00:36:55, Jorge Lucangeli Obes wrote: > > "serialized" > > > > More generally though, I don't think we need serialization right now, but if > > you've already done it I guess we can include it in this CL. > > Please, let's remove it from this CL. We should discuss this separately, but > when we need serialization, it has to be done with Chrome IPC, not a new, ad-hoc > IPC mechanism. > > The current ad-hoc IPC mechanism should be limited to what we need it for: async > signal safe IPCs. Gone :) I was just trying to prove it was really easy to serialize using that API...
I'm struggling with a massive headache, so only another small round of high level comments. Sorry for the long delay. This should be ready soon, it's getting in shape :) https://codereview.chromium.org/721553002/diff/150001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_process_unittest.cc (right): https://codereview.chromium.org/721553002/diff/150001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_process_unittest.cc:242: void TestBadPaths(bool fast_check_in_client) { It looks like this unit test belongs elsewhere: BrokerPolicy or BrokerFilePermission. We still need a little bit of end-to-end test, so you can keep some of all around, but now that I've split BrokerProcess into a bunch of class, we should take advantage of the opportunity for better unit testing. https://codereview.chromium.org/721553002/diff/150001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_process_unittest.cc:606: void GetTempFileName(char* name, size_t len) { This is code duplication, which is evil. Just use and destroy a ScopedTemporaryFile() within scope instead of the duplication. Also in general, this is not guaranteed to work, as there is an obvious race. At the very least, document that this is a hack. https://codereview.chromium.org/721553002/diff/150001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_process_unittest.cc:642: Check also what happens if the file already exists?
On 2014/11/19 00:50:50, leecam wrote: > On 2014/11/19 00:44:24, jln wrote: > > The current ad-hoc IPC mechanism should be limited to what we need it for: > async > > signal safe IPCs. > Gone :) I was just trying to prove it was really easy to serialize using that > API... Understood: but my comments where precisely about making usage of the Chrome IPC easier. But in any case, this problem should be taken care of later. It'll be a little bit of an issue mostly because I wouldn't want sandbox/ to depend on ipc/ so some of the stuff should probably move to content/. TBD! :)
This looks good, let's try to get it in soon. https://codereview.chromium.org/721553002/diff/170001/content/common/sandbox_... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/721553002/diff/170001/content/common/sandbox_... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:35: using sandbox::syscall_broker::BrokerFilePermission; Should we change this to be simply "FilePermission" since it lives in the syscall_broker namespace? I think Matthew had suggested it. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_client.cc (right): https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_client.cc:57: !broker_policy_.GetFileNameIfAllowedToOpen(pathname, flags, NULL, Given that we have two NULL arguments now, it might be clearer to add the names of the arguments: NULL /*arg1*/, NULL /*arg2*/ https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:13: #include "base/pickle.h" not needed anymore. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:222: CHECK(allow_create); Add comments as strings to CHECK: CHECK(...) << "unlink(2) on creation not allowed without create permission". https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:225: CHECK(path_.back() == '/'); Here too. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:227: CHECK(path_.front() == '/'); And here. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:52: // Returns true if |requested_filename| is allowed to be open "open" -> "opened" https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:81: bool IsPathCoveredByThisPermission(const char* requested_filename) const; Since this method lives in the Permission class, "ByThisPermission" is probably redundant? Maybe something like "CoversPath"? https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:86: const bool unlink_; // unlink after openning. "opening"
https://codereview.chromium.org/721553002/diff/170001/content/common/sandbox_... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/721553002/diff/170001/content/common/sandbox_... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:35: using sandbox::syscall_broker::BrokerFilePermission; On 2014/11/20 00:13:20, Jorge Lucangeli Obes wrote: > Should we change this to be simply "FilePermission" since it lives in the > syscall_broker namespace? I think Matthew had suggested it. As discussed as everything else in the syscall_broker namespace still is prefixed with Broker it kinda looks out of place. Will leave for now and maybe we could land a CL that reworks the naming later. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_client.cc (right): https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_client.cc:57: !broker_policy_.GetFileNameIfAllowedToOpen(pathname, flags, NULL, On 2014/11/20 00:13:20, Jorge Lucangeli Obes wrote: > Given that we have two NULL arguments now, it might be clearer to add the names > of the arguments: > > NULL /*arg1*/, NULL /*arg2*/ Done. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:13: #include "base/pickle.h" On 2014/11/20 00:13:20, Jorge Lucangeli Obes wrote: > not needed anymore. Done. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:222: CHECK(allow_create); On 2014/11/20 00:13:20, Jorge Lucangeli Obes wrote: > Add comments as strings to CHECK: > > CHECK(...) << "unlink(2) on creation not allowed without create permission". Done. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:225: CHECK(path_.back() == '/'); On 2014/11/20 00:13:20, Jorge Lucangeli Obes wrote: > Here too. Done. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:227: CHECK(path_.front() == '/'); On 2014/11/20 00:13:20, Jorge Lucangeli Obes wrote: > And here. Done. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:52: // Returns true if |requested_filename| is allowed to be open On 2014/11/20 00:13:20, Jorge Lucangeli Obes wrote: > "open" -> "opened" Done. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:81: bool IsPathCoveredByThisPermission(const char* requested_filename) const; On 2014/11/20 00:13:20, Jorge Lucangeli Obes wrote: > Since this method lives in the Permission class, "ByThisPermission" is probably > redundant? Maybe something like "CoversPath"? Done. https://codereview.chromium.org/721553002/diff/170001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.h:86: const bool unlink_; // unlink after openning. On 2014/11/20 00:13:20, Jorge Lucangeli Obes wrote: > "opening" Done.
I think the logic here is sound, apart from the possible mix-up between trailing slashes. https://chromiumcodereview.appspot.com/721553002/diff/190001/content/common/s... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/190001/content/common/s... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:28: #include "sandbox/linux/syscall_broker/broker_process.h" Do we still need this include? Shouldn't this be "broker_file_permission.h"? https://chromiumcodereview.appspot.com/721553002/diff/190001/content/common/s... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:165: Extra line? https://chromiumcodereview.appspot.com/721553002/diff/190001/content/common/s... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/190001/content/common/s... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:32: #include "sandbox/linux/syscall_broker/broker_process.h" Do you need to include "broker_file_permission.h" here? https://chromiumcodereview.appspot.com/721553002/diff/190001/sandbox/linux/bp... File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/190001/sandbox/linux/bp... sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:44: #include "sandbox/linux/syscall_broker/broker_process.h" Do you need to include "broker_file_permission.h" here? https://chromiumcodereview.appspot.com/721553002/diff/190001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/190001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:38: // No /../ anywhere Can this check be folded into the previous check? What is covered by "No /../ anywhere" that is not covered by "No /.." anywhere? https://chromiumcodereview.appspot.com/721553002/diff/190001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:98: allowed = true; These can probably be simplified to: allowed = (allow_read_ || allow_write_) and variations, but maybe the form with the if statement is clearer. https://chromiumcodereview.appspot.com/721553002/diff/190001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:167: // If this file is to be unlinked, ensure its created. it's https://chromiumcodereview.appspot.com/721553002/diff/190001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:222: // Recursive paths must have a trailing slash In broker_file_permission.cc, you're blocking trailing slashes: 31 // No trailing / (but "/" is valid) 32 if (len > 1 && path[len - 1] == '/') 33 return false; https://chromiumcodereview.appspot.com/721553002/diff/190001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_policy.h (right): https://chromiumcodereview.appspot.com/721553002/diff/190001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.h:61: // policy requests the caller unlink the path after openning. opening
Thanks Jorge. I think my slash logic is correct as is...see comments. https://codereview.chromium.org/721553002/diff/190001/content/common/sandbox_... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/721553002/diff/190001/content/common/sandbox_... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:28: #include "sandbox/linux/syscall_broker/broker_process.h" On 2014/11/20 21:02:59, Jorge Lucangeli Obes wrote: > Do we still need this include? Shouldn't this be "broker_file_permission.h"? Done. https://codereview.chromium.org/721553002/diff/190001/content/common/sandbox_... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://codereview.chromium.org/721553002/diff/190001/content/common/sandbox_... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:32: #include "sandbox/linux/syscall_broker/broker_process.h" On 2014/11/20 21:02:59, Jorge Lucangeli Obes wrote: > Do you need to include "broker_file_permission.h" here? Done. https://codereview.chromium.org/721553002/diff/190001/sandbox/linux/bpf_dsl/b... File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (right): https://codereview.chromium.org/721553002/diff/190001/sandbox/linux/bpf_dsl/b... sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:44: #include "sandbox/linux/syscall_broker/broker_process.h" On 2014/11/20 21:03:00, Jorge Lucangeli Obes wrote: > Do you need to include "broker_file_permission.h" here? Done. https://codereview.chromium.org/721553002/diff/190001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://codereview.chromium.org/721553002/diff/190001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:38: // No /../ anywhere On 2014/11/20 21:03:00, Jorge Lucangeli Obes wrote: > Can this check be folded into the previous check? What is covered by "No /../ > anywhere" that is not covered by "No /.." anywhere? What about this allowed path /good_path/..good_file That would match /.. even though its valid https://codereview.chromium.org/721553002/diff/190001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:98: allowed = true; On 2014/11/20 21:03:00, Jorge Lucangeli Obes wrote: > These can probably be simplified to: > allowed = (allow_read_ || allow_write_) and variations, but maybe the form with > the if statement is clearer. Yeah I did it as 'if' statements for readability, I thought it was easier to reason over this logic. https://codereview.chromium.org/721553002/diff/190001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:167: // If this file is to be unlinked, ensure its created. On 2014/11/20 21:03:00, Jorge Lucangeli Obes wrote: > it's Done. https://codereview.chromium.org/721553002/diff/190001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_file_permission.cc:222: // Recursive paths must have a trailing slash On 2014/11/20 21:03:00, Jorge Lucangeli Obes wrote: > In broker_file_permission.cc, you're blocking trailing slashes: > > 31 // No trailing / (but "/" is valid) > 32 if (len > 1 && path[len - 1] == '/') > 33 return false; We block them on paths that the client is requesting. But this is validating the paths being added to the whitelist where we ensure that recursive paths do have a trailing slash. This means we can allow /tmp/* but access to /tmp (the folder itself) would be blocked unless /tmp was also added as an absolute path. https://codereview.chromium.org/721553002/diff/190001/sandbox/linux/syscall_b... File sandbox/linux/syscall_broker/broker_policy.h (right): https://codereview.chromium.org/721553002/diff/190001/sandbox/linux/syscall_b... sandbox/linux/syscall_broker/broker_policy.h:61: // policy requests the caller unlink the path after openning. On 2014/11/20 21:03:00, Jorge Lucangeli Obes wrote: > opening Done.
Two nits missing from the previous PS, I think the logic is sound. https://chromiumcodereview.appspot.com/721553002/diff/210001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/210001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:167: // If this file is to be unlinked, ensure its created. it's... forgot to upload? https://chromiumcodereview.appspot.com/721553002/diff/210001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_policy.h (right): https://chromiumcodereview.appspot.com/721553002/diff/210001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.h:61: // policy requests the caller unlink the path after openning. Forgot to upload?
https://chromiumcodereview.appspot.com/721553002/diff/210001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/210001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:167: // If this file is to be unlinked, ensure its created. On 2014/11/20 22:20:15, Jorge Lucangeli Obes wrote: > it's... forgot to upload? save fail :( https://chromiumcodereview.appspot.com/721553002/diff/210001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_policy.h (right): https://chromiumcodereview.appspot.com/721553002/diff/210001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.h:61: // policy requests the caller unlink the path after openning. On 2014/11/20 22:20:15, Jorge Lucangeli Obes wrote: > Forgot to upload? Done. https://chromiumcodereview.appspot.com/721553002/diff/210001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.h:61: // policy requests the caller unlink the path after openning. On 2014/11/20 22:20:15, Jorge Lucangeli Obes wrote: > Forgot to upload? Save fail :(
This lgtm, don't know if Matthew or Julien have other comments.
On 2014/11/20 23:25:12, Jorge Lucangeli Obes wrote: > This lgtm, don't know if Matthew or Julien have other comments. Yeah, please wait, I would like to take another look. Sorry I've been so slow (sick + backlogged). Julien
This is in good shape. Please, create a broker_file_permission_unittest.cc and add a few tests there. Otherwise a few comments are related to documenting functions and parameters: be careful there, especially in header files! https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:21: bool ValidatePath(const char* path) { This absolutely needs unit tests. Maybe move to another file broker_utils.cc or something like this. Also please describe what the function does! You could also move it to a private static method, and then friend a FilePermissionTester that you could use in broker_file_permission_unittest.cc to test it. class FilePermissionTester { public: bool ValidatePath(..) { return BrokerFilePermission::ValidatePath(); } private: DISALLO.... }; https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:12: class Pickle; Nit: remove https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:17: Please, add a general class description on top of this. Especially important is the fact that construction is not async signal safe, but that outside of construction / destruction, all methods are async signal safe and thread-safe. https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:24: static BrokerFilePermission ReadOnly(std::string path) { const std::string& path: C++11 is better at optimizing this thanks to move semantics, but constructing an object here could in theory incur 3 copies of the path. Same remark for all the factory methods below. https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:62: bool* unlink_after_open) const; Don't forget to describe unlink_after_open. In particular the fact that it call be NULL. https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:64: // by this permission. Don't forget to mention |mode|. Also you should mention that this pertains to access(2). https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:65: // If |file_to_open| is not NULL it is set to point to either s/file_to_open/file_to_access/ in the comments. https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:75: BrokerFilePermission(std::string path, With C++11, this becomes less of an issue, but there is no reason for a copy here. Use const std::string path& instead. https://chromiumcodereview.appspot.com/721553002/diff/230001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/230001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:219: if (unlink_) Style: {} for multi-line CHECK. Note: in general I highly recommend to always use {}, even though the style guide doesn't enforce that. This makes sure that clang-format doesn't end-up making your code non style compliant like in this case. https://chromiumcodereview.appspot.com/721553002/diff/230001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:221: << "unlink(2) on creation not allowed without create permission"; Note: in general this is "ok" for DCHECK, but for pure CHECK, this ends up wasting string space in the shipping executable. It's often a difficult trade-off, but in this case, this CHECK only depends on compile-time things (how we use the class, which won't be too dynamic). Given that, I would prefer a comment and a simple CHECK() should lead developers to the right comment. https://chromiumcodereview.appspot.com/721553002/diff/230001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:224: CHECK(path_.back() == '/') << "recursive paths must have a trailing slash"; Should we also check that if a path is a directory it is passed as a recursive path? https://chromiumcodereview.appspot.com/721553002/diff/230001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/230001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:83: const std::string path_; Why store a std::string? It looks like a scoped_ptr<char> to a copy of path.c_str() would be much better for this class. Do we ever need to access the std::string? Feel free to punt to another CL if you prefer. https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_host.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_host.cc:43: if (flags & O_CREAT) Your choice, but I recommend {} for all if / else statements. It makes it much easier to forget about it, rather that think on whether or not things could change because of a comment or clang-format putting things on two lines. https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_policy.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.cc:28: permissions_array_ = &permissions_[0]; Nit: You should be able to have that in the initializer list as well. In which case you could mark the pointer itself as const too. https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_policy.h (right): https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.h:12: #include "base/memory/scoped_ptr.h" This doesn't seem required, is it? https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.h:73: const BrokerFilePermission* permissions_array_; Add comment. https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_process_unittest.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:620: int fd = -1; Please, use a ScopedFD here. https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:636: ssize_t len = write(fd, test_text, sizeof(test_text)); HANDLE_EINTR() https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:639: ASSERT_EQ(close(fd), 0); Would need to IGNORE_EINTR(), but just use a scoped fd? https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:641: int fd_check = open(tempfile_name, O_RDONLY); Use a ScopedFD
Also my apologies for straddling multiple patchsets. I miss-clicked in the review tool at some point.
Ok I think that's everything addressed. https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:21: bool ValidatePath(const char* path) { On 2014/11/24 20:12:16, jln wrote: > This absolutely needs unit tests. > > Maybe move to another file broker_utils.cc or something like this. Also please > describe what the function does! > > You could also move it to a private static method, and then friend a > FilePermissionTester that you could use in broker_file_permission_unittest.cc to > test it. > > class FilePermissionTester { > public: > bool ValidatePath(..) { return BrokerFilePermission::ValidatePath(); } > private: > DISALLO.... > }; Done. https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:17: On 2014/11/24 20:12:16, jln wrote: > Please, add a general class description on top of this. Especially important is > the fact that construction is not async signal safe, but that outside of > construction / destruction, all methods are async signal safe and thread-safe. Done. https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:24: static BrokerFilePermission ReadOnly(std::string path) { On 2014/11/24 20:12:16, jln wrote: > const std::string& path: > > C++11 is better at optimizing this thanks to move semantics, but constructing an > object here could in theory incur 3 copies of the path. > > Same remark for all the factory methods below. Done. https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:64: // by this permission. On 2014/11/24 20:12:16, jln wrote: > Don't forget to mention |mode|. Also you should mention that this pertains to > access(2). Done. https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:65: // If |file_to_open| is not NULL it is set to point to either On 2014/11/24 20:12:16, jln wrote: > s/file_to_open/file_to_access/ in the comments. Done. https://chromiumcodereview.appspot.com/721553002/diff/170001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:75: BrokerFilePermission(std::string path, On 2014/11/24 20:12:16, jln wrote: > With C++11, this becomes less of an issue, but there is no reason for a copy > here. > Use const std::string path& instead. Done. https://chromiumcodereview.appspot.com/721553002/diff/230001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/230001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:219: if (unlink_) On 2014/11/24 20:12:16, jln wrote: > Style: {} for multi-line CHECK. > > Note: in general I highly recommend to always use {}, even though the style > guide doesn't enforce that. This makes sure that clang-format doesn't end-up > making your code non style compliant like in this case. Done. https://chromiumcodereview.appspot.com/721553002/diff/230001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:221: << "unlink(2) on creation not allowed without create permission"; On 2014/11/24 20:12:17, jln wrote: > Note: in general this is "ok" for DCHECK, but for pure CHECK, this ends up > wasting string space in the shipping executable. > > It's often a difficult trade-off, but in this case, this CHECK only depends on > compile-time things (how we use the class, which won't be too dynamic). Given > that, I would prefer a comment and a simple CHECK() should lead developers to > the right comment. strings removed https://chromiumcodereview.appspot.com/721553002/diff/230001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:224: CHECK(path_.back() == '/') << "recursive paths must have a trailing slash"; On 2014/11/24 20:12:17, jln wrote: > Should we also check that if a path is a directory it is passed as a recursive > path? I thought you might want to allow access to the directory but not everything under it. E.g /tmp but not /tmp/* https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_host.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_host.cc:43: if (flags & O_CREAT) On 2014/11/24 20:12:17, jln wrote: > Your choice, but I recommend {} for all if / else statements. > > It makes it much easier to forget about it, rather that think on whether or not > things could change because of a comment or clang-format putting things on two > lines. Done. https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_policy.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.cc:28: permissions_array_ = &permissions_[0]; On 2014/11/24 20:12:17, jln wrote: > Nit: You should be able to have that in the initializer list as well. In which > case you could mark the pointer itself as const too. The pointer is const. This should actually only be set when permissions_array_ > 0. So I've added that check and I'd like to leave it like this for now... https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_policy.h (right): https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.h:12: #include "base/memory/scoped_ptr.h" On 2014/11/24 20:12:17, jln wrote: > This doesn't seem required, is it? Nope :) https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.h:73: const BrokerFilePermission* permissions_array_; On 2014/11/24 20:12:17, jln wrote: > Add comment. Done.
Looking good. But a few more nits. In general: remember to add comments. They're especially important for code review, because they are what your reviewer will check against. Make sure to explain what the parameters are, how to use them etc, so that your reviewer can check the implementation. They're also crucial when coming to new code. "ValidatePath()" is not clear enough a name to skip the comments. https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_policy.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/250001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_policy.cc:28: permissions_array_ = &permissions_[0]; On 2014/11/25 02:06:49, leecam wrote: > On 2014/11/24 20:12:17, jln wrote: > > Nit: You should be able to have that in the initializer list as well. In which > > case you could mark the pointer itself as const too. > > The pointer is const. This should actually only be set when permissions_array_ > > 0. So I've added that check and I'd like to leave it like this for now... The pointer points to a const object, but it is not const itself. It's fine if you want the check though. https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:62: // |unlink_after_open| is set to point to true if the caller should unlink I would prefer "If not NULL, ...." https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:95: // the compiler will still enforce no changes outside of the constructor. The issue is actually that std::vector requires copy-assignment (checked with jyasskin) Could you update the comment? Sad :( https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_process_unittest.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:626: const char test_text[] = "TESTTESTTEST"; style: kTestText https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:631: base::ScopedFD sFd(fd); style: unit_hacker_style for variable names. https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:639: ssize_t len = write(fd, test_text, sizeof(test_text)); HANDLE_EINTR https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:648: ssize_t len = read(fd_check, buf, sizeof(buf)); HANDLE_EINTR https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:91: static bool ValidatePath(const char* path); Please, add comments describing what these functions do. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission_unittest.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:20: add also an anonymous namespace (so that the test functions don't get exported). https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:21: // Creation tests are DEATH tests as a bad permission causes termination. Please, use SANDBOX_TEST() instad. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:23: const char k_Path[] = "/tmp/good"; No "_", just kPath (valid a bunch of times in this file). https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:32: SANDBOX_DEATH_TEST(BrokerFilePermission, CreateBad, DEATH_MESSAGE("")) { Is there a message to specify ? Death test are notably unreliable (exit codes are a huge mess) without them (and this is where I fear that you'll have to revert your changes to bring back CHECK <<, right? :) - sorry. One generic message for everything should work. To avoid duplication (of the error string), you could expose a GetBlahErrorMessageForTests() that returns a const char* pointing to the string. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:54: void CheckPerm(BrokerFilePermission& perm, Non const reference are evil (and forbidden)! Looks like this can just be const. Otherwise, use pointer. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:54: void CheckPerm(BrokerFilePermission& perm, Please, add a comment to explain what this function does. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:58: const char* file_to_open; = NULL; https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:84: const int sync_flag = O_SYNC & ~O_DSYNC; style: kSyncFlag https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:90: for (int i = 2; i < 32; i++) { Why not start at 0? https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:135: CheckPerm(perm, k_Path, O_RDONLY, false); Add comments that CheckPerm() has to be the very last thing for ASSERT there to work as expected. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:200: ASSERT_TRUE(BrokerFilePermissionTester::ValidatePath("/path")); s/ASSERT/EXPECT in this function.
Done changes... https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:62: // |unlink_after_open| is set to point to true if the caller should unlink On 2014/11/26 01:02:31, jln wrote: > I would prefer "If not NULL, ...." Done. https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:95: // the compiler will still enforce no changes outside of the constructor. On 2014/11/26 01:02:31, jln wrote: > The issue is actually that std::vector requires copy-assignment (checked with > jyasskin) > Could you update the comment? > > Sad :( Done. https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_process_unittest.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:626: const char test_text[] = "TESTTESTTEST"; On 2014/11/26 01:02:31, jln wrote: > style: kTestText Done. https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:631: base::ScopedFD sFd(fd); On 2014/11/26 01:02:31, jln wrote: > style: unit_hacker_style for variable names. Done. https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:639: ssize_t len = write(fd, test_text, sizeof(test_text)); On 2014/11/26 01:02:31, jln wrote: > HANDLE_EINTR Done. https://chromiumcodereview.appspot.com/721553002/diff/270001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_process_unittest.cc:648: ssize_t len = read(fd_check, buf, sizeof(buf)); On 2014/11/26 01:02:31, jln wrote: > HANDLE_EINTR Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:91: static bool ValidatePath(const char* path); On 2014/11/26 01:02:31, jln wrote: > Please, add comments describing what these functions do. Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission_unittest.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:20: On 2014/11/26 01:02:31, jln wrote: > add also an anonymous namespace (so that the test functions don't get exported). Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:21: // Creation tests are DEATH tests as a bad permission causes termination. On 2014/11/26 01:02:32, jln wrote: > Please, use SANDBOX_TEST() instad. Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:23: const char k_Path[] = "/tmp/good"; On 2014/11/26 01:02:31, jln wrote: > No "_", just kPath (valid a bunch of times in this file). Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:32: SANDBOX_DEATH_TEST(BrokerFilePermission, CreateBad, DEATH_MESSAGE("")) { On 2014/11/26 01:02:31, jln wrote: > Is there a message to specify ? > > Death test are notably unreliable (exit codes are a huge mess) without them (and > this is where I fear that you'll have to revert your changes to bring back CHECK > <<, right? :) - sorry. > > One generic message for everything should work. To avoid duplication (of the > error string), you could expose a GetBlahErrorMessageForTests() that returns a > const char* pointing to the string. Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:54: void CheckPerm(BrokerFilePermission& perm, On 2014/11/26 01:02:31, jln wrote: > Non const reference are evil (and forbidden)! Looks like this can just be const. > Otherwise, use pointer. Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:54: void CheckPerm(BrokerFilePermission& perm, On 2014/11/26 01:02:31, jln wrote: > Please, add a comment to explain what this function does. Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:58: const char* file_to_open; On 2014/11/26 01:02:31, jln wrote: > = NULL; Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:84: const int sync_flag = O_SYNC & ~O_DSYNC; On 2014/11/26 01:02:32, jln wrote: > style: kSyncFlag Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:90: for (int i = 2; i < 32; i++) { On 2014/11/26 01:02:31, jln wrote: > Why not start at 0? This checks the additional open(2) flags. The O_RDONLY, O_WRONLY and O_RDWR flags are checked above and are needed in every test, so they are handled slightly differently. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:135: CheckPerm(perm, k_Path, O_RDONLY, false); On 2014/11/26 01:02:32, jln wrote: > Add comments that CheckPerm() has to be the very last thing for ASSERT there to > work as expected. Done. https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:200: ASSERT_TRUE(BrokerFilePermissionTester::ValidatePath("/path")); On 2014/11/26 01:02:31, jln wrote: > s/ASSERT/EXPECT in this function. Done.
lgtm, with a few more nits https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission_unittest.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:90: for (int i = 2; i < 32; i++) { On 2014/11/26 18:35:33, leecam wrote: > On 2014/11/26 01:02:31, jln wrote: > > Why not start at 0? > > This checks the additional open(2) flags. The O_RDONLY, O_WRONLY and O_RDWR > flags are checked above and are needed in every test, so they are handled > slightly differently. This needs to be documented. At the very least, make it a documented constant: const int kNumberOfBitsInOAccMode = 2; COMPILE_ASSERT(O_ACCMODE == (1 << kNumberOfBitsInOAccMode) - 1); https://chromiumcodereview.appspot.com/721553002/diff/350001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/350001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:24: size_t len = strlen(path); const https://chromiumcodereview.appspot.com/721553002/diff/350001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/350001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:103: return "Invalid BrokerFilePermission"; Please do (in the .cc) file: { static char kInvalidBrokerFileString[] = "..."; return kInvalidBrokerFileString; } Keeping this in headers could lead do string duplication. https://chromiumcodereview.appspot.com/721553002/diff/350001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission_unittest.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/350001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:171: CheckPerm(perm, kPath, O_RDONLY, false); // CheckPerm must be last Explain why: "For ASSERT* to successfully exit the test." (Also don't forget the final dot in comments). You can also reuse the comment used in other tests (put after the call to the subfunction): // Don't do anything here, so that ASSERT works in the subfunction as // expected.
https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission_unittest.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/330001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission_unittest.cc:90: for (int i = 2; i < 32; i++) { On 2014/11/26 19:49:06, jln wrote: > On 2014/11/26 18:35:33, leecam wrote: > > On 2014/11/26 01:02:31, jln wrote: > > > Why not start at 0? > > > > This checks the additional open(2) flags. The O_RDONLY, O_WRONLY and O_RDWR > > flags are checked above and are needed in every test, so they are handled > > slightly differently. > > This needs to be documented. > > At the very least, make it a documented constant: > > const int kNumberOfBitsInOAccMode = 2; > COMPILE_ASSERT(O_ACCMODE == (1 << kNumberOfBitsInOAccMode) - 1); Done. https://chromiumcodereview.appspot.com/721553002/diff/350001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.cc (right): https://chromiumcodereview.appspot.com/721553002/diff/350001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.cc:24: size_t len = strlen(path); On 2014/11/26 19:49:06, jln wrote: > const Done. https://chromiumcodereview.appspot.com/721553002/diff/350001/sandbox/linux/sy... File sandbox/linux/syscall_broker/broker_file_permission.h (right): https://chromiumcodereview.appspot.com/721553002/diff/350001/sandbox/linux/sy... sandbox/linux/syscall_broker/broker_file_permission.h:103: return "Invalid BrokerFilePermission"; On 2014/11/26 19:49:06, jln wrote: > Please do (in the .cc) file: > { > static char kInvalidBrokerFileString[] = "..."; > return kInvalidBrokerFileString; > } > > Keeping this in headers could lead do string duplication. Done.
The CQ bit was checked by leecam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/721553002/370001
Message was sent while issue was closed.
Committed patchset #20 (id:370001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/ad78f42ca0a83d590ff9f32050adfcb277fb34d6 Cr-Commit-Position: refs/heads/master@{#305894} |
