Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
997463002:
Add SetCapabilities for setting capabilities to an exact set. (Closed)
|
Created:
5 years, 9 months ago by rickyz (no longer on Chrome) Modified:
5 years, 9 months ago CC:
chromium-reviews, rickyz+watch_chromium.org, jln+watch_chromium.org, Mark Seaborn Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove sys/capability.h dependency from credentials.cc.
Also adds SetCapabilities and HasCapability functions for more fine-grained control over capabilities.
BUG=
Committed: https://crrev.com/966f039d6f3ae4374045bddd1fbb2c0b3b8a6a9a
Cr-Commit-Position: refs/heads/master@{#320878}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Respond to comments. #
Total comments: 4
Patch Set 3 : Respond to comments. #
Total comments: 1
Patch Set 4 : Get rid of sys/capability.h dependency. #
Total comments: 4
Patch Set 5 : Respond to comments. #
Total comments: 19
Patch Set 6 : Respond to more comments. #
Total comments: 6
Patch Set 7 : Don't expose capability constants. #Patch Set 8 : Update BUILD.gn. #Messages
Total messages: 48 (15 generated)
rickyz@chromium.org changed reviewers: + jln@chromium.org
Sorry for the delay here - I ended up agonizing a bit over whether to expose libcap details to users of credentials.h. Do you think there's any benefit to hiding the capabilities in sys/capabilities.h behind another enum?
Thanks Ricky! It would be nice to hide low-level libcap stuff, as I do think that we'll want to change the implementation at some point and it'll create breakage in any downstream users. In other words, it would make the API more stable. However it's not the highest priority, so we can start with this. https://chromiumcodereview.appspot.com/997463002/diff/1/sandbox/linux/service... File sandbox/linux/services/credentials.cc (right): https://chromiumcodereview.appspot.com/997463002/diff/1/sandbox/linux/service... sandbox/linux/services/credentials.cc:159: DCHECK_LE(0, proc_fd); if (caps.size <= 0) return false; https://chromiumcodereview.appspot.com/997463002/diff/1/sandbox/linux/service... sandbox/linux/services/credentials.cc:159: DCHECK_LE(0, proc_fd); if (caps.size <= 0) return false; https://chromiumcodereview.appspot.com/997463002/diff/1/sandbox/linux/service... sandbox/linux/services/credentials.cc:164: cap_flag_t flags[] = {CAP_EFFECTIVE, CAP_PERMITTED}; Why not include CAP_INHERITABLE ? It seems much safer and I can't think of any drawback since we're not using file capabilities anywhere. https://chromiumcodereview.appspot.com/997463002/diff/1/sandbox/linux/service... sandbox/linux/services/credentials.cc:166: PCHECK(cap_set_flag(cap.get(), flag, caps.size(), &caps[0], CAP_SET) == 0); caps.at(0) instead of caps[0] for good measure? https://chromiumcodereview.appspot.com/997463002/diff/1/sandbox/linux/service... sandbox/linux/services/credentials.cc:184: PCHECK(cap_get_flag(current_cap.get(), cap, CAP_EFFECTIVE, &value) == 0); Let's do a for() loop like above instead? It's more readable.
https://codereview.chromium.org/997463002/diff/1/sandbox/linux/services/crede... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/997463002/diff/1/sandbox/linux/services/crede... sandbox/linux/services/credentials.cc:159: DCHECK_LE(0, proc_fd); On 2015/03/10 17:03:34, jln wrote: > if (caps.size <= 0) return false; Oops, that's embarrassing - I placed the cap_set_flag loop inside an if instead so that callers can pass an empty list. https://codereview.chromium.org/997463002/diff/1/sandbox/linux/services/crede... sandbox/linux/services/credentials.cc:164: cap_flag_t flags[] = {CAP_EFFECTIVE, CAP_PERMITTED}; On 2015/03/10 17:03:34, jln wrote: > Why not include CAP_INHERITABLE ? It seems much safer and I can't think of any > drawback since we're not using file capabilities anywhere. Isn't not including CAP_INHERITABLE slightly more restrictive/safer? https://codereview.chromium.org/997463002/diff/1/sandbox/linux/services/crede... sandbox/linux/services/credentials.cc:166: PCHECK(cap_set_flag(cap.get(), flag, caps.size(), &caps[0], CAP_SET) == 0); On 2015/03/10 17:03:35, jln wrote: > caps.at(0) instead of caps[0] for good measure? Done. https://codereview.chromium.org/997463002/diff/1/sandbox/linux/services/crede... sandbox/linux/services/credentials.cc:184: PCHECK(cap_get_flag(current_cap.get(), cap, CAP_EFFECTIVE, &value) == 0); On 2015/03/10 17:03:34, jln wrote: > Let's do a for() loop like above instead? It's more readable. Done.
https://chromiumcodereview.appspot.com/997463002/diff/1/sandbox/linux/service... File sandbox/linux/services/credentials.cc (right): https://chromiumcodereview.appspot.com/997463002/diff/1/sandbox/linux/service... sandbox/linux/services/credentials.cc:164: cap_flag_t flags[] = {CAP_EFFECTIVE, CAP_PERMITTED}; On 2015/03/10 20:22:13, rickyz wrote: > On 2015/03/10 17:03:34, jln wrote: > > Why not include CAP_INHERITABLE ? It seems much safer and I can't think of any > > drawback since we're not using file capabilities anywhere. > Isn't not including CAP_INHERITABLE slightly more restrictive/safer? You're right that we don't want to set any cap in CAP_INHERITABLE, but we should make sure that CAP_INHERITABLE is empty. Otherwise using SetCapabilities to "drop privileges" isn't necessarily dropping everything.
https://chromiumcodereview.appspot.com/997463002/diff/20001/sandbox/linux/ser... File sandbox/linux/services/credentials.cc (right): https://chromiumcodereview.appspot.com/997463002/diff/20001/sandbox/linux/ser... sandbox/linux/services/credentials.cc:145: PCHECK(0 == cap_set_proc(cap.get())); We should probably just call Credentials::SetCapabilities(proc_fd, std::vector()) here, no? https://chromiumcodereview.appspot.com/997463002/diff/20001/sandbox/linux/ser... sandbox/linux/services/credentials.cc:160: CHECK(ThreadHelpers::IsSingleThreaded(proc_fd)); #if !defined(THREAD_SANITIZER)
https://chromiumcodereview.appspot.com/997463002/diff/1/sandbox/linux/service... File sandbox/linux/services/credentials.cc (right): https://chromiumcodereview.appspot.com/997463002/diff/1/sandbox/linux/service... sandbox/linux/services/credentials.cc:164: cap_flag_t flags[] = {CAP_EFFECTIVE, CAP_PERMITTED}; On 2015/03/10 22:12:11, jln wrote: > On 2015/03/10 20:22:13, rickyz wrote: > > On 2015/03/10 17:03:34, jln wrote: > > > Why not include CAP_INHERITABLE ? It seems much safer and I can't think of > any > > > drawback since we're not using file capabilities anywhere. > > Isn't not including CAP_INHERITABLE slightly more restrictive/safer? > > You're right that we don't want to set any cap in CAP_INHERITABLE, but we should > make sure that CAP_INHERITABLE is empty. Otherwise using SetCapabilities to > "drop privileges" isn't necessarily dropping everything. Ok, forget that: of course CAP_INHERITABLE is *already* empty after cap_init. Maybe add a comment?
https://codereview.chromium.org/997463002/diff/1/sandbox/linux/services/crede... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/997463002/diff/1/sandbox/linux/services/crede... sandbox/linux/services/credentials.cc:164: cap_flag_t flags[] = {CAP_EFFECTIVE, CAP_PERMITTED}; On 2015/03/10 22:18:40, jln wrote: > On 2015/03/10 22:12:11, jln wrote: > > On 2015/03/10 20:22:13, rickyz wrote: > > > On 2015/03/10 17:03:34, jln wrote: > > > > Why not include CAP_INHERITABLE ? It seems much safer and I can't think of > > any > > > > drawback since we're not using file capabilities anywhere. > > > Isn't not including CAP_INHERITABLE slightly more restrictive/safer? > > > > You're right that we don't want to set any cap in CAP_INHERITABLE, but we > should > > make sure that CAP_INHERITABLE is empty. Otherwise using SetCapabilities to > > "drop privileges" isn't necessarily dropping everything. > > Ok, forget that: of course CAP_INHERITABLE is *already* empty after cap_init. > > Maybe add a comment? Done. https://codereview.chromium.org/997463002/diff/20001/sandbox/linux/services/c... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/997463002/diff/20001/sandbox/linux/services/c... sandbox/linux/services/credentials.cc:145: PCHECK(0 == cap_set_proc(cap.get())); On 2015/03/10 22:14:22, jln wrote: > We should probably just call Credentials::SetCapabilities(proc_fd, > std::vector()) here, no? Ah, good point, done. https://codereview.chromium.org/997463002/diff/20001/sandbox/linux/services/c... sandbox/linux/services/credentials.cc:160: CHECK(ThreadHelpers::IsSingleThreaded(proc_fd)); On 2015/03/10 22:14:22, jln wrote: > #if !defined(THREAD_SANITIZER) Done.
lgtm https://chromiumcodereview.appspot.com/997463002/diff/40001/sandbox/linux/ser... File sandbox/linux/services/credentials.cc (right): https://chromiumcodereview.appspot.com/997463002/diff/40001/sandbox/linux/ser... sandbox/linux/services/credentials.cc:137: return false; n.b. We used to never let this function fail and this changes that.
The CQ bit was checked by rickyz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/997463002/40001
The CQ bit was unchecked by rickyz@chromium.org
The CQ bit was checked by rickyz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/997463002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Arg, is this a newlib issue? :(
On 2015/03/11 00:03:29, jln wrote: > Arg, is this a newlib issue? :( Looks like just broken chrome-infra. :/
On 2015/03/11 00:06:57, mdempsky wrote: > On 2015/03/11 00:03:29, jln wrote: > > Arg, is this a newlib issue? :( > > Looks like just broken chrome-infra. :/ Oops, yeah, this one looks like newlib: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... (Sorry, I was looking at http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/03/11 00:08:08, mdempsky wrote: > On 2015/03/11 00:06:57, mdempsky wrote: > > On 2015/03/11 00:03:29, jln wrote: > > > Arg, is this a newlib issue? :( > > > > Looks like just broken chrome-infra. :/ > > Oops, yeah, this one looks like newlib: > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > (Sorry, I was looking at > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Sorry for inconvenience. Would you mind to guard https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/lo... by #if !defined(OS_NACL_NONSFI) temporarily, so that the build should pass. Note that "Credentials" usage in the nacl_sandbox_linux.cc is also guarded, now.
On 2015/03/11 17:14:29, hidehiko wrote: > On 2015/03/11 00:08:08, mdempsky wrote: > > On 2015/03/11 00:06:57, mdempsky wrote: > > > On 2015/03/11 00:03:29, jln wrote: > > > > Arg, is this a newlib issue? :( > > > > > > Looks like just broken chrome-infra. :/ > > > > Oops, yeah, this one looks like newlib: > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > > > (Sorry, I was looking at > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > Sorry for inconvenience. > > Would you mind to guard > https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/lo... > by #if !defined(OS_NACL_NONSFI) temporarily, so that the build should pass. > > Note that "Credentials" usage in the nacl_sandbox_linux.cc is also guarded, now. No problem - we discussed this a little yesterday, and currently I'm planning to make this CL convert credentials.cc to use the capset system call directly for the one or two capabilities that we care about.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
I made the switch to using the capget/set syscalls directly as discussed, but be warned, it's quite ugly - open any suggestions on making it less ugly :-)
Thanks for doing this Ricky! I'll take a look very soon, but after skimming, this doesn't look too bad. One thing that would be great is more unit tests, but I realize that it's hard to do too much without exposing an API for permitted vs. effective etc.. https://codereview.chromium.org/997463002/diff/120001/sandbox/linux/services/... File sandbox/linux/services/syscall_wrappers.h (right): https://codereview.chromium.org/997463002/diff/120001/sandbox/linux/services/... sandbox/linux/services/syscall_wrappers.h:53: #ifndef _LINUX_CAPABILITY_VERSION_3 How about having this horror in system_headers/capability.h ? https://codereview.chromium.org/997463002/diff/120001/sandbox/linux/services/... sandbox/linux/services/syscall_wrappers.h:81: struct SANDBOX_EXPORT cap_data { Let's put in capability.h and forward declare here?
https://codereview.chromium.org/997463002/diff/120001/sandbox/linux/services/... File sandbox/linux/services/syscall_wrappers.h (right): https://codereview.chromium.org/997463002/diff/120001/sandbox/linux/services/... sandbox/linux/services/syscall_wrappers.h:53: #ifndef _LINUX_CAPABILITY_VERSION_3 On 2015/03/12 00:10:47, jln wrote: > How about having this horror in system_headers/capability.h ? Ah nice, didn't realize we had a place for this stuff. https://codereview.chromium.org/997463002/diff/120001/sandbox/linux/services/... sandbox/linux/services/syscall_wrappers.h:81: struct SANDBOX_EXPORT cap_data { On 2015/03/12 00:10:47, jln wrote: > Let's put in capability.h and forward declare here? Done.
Looks good in general, but it makes me a bit nervous. - Maybe at least one test that tests setting caps to CHROOT and try to chroot? - Ricky, Matthew, what would you think of keeping libcap around for unittests (and not build these unit tests in problematic configurations) so we could test our implementation against libcap? - Thanks to this change, credentials.cc should now build on Android (we almost certainly won't be able to test it there since there is no unprivileged CLONE_NEWUSER). In a future change, it would be nice to make sure things build there. https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/se... File sandbox/linux/services/credentials.cc (right): https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/se... sandbox/linux/services/credentials.cc:142: cap_data data[_LINUX_CAPABILITY_U32S_3]; Even here I would use "struct cap_data", given that it's not following the style guide for class / struct names and we're calling low-level kernel APIs. https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/se... sandbox/linux/services/credentials.cc:183: data[index].inheritable) & conidering .inheritable doesn't match the documentation. Update documentation? https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/se... File sandbox/linux/services/credentials.h (right): https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/se... sandbox/linux/services/credentials.h:44: // CAP_PERMITTED flag set for the given capability. The implementation also considers CAP_INHERITABLE. That's fine with me, but we need to match in the documentation. https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/se... File sandbox/linux/services/credentials_unittest.cc (right): https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/se... sandbox/linux/services/credentials_unittest.cc:162: SANDBOX_TEST(Credentials, SetCapabilities) { This test is very limited since we're only testing against our own implementation. Could you add a test like above that drops all caps except chroot and then tries to chroot? https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/se... sandbox/linux/services/credentials_unittest.cc:182: } // namespace. For paranoia, what would you think of keeping libcap around for non-problematic builds and use it in unit tests to cross-check our implementation? https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/se... File sandbox/linux/services/syscall_wrappers.h (right): https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/se... sandbox/linux/services/syscall_wrappers.h:55: SANDBOX_EXPORT int sys_capget(cap_hdr* hdrp, cap_data* datap); How about putting C-style "struct cap_hdr" for these C-style APIs? https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/sy... File sandbox/linux/system_headers/capability.h (right): https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/sy... sandbox/linux/system_headers/capability.h:16: #define _LINUX_CAPABILITY_U32S_3 2 Did you figure out what the comment "Backwardly compatible definition for source code - trapped in a 32-bit world. If you find you need this, please consider using libcap to untrap yourself..." means? https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/sy... sandbox/linux/system_headers/capability.h:40: struct cap_data { I'm pretty sure this should be fine, but I'm slightly worried about not having "exactly" what's in the kernel. I suspect / hope that C++ still guarantees that these struct will be represented the same in memory?
mdempsky@chromium.org changed reviewers: + mdempsky@chromium.org
https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/sy... File sandbox/linux/system_headers/capability.h (right): https://chromiumcodereview.appspot.com/997463002/diff/140001/sandbox/linux/sy... sandbox/linux/system_headers/capability.h:40: struct cap_data { On 2015/03/12 19:24:15, jln wrote: > I'm pretty sure this should be fine, but I'm slightly worried about not having > "exactly" what's in the kernel. > > I suspect / hope that C++ still guarantees that these struct will be represented > the same in memory? Another worry is if we write code assuming that cap_data will implicitly zero-initialize, but then we switch to using the kernel's headers and get cap_hdr/cap_data that won't.
https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... File sandbox/linux/services/credentials.h (right): https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... sandbox/linux/services/credentials.h:40: static bool SetCapabilities(int proc_fd, const std::vector<int>& caps) Would it be worth taking an enum class instead of ints for capabilities? Since we only support two of them, that should be easy. Then, users of this class don't need to include a clunky header to get CAP_SYS_CHROOT value.
https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:142: cap_data data[_LINUX_CAPABILITY_U32S_3]; On 2015/03/12 19:24:15, jln wrote: > Even here I would use "struct cap_data", given that it's not following the style > guide for class / struct names and we're calling low-level kernel APIs. Done. https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:183: data[index].inheritable) & On 2015/03/12 19:24:14, jln wrote: > conidering .inheritable doesn't match the documentation. Update documentation? Done. https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... File sandbox/linux/services/credentials.h (right): https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... sandbox/linux/services/credentials.h:40: static bool SetCapabilities(int proc_fd, const std::vector<int>& caps) On 2015/03/12 20:46:30, jln wrote: > Would it be worth taking an enum class instead of ints for capabilities? > > Since we only support two of them, that should be easy. Then, users of this > class don't need to include a clunky header to get CAP_SYS_CHROOT value. Done. https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... sandbox/linux/services/credentials.h:44: // CAP_PERMITTED flag set for the given capability. On 2015/03/12 19:24:15, jln wrote: > The implementation also considers CAP_INHERITABLE. That's fine with me, but we > need to match in the documentation. Oops, I updated the documentation. It bothers me a bit that this is inconsistent with SetCapabilities, but this seems like the more conservative behavior. https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... File sandbox/linux/services/credentials_unittest.cc (right): https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... sandbox/linux/services/credentials_unittest.cc:162: SANDBOX_TEST(Credentials, SetCapabilities) { On 2015/03/12 19:24:15, jln wrote: > This test is very limited since we're only testing against our own > implementation. > > Could you add a test like above that drops all caps except chroot and then tries > to chroot? Done. https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... sandbox/linux/services/credentials_unittest.cc:182: } // namespace. On 2015/03/12 19:24:15, jln wrote: > For paranoia, what would you think of keeping libcap around for non-problematic > builds and use it in unit tests to cross-check our implementation? Good idea, done. https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... File sandbox/linux/services/syscall_wrappers.h (right): https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/services/... sandbox/linux/services/syscall_wrappers.h:55: SANDBOX_EXPORT int sys_capget(cap_hdr* hdrp, cap_data* datap); On 2015/03/12 19:24:15, jln wrote: > How about putting C-style "struct cap_hdr" for these C-style APIs? Done. https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/system_he... File sandbox/linux/system_headers/capability.h (right): https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/system_he... sandbox/linux/system_headers/capability.h:16: #define _LINUX_CAPABILITY_U32S_3 2 On 2015/03/12 19:24:15, jln wrote: > Did you figure out what the comment > > "Backwardly compatible definition for source code - trapped in a 32-bit world. > If you find you need this, please consider using libcap to untrap yourself..." > means? http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ca0... has the hairy details. Apparently, when they made the change for the syscall to take an array of two struct cap_data instead of one, they just bumped the macros with the default values. I guess some people didn't use an array with _LINUX_CAPABILITY_U32S elements for cap_data, which caused the syscall to write past the end of cap-data. What a sad API :-( https://codereview.chromium.org/997463002/diff/140001/sandbox/linux/system_he... sandbox/linux/system_headers/capability.h:40: struct cap_data { On 2015/03/12 20:43:42, mdempsky wrote: > On 2015/03/12 19:24:15, jln wrote: > > I'm pretty sure this should be fine, but I'm slightly worried about not having > > "exactly" what's in the kernel. > > > > I suspect / hope that C++ still guarantees that these struct will be > represented > > the same in memory? > > Another worry is if we write code assuming that cap_data will implicitly > zero-initialize, but then we switch to using the kernel's headers and get > cap_hdr/cap_data that won't. That's a good point - switched to explicitly zeroing.
lgtm I would prefer if we could completely separate our LinuxCapability values from the actual Linux kernel values (and have a conversion function hidden in the implementation). Since it's reasonably unit tested, it's up to you. https://chromiumcodereview.appspot.com/997463002/diff/160001/sandbox/linux/se... File sandbox/linux/services/credentials.h (right): https://chromiumcodereview.appspot.com/997463002/diff/160001/sandbox/linux/se... sandbox/linux/services/credentials.h:28: kCapSysChroot = 18, This obviously depends on the kernel. I would prefer if we could hide these details in the implementation. In crendentials.cc, we can have a LinuxCapability to native cap conversion function, which will rely on the implementation header you wrote. Given that it is unit tested, I think we could live with this version, so up to you. https://chromiumcodereview.appspot.com/997463002/diff/160001/sandbox/linux/se... File sandbox/linux/services/credentials_unittest.cc (right): https://chromiumcodereview.appspot.com/997463002/diff/160001/sandbox/linux/se... sandbox/linux/services/credentials_unittest.cc:194: SANDBOX_TEST(Credentials, SetCapabilitiesAndChroot) { Might need DISABLE_ON_ASAN as above, no? https://chromiumcodereview.appspot.com/997463002/diff/160001/sandbox/linux/se... sandbox/linux/services/credentials_unittest.cc:218: static_cast<int>(LinuxCapability::kCapSysChroot) == CAP_SYS_CHROOT, I possible, I would rather have LinuxCapability values independent of actual capability values and have a conversion function in the implementation.
rickyz@chromium.org changed reviewers: + thestig@chromium.org
Thanks, adding thestig@ for chrome/installer/linux expected_deps changes. https://codereview.chromium.org/997463002/diff/160001/sandbox/linux/services/... File sandbox/linux/services/credentials.h (right): https://codereview.chromium.org/997463002/diff/160001/sandbox/linux/services/... sandbox/linux/services/credentials.h:28: kCapSysChroot = 18, On 2015/03/16 21:38:10, jln wrote: > This obviously depends on the kernel. I would prefer if we could hide these > details in the implementation. > > In crendentials.cc, we can have a LinuxCapability to native cap conversion > function, which will rely on the implementation header you wrote. > > Given that it is unit tested, I think we could live with this version, so up to > you. Done. https://codereview.chromium.org/997463002/diff/160001/sandbox/linux/services/... File sandbox/linux/services/credentials_unittest.cc (right): https://codereview.chromium.org/997463002/diff/160001/sandbox/linux/services/... sandbox/linux/services/credentials_unittest.cc:194: SANDBOX_TEST(Credentials, SetCapabilitiesAndChroot) { On 2015/03/16 21:38:10, jln wrote: > Might need DISABLE_ON_ASAN as above, no? I don't think it's needed in this case since I only ever chroot to /. https://codereview.chromium.org/997463002/diff/160001/sandbox/linux/services/... sandbox/linux/services/credentials_unittest.cc:218: static_cast<int>(LinuxCapability::kCapSysChroot) == CAP_SYS_CHROOT, On 2015/03/16 21:38:10, jln wrote: > I possible, I would rather have LinuxCapability values independent of actual > capability values and have a conversion function in the implementation. Done.
chrome/installer/linux lgtm
The CQ bit was checked by rickyz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org Link to the patchset: https://codereview.chromium.org/997463002/#ps180001 (title: "Don't expose capability constants.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/997463002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rickyz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, jln@chromium.org Link to the patchset: https://codereview.chromium.org/997463002/#ps200001 (title: "Update BUILD.gn.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/997463002/200001
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/966f039d6f3ae4374045bddd1fbb2c0b3b8a6a9a Cr-Commit-Position: refs/heads/master@{#320878}
Message was sent while issue was closed.
On 2015/03/17 07:24:40, I haz the power (commit-bot) wrote: > Patchset 8 (id:??) landed as > https://crrev.com/966f039d6f3ae4374045bddd1fbb2c0b3b8a6a9a > Cr-Commit-Position: refs/heads/master@{#320878} This breaks device build. ../../sandbox/linux/services/credentials.cc: In function 'int sandbox::{anonymous}::LinuxCapabilityToKernelValue(sandbox::LinuxCapability)': ../../sandbox/linux/services/credentials.cc:123:1: error: control reaches end of non-void function [-Werror=return-type] } ^
Message was sent while issue was closed.
On 2015/03/17 09:49:53, Ivan Podogov wrote: > On 2015/03/17 07:24:40, I haz the power (commit-bot) wrote: > > Patchset 8 (id:??) landed as > > https://crrev.com/966f039d6f3ae4374045bddd1fbb2c0b3b8a6a9a > > Cr-Commit-Position: refs/heads/master@{#320878} > > This breaks device build. > > ../../sandbox/linux/services/credentials.cc: In function 'int > sandbox::{anonymous}::LinuxCapabilityToKernelValue(sandbox::LinuxCapability)': > ../../sandbox/linux/services/credentials.cc:123:1: error: control reaches end of > non-void function [-Werror=return-type] > } > ^ Strange, is the compiler not aware that the LOG(FATAL) line does not return? I'll send a change to add a return under this. Out of curiosity, is there a trybot that would have caught this issue before the CL landed? |
