Fix virtual to binary addr conversion for processes that have an unco…

…mmon 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 <[email protected]>

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.

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"],

src/stirling/obj_tools/address_converter.cc

@@ -17,15 +17,20 @@
  */
 
 #include <memory>
+#include <string>
 #include <vector>
 
+#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<uint32_t> GetProcMapsIndexForBinary(const std::string& binary_path,
+                                             const std::vector<ProcSMaps>& 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<std::unique_ptr<ElfAddressConverter>> ElfAddressConverter::Create(ElfReader* elf_reader,
@@ -64,16 +78,29 @@ StatusOr<std::unique_ptr<ElfAddressConverter>> ElfAddressConverter::Create(ElfRe
   }
   system::ProcParser parser;
   std::vector<system::ProcParser::ProcessSMaps> 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));

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

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"],
+)

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<ElfReader::SymbolInfo> 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<uint64_t>(&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;
+}

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";
+};