Improve error handling for object file parsing as well as a lot of cleanup and refactoring

src/platform/elf.hpp

@@ -9,6 +9,7 @@
 #include <cstdint>
 #include <cstdio>
 #include <cstring>
+#include <type_traits>
 
 #include <elf.h>
 
@@ -23,71 +24,47 @@ namespace detail {
         }
     }
 
-    // TODO: Address code duplication here. Do we actually have to care about 32-bit if the library is compiled as
+    template<std::size_t Bits>
-    // 64-bit? I think probably not...
-
-    // TODO: Re-evaluate use of off_t
-    // I think we can rely on PT_PHDR https://stackoverflow.com/q/61568612/15675011...
     static uintptr_t elf_get_module_image_base_from_program_table(
+        const std::string& obj_path,
         FILE* file,
-        bool is_64,
+        bool is_little_endian
-        bool is_little_endian,
-        off_t e_phoff,
-        off_t e_phentsize,
-        int e_phnum
     ) {
-        for(int i = 0; i < e_phnum; i++) {
+        static_assert(Bits == 32 || Bits == 64);
-            if(is_64) {
+        using Header = typename std::conditional<Bits == 32, Elf32_Ehdr, Elf64_Ehdr>::type;
-                Elf64_Phdr program_header = load_bytes<Elf64_Phdr>(file, e_phoff + e_phentsize * i);
+        using PHeader = typename std::conditional<Bits == 32, Elf32_Phdr, Elf64_Phdr>::type;
-                if(elf_byteswap_if_needed(program_header.p_type, is_little_endian) == PT_PHDR) {
+        Header file_header = load_bytes<Header>(file, 0);
-                    return elf_byteswap_if_needed(program_header.p_vaddr, is_little_endian)
+        CPPTRACE_VERIFY(file_header.e_ehsize == sizeof(Header), "ELF file header size mismatch" + obj_path);
-                            - elf_byteswap_if_needed(program_header.p_offset, is_little_endian);
+        // PT_PHDR will occur at most once
-                }
+        // Should be somewhat reliable https://stackoverflow.com/q/61568612/15675011
-            } else {
+        // It should occur at the beginning but may as well loop just in case
-                Elf32_Phdr program_header = load_bytes<Elf32_Phdr>(file, e_phoff + e_phentsize * i);
+        for(int i = 0; i < file_header.e_phnum; i++) {
-                if(elf_byteswap_if_needed(program_header.p_type, is_little_endian) == PT_PHDR) {
+            PHeader program_header = load_bytes<PHeader>(file, file_header.e_phoff + file_header.e_phentsize * i);
-                    return elf_byteswap_if_needed(program_header.p_vaddr, is_little_endian)
+            if(elf_byteswap_if_needed(program_header.p_type, is_little_endian) == PT_PHDR) {
-                            - elf_byteswap_if_needed(program_header.p_offset, is_little_endian);
+                return elf_byteswap_if_needed(program_header.p_vaddr, is_little_endian) -
-                }
+                       elf_byteswap_if_needed(program_header.p_offset, is_little_endian);
             }
         }
+        // Apparently some objects like shared objects can end up missing this file. 0 as a base seems correct.
         return 0;
     }
 
     static uintptr_t elf_get_module_image_base(const std::string& obj_path) {
-        FILE* file = fopen(obj_path.c_str(), "rb");
+        auto file = raii_wrapper(fopen(obj_path.c_str(), "rb"), file_deleter);
         if(file == nullptr) {
-            throw file_error();
+            throw file_error("Unable to read object file " + obj_path);
         }
         // Initial checks/metadata
         auto magic = load_bytes<std::array<char, 4>>(file, 0);
-        CPPTRACE_VERIFY(magic == (std::array<char, 4>{0x7F, 'E', 'L', 'F'}));
+        CPPTRACE_VERIFY(magic == (std::array<char, 4>{0x7F, 'E', 'L', 'F'}), "File is not ELF " + obj_path);
         bool is_64 = load_bytes<uint8_t>(file, 4) == 2;
         bool is_little_endian = load_bytes<uint8_t>(file, 5) == 1;
-        CPPTRACE_VERIFY(load_bytes<uint8_t>(file, 6) == 1, "Unexpected ELF version");
+        CPPTRACE_VERIFY(load_bytes<uint8_t>(file, 6) == 1, "Unexpected ELF endianness " + obj_path);
-        //
+        // get image base
         if(is_64) {
-            Elf64_Ehdr file_header = load_bytes<Elf64_Ehdr>(file, 0);
+            return elf_get_module_image_base_from_program_table<64>(obj_path, file, is_little_endian);
-            CPPTRACE_VERIFY(file_header.e_ehsize == sizeof(Elf64_Ehdr));
-            return elf_get_module_image_base_from_program_table(
-                file,
-                is_64,
-                is_little_endian,
-                elf_byteswap_if_needed(file_header.e_phoff, is_little_endian),
-                elf_byteswap_if_needed(file_header.e_phentsize, is_little_endian),
-                elf_byteswap_if_needed(file_header.e_phnum, is_little_endian)
-            );
         } else {
-            Elf32_Ehdr file_header = load_bytes<Elf32_Ehdr>(file, 0);
+            return elf_get_module_image_base_from_program_table<32>(obj_path, file, is_little_endian);
-            CPPTRACE_VERIFY(file_header.e_ehsize == sizeof(Elf32_Ehdr));
-            return elf_get_module_image_base_from_program_table(
-                file,
-                is_64,
-                is_little_endian,
-                elf_byteswap_if_needed(file_header.e_phoff, is_little_endian),
-                elf_byteswap_if_needed(file_header.e_phentsize, is_little_endian),
-                elf_byteswap_if_needed(file_header.e_phnum, is_little_endian)
-            );
         }
     }
 }

src/platform/error.hpp

@@ -4,6 +4,7 @@
 #include <cstdio>
 #include <exception>
 #include <stdexcept>
+#include <string>
 
 #include "common.hpp"
 
@@ -16,8 +17,11 @@
 namespace cpptrace {
 namespace detail {
     class file_error : public std::exception {
+        std::string msg;
+    public:
+        file_error(std::string path) : msg("Unable to read file " + std::move(path)) {}
         const char* what() const noexcept override {
-            return "Unable to read file";
+            return msg.c_str();
         }
     };
 
@@ -39,12 +43,12 @@ namespace detail {
         const char* expression,
         const char* signature,
         source_location location,
-        const char* message = nullptr
+        const std::string& message = ""
     ) {
         if(!condition) {
             const char* action = verify ? "verification" : "assertion";
             const char* name   = verify ? "VERIFY"       : "ASSERT";
-            if(message == nullptr) {
+            if(message == "") {
                 throw std::runtime_error(
                     stringf(
                         "Cpptrace %s failed at %s:%d: %s\n"
@@ -58,7 +62,7 @@ namespace detail {
                     stringf(
                         "Cpptrace %s failed at %s:%d: %s: %s\n"
                         "    CPPTRACE_%s(%s);\n",
-                        action, location.file, location.line, signature, message,
+                        action, location.file, location.line, signature, message.c_str(),
                         name, expression
                     )
                 );

src/platform/mach-o.hpp

@@ -27,6 +27,20 @@
 
 namespace cpptrace {
 namespace detail {
+    static bool is_mach_o(uint32_t magic) {
+        switch(magic) {
+            case FAT_MAGIC:
+            case FAT_CIGAM:
+            case MH_MAGIC:
+            case MH_CIGAM:
+            case MH_MAGIC_64:
+            case MH_CIGAM_64:
+                return true;
+            default:
+                return false;
+        }
+    }
+
     // Based on https://github.com/AlexDenisov/segment_dumper/blob/master/main.c
     // and https://lowlevelbits.org/parsing-mach-o-files/
     static bool is_magic_64(uint32_t magic) {
@@ -37,79 +51,60 @@ namespace detail {
         return magic == MH_CIGAM || magic == MH_CIGAM_64 || magic == FAT_CIGAM;
     }
 
-    static uintptr_t macho_get_text_vmaddr_from_segments(
+    static void swap_mach_header(mach_header_64& header) {
-        FILE* obj_file,
+        swap_mach_header_64(&header, NX_UnknownByteOrder);
-        off_t offset,
+    }
-        bool should_swap,
+
-        uint32_t ncmds
+    static void swap_mach_header(mach_header& header) {
-    ) {
+        swap_mach_header(&header, NX_UnknownByteOrder);
-        off_t actual_offset = offset;
+    }
+
+    static void swap_segment_command(segment_command_64& segment) {
+        swap_segment_command_64(&segment, NX_UnknownByteOrder);
+    }
+
+    static void swap_segment_command(segment_command& segment) {
+        swap_segment_command(&segment, NX_UnknownByteOrder);
+    }
+
+    template<std::size_t Bits>
+    static uintptr_t macho_get_text_vmaddr_mach(FILE* obj_file, off_t offset, bool is_64, bool should_swap) {
+        static_assert(Bits == 32 || Bits == 64);
+        using Mach_Header = typename std::conditional<Bits == 32, mach_header, mach_header_64>::type;
+        using Segment_Command = typename std::conditional<Bits == 32, segment_command, segment_command_64>::type;
+        uint32_t ncmds;
+        off_t load_commands_offset = offset;
+        size_t header_size = sizeof(Mach_Header);
+        Mach_Header header = load_bytes<Mach_Header>(obj_file, offset);
+        if(header.cputype != CURRENT_CPU) {
+            return 0;
+        }
+        if(should_swap) {
+            swap_mach_header<Bits>(header);
+        }
+        ncmds = header.ncmds;
+        load_commands_offset += header_size;
+        // iterate load commands
+        off_t actual_offset = load_commands_offset;
         for(uint32_t i = 0; i < ncmds; i++) {
             load_command cmd = load_bytes<load_command>(obj_file, actual_offset);
             if(should_swap) {
                 swap_load_command(&cmd, NX_UnknownByteOrder);
             }
-            if(cmd.cmd == LC_SEGMENT_64) {
+            Segment_Command segment = load_bytes<Segment_Command>(obj_file, actual_offset);
-                segment_command_64 segment = load_bytes<segment_command_64>(obj_file, actual_offset);
+            if(should_swap) {
-                if(should_swap) {
+                swap_segment_command(&segment, NX_UnknownByteOrder);
-                    swap_segment_command_64(&segment, NX_UnknownByteOrder);
+            }
-                }
+            if(strcmp(segment.segname, "__TEXT") == 0) {
-                //printf("segname(64): %s\n", segment.segname);
+                return segment.vmaddr;
-                //printf("             %d\n", segment.nsects);
-                //printf("             %p\n", segment.vmaddr);
-                //printf("             %p\n", segment.vmsize);
-                if(strcmp(segment.segname, "__TEXT") == 0) {
-                    return segment.vmaddr;
-                }
-            } else if(cmd.cmd == LC_SEGMENT) {
-                segment_command segment = load_bytes<segment_command>(obj_file, actual_offset);
-                if(should_swap) {
-                    swap_segment_command(&segment, NX_UnknownByteOrder);
-                }
-                //printf("segname: %s\n", segment.segname);
-                if(strcmp(segment.segname, "__TEXT") == 0) {
-                    return segment.vmaddr;
-                }
             }
             actual_offset += cmd.cmdsize;
         }
         // somehow no __TEXT section was found...
+        CPPTRACE_VERIFY(false, "Couldn't find __TEXT section while parsing Mach-O object")
         return 0;
     }
 
-    static uintptr_t macho_get_text_vmaddr_mach(FILE* obj_file, off_t offset, bool is_64, bool should_swap) {
-        uint32_t ncmds;
-        off_t load_commands_offset = offset;
-        if(is_64) {
-            size_t header_size = sizeof(mach_header_64);
-            mach_header_64 header = load_bytes<mach_header_64>(obj_file, offset);
-            //if(offset != 0) { // if fat the offset will be non-zero, if not fat the offset will be zero
-                if(header.cputype != CURRENT_CPU) {
-                    return 0;
-                }
-            //}
-            if(should_swap) {
-                swap_mach_header_64(&header, NX_UnknownByteOrder);
-            }
-            ncmds = header.ncmds;
-            load_commands_offset += header_size;
-        } else {
-            size_t header_size = sizeof(mach_header);
-            mach_header header = load_bytes<mach_header>(obj_file, offset);
-            //if(offset != 0) { // if fat the offset will be non-zero, if not fat the offset will be zero
-                if(header.cputype != CURRENT_CPU) {
-                    return 0;
-                }
-            //}
-            if(should_swap) {
-                swap_mach_header(&header, NX_UnknownByteOrder);
-            }
-            ncmds = header.ncmds;
-            load_commands_offset += header_size;
-        }
-        return macho_get_text_vmaddr_from_segments(obj_file, load_commands_offset, should_swap, ncmds);
-    }
-
     static uintptr_t macho_get_text_vmaddr_fat(FILE* obj_file, bool should_swap) {
         size_t header_size = sizeof(fat_header);
         size_t arch_size = sizeof(fat_arch);
@@ -138,25 +133,24 @@ namespace detail {
             }
         }
         // If this is reached... something went wrong. The cpu we're on wasn't found.
-        return text_vmaddr;
+        CPPTRACE_VERIFY(false, "Couldn't find appropriate architecture in fat Mach-O");
+        return 0;
     }
 
-    static uintptr_t macho_get_text_vmaddr(const char* path) {
+    static uintptr_t macho_get_text_vmaddr(const std::string& obj_path) {
-        FILE* obj_file = fopen(path, "rb");
+        auto file = raii_wrapper(fopen(obj_path.c_str(), "rb"), file_deleter);
-        if(obj_file == nullptr) {
+        if(file == nullptr) {
-            throw file_error();
+            throw file_error("Unable to read object file " + obj_path);
         }
-        uint32_t magic = load_bytes<uint32_t>(obj_file, 0);
+        uint32_t magic = load_bytes<uint32_t>(file, 0);
+        CPPTRACE_VERIFY(is_mach_o(magic), "File is not Mach-O " + obj_path);
         bool is_64 = is_magic_64(magic);
         bool should_swap = should_swap_bytes(magic);
-        uintptr_t addr;
         if(magic == FAT_MAGIC || magic == FAT_CIGAM) {
-            addr = macho_get_text_vmaddr_fat(obj_file, should_swap);
+            return macho_get_text_vmaddr_fat(file, should_swap);
         } else {
-            addr = macho_get_text_vmaddr_mach(obj_file, 0, is_64, should_swap);
+            return macho_get_text_vmaddr_mach(file, 0, is_64, should_swap);
         }
-        fclose(obj_file);
-        return addr;
     }
 }
 }

src/platform/object.hpp

@@ -53,7 +53,7 @@ namespace detail {
         if(it == cache.end()) {
             // arguably it'd be better to release the lock while computing this, but also arguably it's good to not
             // have two threads try to do the same computation
-            auto base = macho_get_text_vmaddr(obj_path.c_str());
+            auto base = macho_get_text_vmaddr(obj_path);
             cache.insert(it, {obj_path, base});
             return base;
         } else {

src/platform/pe.hpp

@@ -18,7 +18,7 @@ namespace cpptrace {
 namespace detail {
     template<typename T, typename std::enable_if<std::is_integral<T>::value, int>::type = 0>
     T pe_byteswap_if_needed(T value) {
-        // PE header values are little endian
+        // PE header values are little endian, I think dos e_lfanew should be too
         if(!is_little_endian()) {
             return byteswap(value);
         } else {
@@ -27,17 +27,21 @@ namespace detail {
     }
 
     inline uintptr_t pe_get_module_image_base(const std::string& obj_path) {
-        FILE* file;
+        // https://drive.google.com/file/d/0B3_wGJkuWLytbnIxY1J5WUs4MEk/view?pli=1&resourcekey=0-n5zZ2UW39xVTH8ZSu6C2aQ
-        errno_t ret = fopen_s(&file, obj_path.c_str(), "rb");
+        // https://0xrick.github.io/win-internals/pe3/
+        // Endianness should always be little for dos and pe headers
+        FILE* file_ptr;
+        errno_t ret = fopen_s(&file_ptr, obj_path.c_str(), "rb");
+        auto file = raii_wrap(file_ptr, file_deleter);
         if(ret != 0 || file == nullptr) {
-            throw file_error();
+            throw file_error("Unable to read object file " + obj_path);
         }
         auto magic = load_bytes<std::array<char, 2>>(file, 0);
-        CPPTRACE_VERIFY(memcmp(magic.data(), "MZ", 2) == 0);
+        CPPTRACE_VERIFY(memcmp(magic.data(), "MZ", 2) == 0, "File is not a PE file " + obj_path);
         DWORD e_lfanew = pe_byteswap_if_needed(load_bytes<DWORD>(file, 0x3c)); // dos header + 0x3c
-        long nt_header_offset = e_lfanew;
+        DWORD nt_header_offset = e_lfanew;
         auto signature = load_bytes<std::array<char, 4>>(file, nt_header_offset); // nt header + 0
-        CPPTRACE_VERIFY(memcmp(signature.data(), "PE\0\0", 4) == 0);
+        CPPTRACE_VERIFY(memcmp(signature.data(), "PE\0\0", 4) == 0, "File is not a PE file " + obj_path);
         WORD size_of_optional_header = pe_byteswap_if_needed(
             load_bytes<WORD>(file, nt_header_offset + 4 + 0x10) // file header + 0x10
         );
@@ -45,11 +49,14 @@ namespace detail {
         WORD optional_header_magic = pe_byteswap_if_needed(
             load_bytes<WORD>(file, nt_header_offset + 0x18) // optional header + 0x0
         );
-        CPPTRACE_VERIFY(optional_header_magic == IMAGE_NT_OPTIONAL_HDR_MAGIC);
+        CPPTRACE_VERIFY(
-        uintptr_t image_base;
+            optional_header_magic == IMAGE_NT_OPTIONAL_HDR_MAGIC,
+            "PE file does not match expected bit-mode " + obj_path
+        );
+        // finally get image base
         if(optional_header_magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
             // 32 bit
-            image_base = to<uintptr_t>(
+            return to<uintptr_t>(
                 pe_byteswap_if_needed(
                     load_bytes<DWORD>(file, nt_header_offset + 0x18 + 0x1c) // optional header + 0x1c
                 )
@@ -57,14 +64,12 @@ namespace detail {
         } else {
             // 64 bit
             // I get an "error: 'QWORD' was not declared in this scope" for some reason when using QWORD
-            image_base = to<uintptr_t>(
+            return to<uintptr_t>(
                 pe_byteswap_if_needed(
                     load_bytes<unsigned __int64>(file, nt_header_offset + 0x18 + 0x18) // optional header + 0x18
                 )
             );
         }
-        fclose(file);
-        return image_base;
     }
 }
 }

src/platform/utils.hpp

@@ -342,6 +342,52 @@ namespace detail {
     U to(V v) {
         return static_cast<U>(v);
     }
+
+    template<
+        typename T,
+        typename D,
+        typename std::enable_if<
+            std::is_same<decltype(std::declval<D>()(std::declval<T>())), void>::value, int
+        >::type = 0
+    >
+    class raii_wrapper {
+        T obj;
+        optional<D> deleter;
+    public:
+        raii_wrapper(T&& obj, D deleter) : obj(std::move(obj)), deleter(deleter) {}
+        raii_wrapper(raii_wrapper&& other) : obj(std::move(other.obj)), deleter(other.deleter) {
+            other.deleter = nullopt;
+        }
+        raii_wrapper(const raii_wrapper&) = delete;
+        raii_wrapper& operator=(raii_wrapper&&) = delete;
+        raii_wrapper& operator=(const raii_wrapper&) = delete;
+        ~raii_wrapper() {
+            if(deleter) {
+                deleter.unwrap()(obj);
+            }
+        }
+        operator T&() {
+            return obj;
+        }
+        operator const T&() const {
+            return obj;
+        }
+    };
+
+    template<
+        typename T,
+        typename D,
+        typename std::enable_if<
+            std::is_same<decltype(std::declval<D>()(std::declval<T>())), void>::value, int
+        >::type = 0
+    >
+    raii_wrapper<T, D> raii_wrap(T&& obj, D deleter) {
+        return {std::move(obj), deleter};
+    }
+
+    inline void file_deleter(FILE* ptr) {
+        fclose(ptr);
+    }
 }
 }