From dad3bd1843c358f38731926a328ec8384ebf7275 Mon Sep 17 00:00:00 2001
From: Jeremy <51220084+jeremy-rifkin@users.noreply.github.com>
Date: Sat, 25 May 2024 00:40:08 -0500
Subject: [PATCH] Some comments regarding dlfcn logistics and comment cleanup

---
 src/binary/object.cpp  | 12 ++++++------
 src/binary/safe_dl.cpp |  3 ++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/binary/object.cpp b/src/binary/object.cpp
index db023e1..c86da57 100644
--- a/src/binary/object.cpp
+++ b/src/binary/object.cpp
@@ -39,6 +39,9 @@ namespace detail {
         }
     }
     #endif
+    // dladdr queries are needed to get pre-ASLR addresses and targets to run symbol resolution on
+    // _dl_find_object is preferred if at all possible as it is much faster (added in glibc 2.35)
+    // dladdr1 is preferred if possible because it allows for a more accurate object path to be resolved (glibc 2.3.3)
     #ifdef CPPTRACE_HAS_DL_FIND_OBJECT // we don't even check for this on apple
     object_frame get_frame_object_info(frame_ptr address) {
         // Use _dl_find_object when we can, it's orders of magnitude faster
@@ -53,9 +56,7 @@ namespace detail {
         return frame;
     }
     #elif defined(HAS_DLADDR1)
-    // dladdr queries are needed to get pre-ASLR addresses and targets to run addr2line on
     object_frame get_frame_object_info(frame_ptr address) {
-        // reference: https://github.com/bminor/glibc/blob/master/debug/backtracesyms.c
         // https://github.com/bminor/glibc/blob/91695ee4598b39d181ab8df579b888a8863c4cab/elf/dl-addr.c#L26
         Dl_info info;
         link_map* link_map_info;
@@ -79,9 +80,9 @@ namespace detail {
         return frame;
     }
     #else
-    // TODO: Try to verify this is correct in the context of bad argv[0]
-    // macos doesn't have dladdr1 but it seems its dli_fname behaves more sensibly?
-    // dladdr queries are needed to get pre-ASLR addresses and targets to run addr2line on
+    // glibc dladdr may not return an accurate dli_fname as it uses argv[0] for addresses in the main executable
+    // https://github.com/bminor/glibc/blob/caed1f5c0b2e31b5f4e0f21fea4b2c9ecd3b5b30/elf/dl-addr.c#L33-L36
+    // macos doesn't have dladdr1 but its dli_fname behaves more sensibly, same with some other libc's like musl
     object_frame get_frame_object_info(frame_ptr address) {
         // reference: https://github.com/bminor/glibc/blob/master/debug/backtracesyms.c
         Dl_info info;
@@ -111,7 +112,6 @@ namespace detail {
         if(it == cache.end()) {
             char path[MAX_PATH];
             if(GetModuleFileNameA(handle, path, sizeof(path))) {
-                ///std::fprintf(stderr, "path: %s base: %p\n", path, handle);
                 cache.insert(it, {handle, path});
                 return path;
             } else {
diff --git a/src/binary/safe_dl.cpp b/src/binary/safe_dl.cpp
index 5c7ad6e..a4ecf0c 100644
--- a/src/binary/safe_dl.cpp
+++ b/src/binary/safe_dl.cpp
@@ -23,7 +23,7 @@ namespace detail {
     void get_safe_object_frame(frame_ptr address, safe_object_frame* out) {
         out->raw_address = address;
         dl_find_object result;
-        if(_dl_find_object(reinterpret_cast<void*>(address), &result) == 0) {
+        if(_dl_find_object(reinterpret_cast<void*>(address), &result) == 0) { // thread-safe, signal-safe
             out->address_relative_to_object_start = address - to_frame_ptr(result.dlfo_link_map->l_addr);
             if(result.dlfo_link_map->l_name != nullptr && result.dlfo_link_map->l_name[0] != 0) {
                 std::size_t path_length = std::strlen(result.dlfo_link_map->l_name);
@@ -35,6 +35,7 @@ namespace detail {
             } else {
                 // empty l_name, this means it's the currently running executable
                 memset(out->object_path, 0, CPPTRACE_PATH_MAX + 1);
+                // signal-safe
                 auto res = readlink("/proc/self/exe", out->object_path, CPPTRACE_PATH_MAX);
                 if(res == -1) {
                     // error handling?