From 5e4a581391cbeb37742f31c2c8b2dc9bdb8daadf Mon Sep 17 00:00:00 2001 From: Dom Del Nano Date: Tue, 1 Aug 2023 15:32:04 -0700 Subject: [PATCH] Fix virtual to binary addr conversion for processes that have an uncommon virtual memory mapping (#1637) Summary: Fix virtual to binary addr conversion for processes that have an uncommon virtual memory mapping Our previous virtual to binary address conversion logic assumed that the first offset within `/proc/$PID/maps` was the correct offset to apply for PIE binaries. There are certain cases, such as when an unlimited stack ulimit is applied, where this assumption doesn't hold true (see the linked issue before for more details). This change adjusts our conversion logic to take into account the correct `/proc/$PID/maps` entry so address conversion works in all known cases. Relevant Issues: #1630 Type of change: /kind bug Test Plan: Verified the following: - [x] New test verifies the status quo case as well as the situation reported in #1630 - [x] Verified `perf_profiler_bpf_test` passes when the perf profiler uses the ELF symbolizer --------- Signed-off-by: Dom Del Nano --- .../testing/test_utils/container_runner.h | 5 ++ src/stirling/obj_tools/BUILD.bazel | 13 ++++ src/stirling/obj_tools/address_converter.cc | 45 +++++++++++--- .../obj_tools/address_converter_test.cc | 58 +++++++++++++++++ .../obj_tools/testdata/containers/BUILD.bazel | 48 ++++++++++++++ .../containers/vaddr_convert_self_func.cc | 62 +++++++++++++++++++ .../vaddr_convert_self_func_container.h | 38 ++++++++++++ 7 files changed, 260 insertions(+), 9 deletions(-) create mode 100644 src/stirling/obj_tools/address_converter_test.cc create mode 100644 src/stirling/obj_tools/testdata/containers/BUILD.bazel create mode 100644 src/stirling/obj_tools/testdata/containers/vaddr_convert_self_func.cc create mode 100644 src/stirling/obj_tools/testdata/containers/vaddr_convert_self_func_container.h diff --git a/src/common/testing/test_utils/container_runner.h b/src/common/testing/test_utils/container_runner.h index b72cd1a71cd..33dbc612e4a 100644 --- a/src/common/testing/test_utils/container_runner.h +++ b/src/common/testing/test_utils/container_runner.h @@ -66,6 +66,11 @@ class ContainerRunner { */ void Wait(bool close_pipe = true); + /** + * Returns the exit status of the container. + */ + int GetStatus() { return podman_.GetStatus(); } + /** * Returns the stdout of the container. Needs to be combined with the full output from Run * to ensure the entire result is present. diff --git a/src/stirling/obj_tools/BUILD.bazel b/src/stirling/obj_tools/BUILD.bazel index e8a84b1679d..397a4ac9d64 100644 --- a/src/stirling/obj_tools/BUILD.bazel +++ b/src/stirling/obj_tools/BUILD.bazel @@ -69,6 +69,19 @@ pl_cc_test( ], ) +pl_cc_test( + name = "address_converter_test", + srcs = ["address_converter_test.cc"], + tags = [ + "requires_bpf", + ], + deps = [ + ":cc_library", + "//src/stirling/obj_tools/testdata/containers:vaddr_convert_self_func_container", + "//src/stirling/testing:cc_library", + ], +) + pl_cc_test( name = "dwarf_reader_test", srcs = ["dwarf_reader_test.cc"], diff --git a/src/stirling/obj_tools/address_converter.cc b/src/stirling/obj_tools/address_converter.cc index 440d1ca56cd..c764a3b3c8f 100644 --- a/src/stirling/obj_tools/address_converter.cc +++ b/src/stirling/obj_tools/address_converter.cc @@ -17,15 +17,20 @@ */ #include +#include #include +#include "src/common/fs/fs_wrapper.h" #include "src/common/system/proc_parser.h" +#include "src/common/system/proc_pid_path.h" #include "src/stirling/obj_tools/address_converter.h" namespace px { namespace stirling { namespace obj_tools { +using ProcSMaps = system::ProcParser::ProcessSMaps; + uint64_t ElfAddressConverter::VirtualAddrToBinaryAddr(uint64_t virtual_addr) const { return virtual_addr + virtual_to_binary_addr_offset_; } @@ -34,22 +39,31 @@ uint64_t ElfAddressConverter::BinaryAddrToVirtualAddr(uint64_t binary_addr) cons return binary_addr - virtual_to_binary_addr_offset_; } +StatusOr GetProcMapsIndexForBinary(const std::string& binary_path, + const std::vector& map_entries) { + for (const auto& [idx, entry] : Enumerate(map_entries)) { + if (absl::EndsWith(binary_path, entry.pathname)) { + return idx; + } + } + return error::NotFound(""); +} + /** * The calculated offset is used to convert between virtual addresses (eg. the address you * would get from a function pointer) and "binary" addresses (i.e. the address that `nm` would * display for a given function). * * This conversion is non-trivial and requires information from both the ELF file of the binary in - * question, as well as the /proc/PID/maps file for the PID of the process in question. + * question, as well as the /proc/$PID/maps file for the PID of the process in question. * * For non-PIE executables, this conversion is trivial as the virtual addresses in the ELF file are * used directly when loading. * - * However, for PIE, the loaded virtual address can be whatever. So to calculate the offset we look - * at the first loadable segment in the ELF file and compare it to the first entry in the - * /proc/PID/maps file to see how the loader changed the virtual address. This works because the - * loader guarantees that the relative offsets of the different segments remain the same, regardless - * of where in virtual address space it ends up putting the segment. + * However, for PIE, the loaded virtual address can be whatever. To calculate the offset we must + * find the /proc/$PID/maps entry that corresponds to the given process's executable (entry that + * matches /proc/$PID/exe) and use that entry's virtual memory offset to find the binary + * address. * **/ StatusOr> ElfAddressConverter::Create(ElfReader* elf_reader, @@ -64,16 +78,29 @@ StatusOr> ElfAddressConverter::Create(ElfRe } system::ProcParser parser; std::vector map_entries; - // This is a little inefficient as we only need the first entry. PX_RETURN_IF_ERROR(parser.ParseProcPIDMaps(pid, &map_entries)); if (map_entries.size() < 1) { return Status( statuspb::INTERNAL, absl::Substitute("ElfAddressConverter::Create: Failed to parse /proc/$0/maps", pid)); } - const auto mapped_virt_addr = map_entries[0].vmem_start; + + PX_ASSIGN_OR_RETURN(const auto proc_exe, parser.GetExePath(pid)); + // Prior to pixie#1630, we believed that a process's VMA space would always have its executable + // at the first (lowest) virtual memory address. This isn't always the case, however, if we fail + // to match against a /proc/$PID/maps entry, default to the first one. + auto map_entry = map_entries[0]; + auto idx_status = GetProcMapsIndexForBinary(proc_exe, map_entries); + if (idx_status.ok()) { + map_entry = map_entries[idx_status.ConsumeValueOrDie()]; + } else { + LOG(WARNING) << absl::Substitute( + "Failed to find match for $0 in /proc/$1/maps. Defaulting to the first entry", + proc_exe.string(), pid); + } + const auto mapped_virt_addr = map_entry.vmem_start; uint64_t mapped_offset; - if (!absl::SimpleHexAtoi(map_entries[0].offset, &mapped_offset)) { + if (!absl::SimpleHexAtoi(map_entry.offset, &mapped_offset)) { return Status(statuspb::INTERNAL, absl::Substitute( "ElfAddressConverter::Create: Failed to parse offset in /proc/$0/maps", pid)); diff --git a/src/stirling/obj_tools/address_converter_test.cc b/src/stirling/obj_tools/address_converter_test.cc new file mode 100644 index 00000000000..ed0294b7225 --- /dev/null +++ b/src/stirling/obj_tools/address_converter_test.cc @@ -0,0 +1,58 @@ +/* + * Copyright 2018- The Pixie Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include "src/stirling/obj_tools/address_converter.h" + +#include "src/stirling/obj_tools/testdata/containers/vaddr_convert_self_func_container.h" +#include "src/stirling/testing/common.h" + +namespace px { +namespace stirling { +namespace obj_tools { + +TEST(ElfAddressConverterTest, VirtualAddrToBinaryAddr) { + VaddrConvertSelfFuncContainer container; + ASSERT_OK(container.Run()); + + int status = -1; + testing::Timeout t(std::chrono::minutes{1}); + while (status == -1 && !t.TimedOut()) { + status = container.GetStatus(); + std::this_thread::sleep_for(std::chrono::milliseconds{200}); + } + EXPECT_EQ(0, status); +} + +TEST(ElfAddressConverterTest, VirtualAddrToBinaryAddrForReorderedVirtualMemoryMappings) { + // Setting an unlimited stack size ulimit causes the VMAs of a process to be reordered and + // caused a previous crash (as described in https://github.com/pixie-io/pixie/issues/1630). + VaddrConvertSelfFuncContainer container; + ASSERT_OK(container.Run(std::chrono::seconds{5}, {"--ulimit=stack=-1"})); + + int status = -1; + testing::Timeout t(std::chrono::minutes{1}); + while (status == -1 && !t.TimedOut()) { + status = container.GetStatus(); + std::this_thread::sleep_for(std::chrono::milliseconds{200}); + } + EXPECT_EQ(0, status); +} + +} // namespace obj_tools +} // namespace stirling +} // namespace px diff --git a/src/stirling/obj_tools/testdata/containers/BUILD.bazel b/src/stirling/obj_tools/testdata/containers/BUILD.bazel new file mode 100644 index 00000000000..576620f0393 --- /dev/null +++ b/src/stirling/obj_tools/testdata/containers/BUILD.bazel @@ -0,0 +1,48 @@ +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# SPDX-License-Identifier: Apache-2.0 + +load("@io_bazel_rules_docker//cc:image.bzl", "cc_image") +load("//bazel:pl_build_system.bzl", "pl_cc_binary", "pl_cc_test_library") + +package(default_visibility = ["//src/stirling/obj_tools:__subpackages__"]) + +pl_cc_binary( + name = "vaddr_convert_self_func", + srcs = ["vaddr_convert_self_func.cc"], + deps = [ + "//src/stirling/obj_tools:cc_library", + ], +) + +cc_image( + name = "vaddr_convert_self_func_image", + base = "//:pl_cc_base_debug_image", + binary = ":vaddr_convert_self_func", +) + +pl_cc_test_library( + name = "vaddr_convert_self_func_container", + srcs = glob( + ["*.cc"], + exclude = [ + "vaddr_convert_self_func.cc", + ], + ), + hdrs = glob( + [ + "*.h", + ], + ), + data = [ + ":vaddr_convert_self_func_image.tar", + ], + deps = ["//src/common/testing/test_utils:cc_library"], +) diff --git a/src/stirling/obj_tools/testdata/containers/vaddr_convert_self_func.cc b/src/stirling/obj_tools/testdata/containers/vaddr_convert_self_func.cc new file mode 100644 index 00000000000..3fd4d6d4a0f --- /dev/null +++ b/src/stirling/obj_tools/testdata/containers/vaddr_convert_self_func.cc @@ -0,0 +1,62 @@ +/* + * Copyright 2018- The Pixie Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include "src/stirling/obj_tools/address_converter.h" +#include "src/stirling/utils/proc_path_tools.h" + +// Using extern C to avoid name mangling since ElfReader must be able to address this +// by its symbol name. +extern "C" { +void TestFunc() {} + +} // extern "C" + +using px::stirling::GetSelfPath; +using px::stirling::obj_tools::ElfAddressConverter; +using px::stirling::obj_tools::ElfReader; +using px::stirling::obj_tools::SymbolMatchType; + +// This utility performs an assertion that the ElfAddressConverter::VirtualAddrToBinaryAddr function +// returns the correct (consistent with ElfReader and `nm` cli output) binary address for a given +// function. This is used to test our address conversion logic when a PIE binary is memory mapped +// differently from the common scenario (where the process's ELF segments are mapped at the +// lowest VMA), such as when an unlimited stack size ulimit is set on a process. +int main() { + LOG(INFO) << "Running"; + + // This sleep is required otherwise when it is run inside a container (via ContainerRunner) we + // will fail to detect the child process's pid during the test case that uses this. + sleep(5); + + std::filesystem::path self_path = GetSelfPath().ValueOrDie(); + PX_ASSIGN_OR(auto elf_reader, ElfReader::Create(self_path.string()), return -1); + PX_ASSIGN_OR(std::vector syms, + elf_reader->ListFuncSymbols("TestFunc", SymbolMatchType::kSubstr)); + + PX_ASSIGN_OR(auto converter, ElfAddressConverter::Create(elf_reader.get(), getpid()), return -1); + auto symbol_addr = converter->VirtualAddrToBinaryAddr(reinterpret_cast(&TestFunc)); + + auto expected_addr = syms[0].address; + if (symbol_addr != expected_addr) { + LOG(ERROR) << absl::Substitute( + "Expected ElfAddressConverter address=$0 to match binary address=$1", symbol_addr, + expected_addr); + return -1; + } + return 0; +} diff --git a/src/stirling/obj_tools/testdata/containers/vaddr_convert_self_func_container.h b/src/stirling/obj_tools/testdata/containers/vaddr_convert_self_func_container.h new file mode 100644 index 00000000000..f5b7f463b87 --- /dev/null +++ b/src/stirling/obj_tools/testdata/containers/vaddr_convert_self_func_container.h @@ -0,0 +1,38 @@ +/* + * Copyright 2018- The Pixie Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#pragma once + +#include "src/common/base/base.h" +#include "src/common/testing/test_environment.h" +#include "src/common/testing/test_utils/container_runner.h" + +using px::ContainerRunner; + +class VaddrConvertSelfFuncContainer : public ContainerRunner { + public: + VaddrConvertSelfFuncContainer() + : ContainerRunner(px::testing::BazelRunfilePath(kBazelImageTar), kInstanceNamePrefix, + kReadyMessage) {} + + private: + static constexpr std::string_view kBazelImageTar = + "src/stirling/obj_tools/testdata/containers/vaddr_convert_self_func_image.tar"; + static constexpr std::string_view kInstanceNamePrefix = "vaddr_convert_self_func_container"; + static constexpr std::string_view kReadyMessage = "Running"; +};