From e9f7293e21c5fae7f3368069e63c342a1dd79a8d Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Fri, 17 Jul 2020 03:20:18 -0700 Subject: [PATCH] Fix ptrace_hook dependency graph PiperOrigin-RevId: 321748143 Change-Id: Idb453054b78e932ce13c5f44f7d408cc0f9c31f2 --- cmake/libunwind/Download.cmake | 1 + sandboxed_api/bazel/external/libunwind.BUILD | 4 ++- sandboxed_api/sandbox2/BUILD.bazel | 36 +++++++++----------- sandboxed_api/sandbox2/CMakeLists.txt | 3 -- sandboxed_api/sandbox2/forkserver.cc | 15 ++------ sandboxed_api/sandbox2/stack_trace.h | 1 - sandboxed_api/sandbox2/unwind/BUILD.bazel | 8 +++-- sandboxed_api/sandbox2/unwind/CMakeLists.txt | 1 + sandboxed_api/sandbox2/unwind/unwind.cc | 18 +++++++--- sandboxed_api/sandbox2/unwind/unwind.h | 3 +- 10 files changed, 43 insertions(+), 47 deletions(-) diff --git a/cmake/libunwind/Download.cmake b/cmake/libunwind/Download.cmake index 9eaf58c..d7c9981 100644 --- a/cmake/libunwind/Download.cmake +++ b/cmake/libunwind/Download.cmake @@ -136,6 +136,7 @@ foreach(wrapped _wrapped "") ) target_link_libraries(unwind_ptrace${wrapped} PRIVATE sapi::base + sandbox2::ptrace_hook ) endforeach() target_compile_definitions(unwind_ptrace_wrapped PUBLIC diff --git a/sandboxed_api/bazel/external/libunwind.BUILD b/sandboxed_api/bazel/external/libunwind.BUILD index d396a4d..fb303f1 100644 --- a/sandboxed_api/bazel/external/libunwind.BUILD +++ b/sandboxed_api/bazel/external/libunwind.BUILD @@ -155,7 +155,9 @@ filegroup( ]] if do_wrap else [] ), visibility = ["//visibility:public"], - deps = [":included_sources"], + deps = [":included_sources"] + ( + ["@com_google_sandboxed_api//sandboxed_api/sandbox2/unwind:ptrace_hook"] if do_wrap else [] + ), # This forces a link failure in any target that depends on both # unwind-ptrace and unwind-ptrace-wrapped. alwayslink = 1, diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index 2e562a3..81916d0 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/sandboxed_api/sandbox2/BUILD.bazel @@ -271,13 +271,11 @@ cc_library( visibility = ["//visibility:public"], deps = [ ":client", - ":executor", ":comms", + ":executor", + ":forkserver", ":forkserver_cc_proto", ":global_forkserver", - ":sanitizer", - ":violation_cc_proto", - ":forkserver", ":ipc", ":limits", ":logsink", @@ -287,20 +285,10 @@ cc_library( ":policy", ":regs", ":result", + ":sanitizer", ":syscall", ":util", - "@com_google_absl//absl/base:core_headers", - "@com_google_absl//absl/container:flat_hash_map", - "@com_google_absl//absl/container:flat_hash_set", - "//sandboxed_api/util:flags", - "@com_google_absl//absl/memory", - "@com_google_absl//absl/status", - "@com_google_absl//absl/strings", - "@com_google_absl//absl/strings:str_format", - "@com_google_absl//absl/synchronization", - "@com_google_absl//absl/time", - "@com_google_absl//absl/types:optional", - "@org_kernel_libcap//:libcap", + ":violation_cc_proto", "//sandboxed_api/sandbox2/network_proxy:client", "//sandboxed_api/sandbox2/network_proxy:filtering", "//sandboxed_api/sandbox2/network_proxy:server", @@ -309,12 +297,21 @@ cc_library( "//sandboxed_api/sandbox2/util:bpf_helper", "//sandboxed_api/sandbox2/util:file_base", "//sandboxed_api/sandbox2/util:fileops", + "//sandboxed_api/util:flags", "//sandboxed_api/util:raw_logging", "//sandboxed_api/util:status", "//sandboxed_api/util:statusor", - - # Do not remove this dependency as it defines ptrace_wrapped. - "//sandboxed_api/sandbox2/unwind:ptrace_hook", # buildcleaner: keep + "@com_google_absl//absl/base:core_headers", + "@com_google_absl//absl/container:flat_hash_map", + "@com_google_absl//absl/container:flat_hash_set", + "@com_google_absl//absl/memory", + "@com_google_absl//absl/status", + "@com_google_absl//absl/strings", + "@com_google_absl//absl/strings:str_format", + "@com_google_absl//absl/synchronization", + "@com_google_absl//absl/time", + "@com_google_absl//absl/types:optional", + "@org_kernel_libcap//:libcap", ], ) @@ -372,7 +369,6 @@ cc_library( ":syscall", ":util", "//sandboxed_api/sandbox2/unwind", - "//sandboxed_api/sandbox2/unwind:ptrace_hook", "//sandboxed_api/sandbox2/unwind:unwind_cc_proto", "//sandboxed_api/sandbox2/util:bpf_helper", "//sandboxed_api/sandbox2/util:fileops", diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index 2e400cf..cc36252 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -291,7 +291,6 @@ target_link_libraries(sandbox2_sandbox2 sandbox2::network_proxy_client sandbox2::notify sandbox2::policy - sandbox2::ptrace_hook sandbox2::regs sandbox2::result sandbox2::sanitizer @@ -363,12 +362,10 @@ target_link_libraries(sandbox2_forkserver PRIVATE sandbox2::forkserver_proto sandbox2::namespace sandbox2::policy - sandbox2::ptrace_hook sandbox2::strerror sandbox2::sanitizer sandbox2::syscall sandbox2::unwind - sandbox2::unwind_proto sandbox2::util sapi::base sapi::raw_logging diff --git a/sandboxed_api/sandbox2/forkserver.cc b/sandboxed_api/sandbox2/forkserver.cc index 21dca46..e9e8172 100644 --- a/sandboxed_api/sandbox2/forkserver.cc +++ b/sandboxed_api/sandbox2/forkserver.cc @@ -50,7 +50,6 @@ #include "sandboxed_api/sandbox2/policy.h" #include "sandboxed_api/sandbox2/sanitizer.h" #include "sandboxed_api/sandbox2/syscall.h" -#include "sandboxed_api/sandbox2/unwind/ptrace_hook.h" #include "sandboxed_api/sandbox2/unwind/unwind.h" #include "sandboxed_api/sandbox2/unwind/unwind.pb.h" #include "sandboxed_api/sandbox2/util.h" @@ -346,18 +345,8 @@ void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd, c.EnableSandbox(); if (request.mode() == FORKSERVER_FORK_JOIN_SANDBOX_UNWIND) { - UnwindSetup pb_setup; - if (!client_comms.RecvProtoBuf(&pb_setup)) { - exit(1); - } - - std::string data = pb_setup.regs(); - InstallUserRegs(data.c_str(), data.length()); - ArmPtraceEmulation(); - RunLibUnwindAndSymbolizer(pb_setup.pid(), &client_comms, - pb_setup.default_max_frames(), - pb_setup.delim()); - exit(0); + exit(RunLibUnwindAndSymbolizer(&client_comms) ? EXIT_SUCCESS + : EXIT_FAILURE); } else { ExecuteProcess(execve_fd, argv, envp); } diff --git a/sandboxed_api/sandbox2/stack_trace.h b/sandboxed_api/sandbox2/stack_trace.h index 80bfc05..ddb5fee 100644 --- a/sandboxed_api/sandbox2/stack_trace.h +++ b/sandboxed_api/sandbox2/stack_trace.h @@ -27,7 +27,6 @@ #include "sandboxed_api/sandbox2/mounts.h" #include "sandboxed_api/sandbox2/policy.h" #include "sandboxed_api/sandbox2/regs.h" -#include "sandboxed_api/sandbox2/unwind/unwind.pb.h" namespace sandbox2 { diff --git a/sandboxed_api/sandbox2/unwind/BUILD.bazel b/sandboxed_api/sandbox2/unwind/BUILD.bazel index bbf0a76..1098941 100644 --- a/sandboxed_api/sandbox2/unwind/BUILD.bazel +++ b/sandboxed_api/sandbox2/unwind/BUILD.bazel @@ -12,20 +12,21 @@ # See the License for the specific language governing permissions and # limitations under the License. +load("//sandboxed_api/bazel:build_defs.bzl", "sapi_platform_copts") +load("//sandboxed_api/bazel:proto.bzl", "sapi_proto_library") + package(default_visibility = [ "//sandboxed_api/sandbox2:__subpackages__", ]) licenses(["notice"]) # Apache 2.0 -load("//sandboxed_api/bazel:build_defs.bzl", "sapi_platform_copts") -load("//sandboxed_api/bazel:proto.bzl", "sapi_proto_library") - cc_library( name = "ptrace_hook", srcs = ["ptrace_hook.cc"], hdrs = ["ptrace_hook.h"], copts = sapi_platform_copts(), + visibility = ["@org_gnu_libunwind//:__subpackages__"], ) cc_library( @@ -50,6 +51,7 @@ cc_library( ] ]), deps = [ + ":ptrace_hook", ":unwind_cc_proto", "//sandboxed_api/sandbox2:comms", "//sandboxed_api/sandbox2/util:maps_parser", diff --git a/sandboxed_api/sandbox2/unwind/CMakeLists.txt b/sandboxed_api/sandbox2/unwind/CMakeLists.txt index 66233f4..01f818b 100644 --- a/sandboxed_api/sandbox2/unwind/CMakeLists.txt +++ b/sandboxed_api/sandbox2/unwind/CMakeLists.txt @@ -32,6 +32,7 @@ target_link_libraries(sandbox2_unwind PRIVATE sandbox2::comms sandbox2::maps_parser sandbox2::minielf + sandbox2::ptrace_hook sandbox2::strerror sandbox2::unwind_proto sapi::base diff --git a/sandboxed_api/sandbox2/unwind/unwind.cc b/sandboxed_api/sandbox2/unwind/unwind.cc index 781156c..7d57519 100644 --- a/sandboxed_api/sandbox2/unwind/unwind.cc +++ b/sandboxed_api/sandbox2/unwind/unwind.cc @@ -27,6 +27,7 @@ #include "absl/strings/str_cat.h" #include "libunwind-ptrace.h" #include "sandboxed_api/sandbox2/comms.h" +#include "sandboxed_api/sandbox2/unwind/ptrace_hook.h" #include "sandboxed_api/sandbox2/unwind/unwind.pb.h" #include "sandboxed_api/sandbox2/util/maps_parser.h" #include "sandboxed_api/sandbox2/util/minielf.h" @@ -116,18 +117,27 @@ void GetIPList(pid_t pid, std::vector* ips, int max_frames) { _UPT_destroy(ui); } -void RunLibUnwindAndSymbolizer(pid_t pid, Comms* comms, int max_frames, - const std::string& delim) { +bool RunLibUnwindAndSymbolizer(Comms* comms) { + UnwindSetup pb_setup; + if (!comms->RecvProtoBuf(&pb_setup)) { + return false; + } + + std::string data = pb_setup.regs(); + InstallUserRegs(data.c_str(), data.length()); + ArmPtraceEmulation(); + UnwindResult msg; std::string stack_trace; std::vector ips; - RunLibUnwindAndSymbolizer(pid, &stack_trace, &ips, max_frames, delim); + RunLibUnwindAndSymbolizer(pb_setup.pid(), &stack_trace, &ips, + pb_setup.default_max_frames(), pb_setup.delim()); for (const auto& i : ips) { msg.add_ip(i); } msg.set_stacktrace(stack_trace.c_str(), stack_trace.size()); - comms->SendProtoBuf(msg); + return comms->SendProtoBuf(msg); } void RunLibUnwindAndSymbolizer(pid_t pid, std::string* stack_trace_out, diff --git a/sandboxed_api/sandbox2/unwind/unwind.h b/sandboxed_api/sandbox2/unwind/unwind.h index 00de414..eae539f 100644 --- a/sandboxed_api/sandbox2/unwind/unwind.h +++ b/sandboxed_api/sandbox2/unwind/unwind.h @@ -26,8 +26,7 @@ namespace sandbox2 { void GetIPList(pid_t pid, std::vector* ips, int max_frames); -void RunLibUnwindAndSymbolizer(pid_t pid, Comms* comms, int max_frames, - const std::string& delim); +bool RunLibUnwindAndSymbolizer(Comms* comms); void RunLibUnwindAndSymbolizer(pid_t pid, std::string* stack_trace_out, std::vector* ips, int max_frames,