Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2869203003:
Add the SeatbeltExec classes to facilitate the V2 sandbox. (Closed)
|
Created:
3 years, 7 months ago by Greg K Modified:
3 years, 7 months ago Reviewers:
Robert Sesek CC:
chromium-reviews, mac-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the SeatbeltExec classes to facilitate the V2 sandbox.
This adds the SeatbeltExec classes and unit tests. These classes are
used to pipe data, such as the sandbox profile and parameters, from the
browser process to the helper process which launces the sandboxed
renderers.
BUG=689306
Review-Url: https://codereview.chromium.org/2869203003
Cr-Commit-Position: refs/heads/master@{#472658}
Committed: https://chromium.googlesource.com/chromium/src/+/169c4a19654509f37de918d8aee9df4ad343cc2a
Patch Set 1 #Patch Set 2 : Fix component build #Patch Set 3 : Fix component build #Patch Set 4 : Quiet logging from unit tests #
Total comments: 51
Patch Set 5 : Address review feedback #Patch Set 6 : Remove path which is not on 10.9 #
Total comments: 29
Patch Set 7 : Cleanup per review #
Total comments: 6
Patch Set 8 : Fix the last few nits. #
Messages
Total messages: 45 (33 generated)
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
kerrnel@chromium.org changed reviewers: + rsesek@chromium.org
PTAL. Thanks, Greg
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
In the CL description, change the word "land" to "add" or "create." CLs are _landed_, so the description should describe what is _landing_. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/BUILD.gn File sandbox/mac/BUILD.gn (right): https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/BUILD.gn#ne... sandbox/mac/BUILD.gn:37: proto_library("seatbelt_proto") { Restrict |visibility = [":*"]|. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/sandbox_mac... File sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc (right): https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/sandbox_mac... sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc:38: CHECK(exec_server.ApplySandboxProfile(params) == 0); CHECK_EQ https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/sandbox_mac... sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc:42: CHECK(stat(allowed_path.c_str(), &sb) == 0); CHECK_EQ, and the two lines below https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... File sandbox/mac/seatbelt_exec.cc (right): https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:26: close(pipe_[1]); Use IGNORE_EINTR. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:28: close(pipe_[0]); Same. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:51: LOG(FATAL) << "failed to serialize protobuf"; Why is this LOG(FATAL) and not a return -1? https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:63: // iov takes a non-const pointer. Can you use &str[0] ? https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:72: HANDLE_EINTR(writev(pipe_[1], iov, sizeof(iov) / sizeof(iov[0]))); Use arraysize(). https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:74: return false; PLOG(ERROR)? https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:91: LOG(ERROR) << "ReadString"; Not necessary to log here since ReadString logs internally. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:112: weak_params.push_back("CHROMIUM_PID"); What's with this magic variable? //sandbox generally shouldn't know about something that's so specific to //chrome or //content. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:130: char buffer[4096 * 4]; Use std::vector<char> instead. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:137: ssize_t read_length = readv(fd_, iov, sizeof(iov) / sizeof(iov[0])); HANDLE_EINTR https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:137: ssize_t read_length = readv(fd_, iov, sizeof(iov) / sizeof(iov[0])); Use arraysize(). https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:139: PLOG(ERROR) << "ReadString"; "readv" not "ReadString" https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... File sandbox/mac/seatbelt_exec.h (right): https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:17: class SEATBELT_EXPORT SeatbeltExecClient { These two classes need some high-level commentary. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:34: // Sends the parameters to the SeatbeltServer and returns the communication SeatbeltExecServer? https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:36: int GetSandboxFD(); This should probably be renamed to indicate that it does more than getting an FD. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:39: sandbox::mac::SandboxParams GetParamsForTesting() { return params_; } const& return value https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:46: bool got_fd_; Document the members. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:57: void AllowProcessExec(const std::string& exec_path); This interface is a little odd and I'm not sure it serves a point, since the caller could just set the EXECUTABLE_PATH variable themselves. Either way, this does end up defining a magic variable that isn't discussed at all in the interface documentation. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:59: // Read the parameters and policy from the client, and apply the sandbox. What does this return? https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:62: // Applies the given sandbox profile. What does this return? https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:66: // Reads from the global |fd_| and stores the data into a string. This does global?
Also, the trybots have test failures: [17914:771:0509/165037.446933:6822977355062:ERROR:seatbelt_exec.cc(120)] Failed to initialize sandbox: --1 line 5: unbound variable: process-exec* [17914:771:0509/165037.446970:6822977389679:FATAL:sandbox_mac_seatbelt_exec_unittest.cc(38)] Check failed: exec_server.ApplySandboxProfile(params) == 0. 0 sandbox_mac_unittests 0x000000010571286c base::debug::StackTrace::StackTrace(unsigned long) + 28 1 sandbox_mac_unittests 0x00000001057349a0 logging::LogMessage::~LogMessage() + 224 2 sandbox_mac_unittests 0x00000001056c3f70 sandbox::ServerTest() + 528 3 sandbox_mac_unittests 0x000000010581193d base::TestSuite::Run() + 109 4 sandbox_mac_unittests 0x000000010581fe86 base::LaunchUnitTests(int, char**, base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) + 134 5 sandbox_mac_unittests 0x00000001057fd909 main + 121 6 libdyld.dylib 0x00007fff808635fd start + 1 ../../sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc:55: Failure https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... File sandbox/mac/seatbelt_exec.cc (right): https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:120: LOG(ERROR) << "Failed to initialize sandbox: -" << rv << " " << error; No need for the negative sign in the string, apparently.
Description was changed from ========== Land the SeatbeltExec classes to facilitate the V2 sandbox. This lands the SeatbeltExec classes and unit tests. These classes are used to pipe data, such as the sandbox profile and parameters, from the browser process to the helper process which launces the sandboxed renderers. BUG=689306 ========== to ========== Add the SeatbeltExec classes to facilitate the V2 sandbox. This adds the SeatbeltExec classes and unit tests. These classes are used to pipe data, such as the sandbox profile and parameters, from the browser process to the helper process which launces the sandboxed renderers. BUG=689306 ==========
On 2017/05/10 17:53:28, Robert Sesek wrote: > Also, the trybots have test failures: > > [17914:771:0509/165037.446933:6822977355062:ERROR:seatbelt_exec.cc(120)] Failed > to initialize sandbox: --1 line 5: unbound variable: process-exec* > [17914:771:0509/165037.446970:6822977389679:FATAL:sandbox_mac_seatbelt_exec_unittest.cc(38)] > Check failed: exec_server.ApplySandboxProfile(params) == 0. > 0 sandbox_mac_unittests 0x000000010571286c > base::debug::StackTrace::StackTrace(unsigned long) + 28 > 1 sandbox_mac_unittests 0x00000001057349a0 > logging::LogMessage::~LogMessage() + 224 > 2 sandbox_mac_unittests 0x00000001056c3f70 sandbox::ServerTest() > + 528 > 3 sandbox_mac_unittests 0x000000010581193d > base::TestSuite::Run() + 109 > 4 sandbox_mac_unittests 0x000000010581fe86 > base::LaunchUnitTests(int, char**, base::Callback<int (), > (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) + 134 > 5 sandbox_mac_unittests 0x00000001057fd909 main + 121 > 6 libdyld.dylib 0x00007fff808635fd start + 1 > ../../sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc:55: Failure > > https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... > File sandbox/mac/seatbelt_exec.cc (right): > > https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... > sandbox/mac/seatbelt_exec.cc:120: LOG(ERROR) << "Failed to initialize sandbox: > -" << rv << " " << error; > No need for the negative sign in the string, apparently. Thanks for pointing out that trybot failure. It appears 10.9 has process-exec, not process-exec*. On other versions, process-exec is aliased to process-exec*, so we can just use (process-exec). Thanks, Greg
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/BUILD.gn File sandbox/mac/BUILD.gn (right): https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/BUILD.gn#ne... sandbox/mac/BUILD.gn:37: proto_library("seatbelt_proto") { On 2017/05/10 15:25:28, Robert Sesek wrote: > Restrict |visibility = [":*"]|. Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/sandbox_mac... File sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc (right): https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/sandbox_mac... sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc:38: CHECK(exec_server.ApplySandboxProfile(params) == 0); On 2017/05/10 15:25:28, Robert Sesek wrote: > CHECK_EQ Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/sandbox_mac... sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc:42: CHECK(stat(allowed_path.c_str(), &sb) == 0); On 2017/05/10 15:25:28, Robert Sesek wrote: > CHECK_EQ, and the two lines below Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... File sandbox/mac/seatbelt_exec.cc (right): https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:26: close(pipe_[1]); On 2017/05/10 15:25:28, Robert Sesek wrote: > Use IGNORE_EINTR. Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:28: close(pipe_[0]); On 2017/05/10 15:25:29, Robert Sesek wrote: > Same. Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:51: LOG(FATAL) << "failed to serialize protobuf"; On 2017/05/10 15:25:29, Robert Sesek wrote: > Why is this LOG(FATAL) and not a return -1? Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:63: // iov takes a non-const pointer. On 2017/05/10 15:25:28, Robert Sesek wrote: > Can you use &str[0] ? The compiler says that &str[0] is also a const char *. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:72: HANDLE_EINTR(writev(pipe_[1], iov, sizeof(iov) / sizeof(iov[0]))); On 2017/05/10 15:25:28, Robert Sesek wrote: > Use arraysize(). Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:74: return false; On 2017/05/10 15:25:29, Robert Sesek wrote: > PLOG(ERROR)? Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:91: LOG(ERROR) << "ReadString"; On 2017/05/10 15:25:28, Robert Sesek wrote: > Not necessary to log here since ReadString logs internally. Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:112: weak_params.push_back("CHROMIUM_PID"); On 2017/05/10 15:25:28, Robert Sesek wrote: > What's with this magic variable? //sandbox generally shouldn't know about > something that's so specific to //chrome or //content. The calling code will just add those as parameters. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:120: LOG(ERROR) << "Failed to initialize sandbox: -" << rv << " " << error; On 2017/05/10 17:53:28, Robert Sesek wrote: > No need for the negative sign in the string, apparently. Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:130: char buffer[4096 * 4]; On 2017/05/10 15:25:29, Robert Sesek wrote: > Use std::vector<char> instead. Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:137: ssize_t read_length = readv(fd_, iov, sizeof(iov) / sizeof(iov[0])); On 2017/05/10 15:25:28, Robert Sesek wrote: > Use arraysize(). Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:137: ssize_t read_length = readv(fd_, iov, sizeof(iov) / sizeof(iov[0])); On 2017/05/10 15:25:28, Robert Sesek wrote: > HANDLE_EINTR Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:137: ssize_t read_length = readv(fd_, iov, sizeof(iov) / sizeof(iov[0])); On 2017/05/10 15:25:28, Robert Sesek wrote: > HANDLE_EINTR Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.cc:139: PLOG(ERROR) << "ReadString"; On 2017/05/10 15:25:28, Robert Sesek wrote: > "readv" not "ReadString" Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... File sandbox/mac/seatbelt_exec.h (right): https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:17: class SEATBELT_EXPORT SeatbeltExecClient { On 2017/05/10 15:25:29, Robert Sesek wrote: > These two classes need some high-level commentary. Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:34: // Sends the parameters to the SeatbeltServer and returns the communication On 2017/05/10 15:25:29, Robert Sesek wrote: > SeatbeltExecServer? Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:36: int GetSandboxFD(); On 2017/05/10 15:25:29, Robert Sesek wrote: > This should probably be renamed to indicate that it does more than getting an > FD. Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:39: sandbox::mac::SandboxParams GetParamsForTesting() { return params_; } On 2017/05/10 15:25:29, Robert Sesek wrote: > const& return value Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:46: bool got_fd_; On 2017/05/10 15:25:29, Robert Sesek wrote: > Document the members. Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:57: void AllowProcessExec(const std::string& exec_path); On 2017/05/10 15:25:29, Robert Sesek wrote: > This interface is a little odd and I'm not sure it serves a point, since the > caller could just set the EXECUTABLE_PATH variable themselves. Either way, this > does end up defining a magic variable that isn't discussed at all in the > interface documentation. Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:59: // Read the parameters and policy from the client, and apply the sandbox. On 2017/05/10 15:25:29, Robert Sesek wrote: > What does this return? Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:62: // Applies the given sandbox profile. On 2017/05/10 15:25:29, Robert Sesek wrote: > What does this return? Done. https://codereview.chromium.org/2869203003/diff/60001/sandbox/mac/seatbelt_ex... sandbox/mac/seatbelt_exec.h:66: // Reads from the global |fd_| and stores the data into a string. This does On 2017/05/10 15:25:29, Robert Sesek wrote: > global? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/sandbox_ma... File sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc (right): https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/sandbox_ma... sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc:31: mac::SandboxPolicy policy; nit: consistency in referring to this type; it's sandbox::mac in the below test and the headers, but just mac:: here. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... File sandbox/mac/seatbelt_exec.cc (right): https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.cc:58: close(pipe_[1]); IGNORE_EINTR https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.cc:65: // iov takes a non-const pointer. &str[0] is probably const because the arg is. If you take a copy of the string (non-const ref) or a non-const pointer, this you can probably avoid copying. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.cc:84: close(fd_); IGNORE_EINTR https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.cc:124: memset(buffer.data(), '\0', buffer.size()); Drop the memset in favor of constructor #2 http://en.cppreference.com/w/cpp/container/vector/vector https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... File sandbox/mac/seatbelt_exec.h (right): https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:13: #include "seatbelt_export.h" nit: fully-qualified include paths https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:35: // Set the actual sandbox profile, using the scheme-like SBPL. nit: blank line above https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:36: void SetProfile(const char* policy); |const char*| -> |base::StringPiece| May also want to do the above Set*Parameter functions, so that an implicit conversion to std::string can be avoided for const char keys. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:37: // Sends the policy to the SeatbeltExecServer and returns the communication nit: blank line above https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:38: // FD. What is the caller supposed to do with the FD? https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:58: // the profiel, sandboxing the process. spelling: profile https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:65: // returns 0 on success, otherwise -1. Should this function and the one below just return bool then? https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:69: int ApplySandboxProfile(const mac::SandboxPolicy& sandbox_policy); Does this need to be public? https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:77: int fd_; Maybe ScopedFD? Maybe also for pipe_ above but it's probably a bit more clumsy there.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/sandbox_ma... File sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc (right): https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/sandbox_ma... sandbox/mac/sandbox_mac_seatbelt_exec_unittest.cc:31: mac::SandboxPolicy policy; On 2017/05/15 21:05:24, Robert Sesek wrote: > nit: consistency in referring to this type; it's sandbox::mac in the below test > and the headers, but just mac:: here. Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... File sandbox/mac/seatbelt_exec.cc (right): https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.cc:58: close(pipe_[1]); On 2017/05/15 21:05:24, Robert Sesek wrote: > IGNORE_EINTR Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.cc:65: // iov takes a non-const pointer. On 2017/05/15 21:05:24, Robert Sesek wrote: > &str[0] is probably const because the arg is. If you take a copy of the string > (non-const ref) or a non-const pointer, this you can probably avoid copying. Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.cc:84: close(fd_); On 2017/05/15 21:05:24, Robert Sesek wrote: > IGNORE_EINTR This no longer exists because it uses a ScopedFD. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.cc:124: memset(buffer.data(), '\0', buffer.size()); On 2017/05/15 21:05:24, Robert Sesek wrote: > Drop the memset in favor of constructor #2 > > http://en.cppreference.com/w/cpp/container/vector/vector Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... File sandbox/mac/seatbelt_exec.h (right): https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:13: #include "seatbelt_export.h" On 2017/05/15 21:05:24, Robert Sesek wrote: > nit: fully-qualified include paths Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:35: // Set the actual sandbox profile, using the scheme-like SBPL. On 2017/05/15 21:05:24, Robert Sesek wrote: > nit: blank line above Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:36: void SetProfile(const char* policy); On 2017/05/15 21:05:24, Robert Sesek wrote: > |const char*| -> |base::StringPiece| > > May also want to do the above Set*Parameter functions, so that an implicit > conversion to std::string can be avoided for const char keys. Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:37: // Sends the policy to the SeatbeltExecServer and returns the communication On 2017/05/15 21:05:24, Robert Sesek wrote: > nit: blank line above Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:38: // FD. On 2017/05/15 21:05:24, Robert Sesek wrote: > What is the caller supposed to do with the FD? Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:58: // the profiel, sandboxing the process. On 2017/05/15 21:05:24, Robert Sesek wrote: > spelling: profile Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:65: // returns 0 on success, otherwise -1. On 2017/05/15 21:05:24, Robert Sesek wrote: > Should this function and the one below just return bool then? Done. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:69: int ApplySandboxProfile(const mac::SandboxPolicy& sandbox_policy); On 2017/05/15 21:05:24, Robert Sesek wrote: > Does this need to be public? If this method isn't public, it's hard to test from the multiprocess unit test. And ifs called "*ForTesting," then it cannot be called from the InitializeSandbox() method. https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:77: int fd_; On 2017/05/15 21:05:24, Robert Sesek wrote: > Maybe ScopedFD? Maybe also for pipe_ above but it's probably a bit more clumsy > there. Done. And I do think it's a bit clumsy for the pipe_.
Just a few nits! https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... File sandbox/mac/seatbelt_exec.h (right): https://codereview.chromium.org/2869203003/diff/100001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:69: int ApplySandboxProfile(const mac::SandboxPolicy& sandbox_policy); On 2017/05/17 17:57:24, Greg K wrote: > On 2017/05/15 21:05:24, Robert Sesek wrote: > > Does this need to be public? > > If this method isn't public, it's hard to test from the multiprocess unit test. > And ifs called "*ForTesting," then it cannot be called from the > InitializeSandbox() method. Got it. How about making it static then? https://codereview.chromium.org/2869203003/diff/120001/sandbox/mac/seatbelt_e... File sandbox/mac/seatbelt_exec.h (right): https://codereview.chromium.org/2869203003/diff/120001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2869203003/diff/120001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:9: #include <unordered_map> unused https://codereview.chromium.org/2869203003/diff/120001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:66: explicit SeatbeltExecServer(int sandbox_fd); Comment that |sandbox_fd| should the result of SendProfileAndGetFD.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
PTAL. The trybot failure seems unrelated to I will re-run it. - Greg https://codereview.chromium.org/2869203003/diff/120001/sandbox/mac/seatbelt_e... File sandbox/mac/seatbelt_exec.h (right): https://codereview.chromium.org/2869203003/diff/120001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/17 18:35:26, Robert Sesek wrote: > 2017 Done. https://codereview.chromium.org/2869203003/diff/120001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:9: #include <unordered_map> On 2017/05/17 18:35:26, Robert Sesek wrote: > unused Done. https://codereview.chromium.org/2869203003/diff/120001/sandbox/mac/seatbelt_e... sandbox/mac/seatbelt_exec.h:66: explicit SeatbeltExecServer(int sandbox_fd); On 2017/05/17 18:35:26, Robert Sesek wrote: > Comment that |sandbox_fd| should the result of SendProfileAndGetFD. Done.
LGTM
The CQ bit was checked by kerrnel@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495061070392760,
"parent_rev": "a58f2e80c253a8586f789c6bfc77432015e47e40", "commit_rev":
"169c4a19654509f37de918d8aee9df4ad343cc2a"}
Message was sent while issue was closed.
Description was changed from ========== Add the SeatbeltExec classes to facilitate the V2 sandbox. This adds the SeatbeltExec classes and unit tests. These classes are used to pipe data, such as the sandbox profile and parameters, from the browser process to the helper process which launces the sandboxed renderers. BUG=689306 ========== to ========== Add the SeatbeltExec classes to facilitate the V2 sandbox. This adds the SeatbeltExec classes and unit tests. These classes are used to pipe data, such as the sandbox profile and parameters, from the browser process to the helper process which launces the sandboxed renderers. BUG=689306 Review-Url: https://codereview.chromium.org/2869203003 Cr-Commit-Position: refs/heads/master@{#472658} Committed: https://chromium.googlesource.com/chromium/src/+/169c4a19654509f37de918d8aee9... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/169c4a19654509f37de918d8aee9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
