Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Relative Virtual Addresses to allow their decoding without knowing the base address #200

Merged
merged 7 commits into from
Jan 7, 2025

Conversation

McCzarny
Copy link
Contributor

@McCzarny McCzarny commented Jan 1, 2025

Instead of printing absolute addresses use relative ones so they can be used later to decode the stack trace.

Motivation

It’s quite common to release apps without debug symbols while keeping them internally. Currently the lib prints absolute addresses which require the base address to decode them using symbols. With this change, the base address won’t be needed as values are relative to the beginning of the loaded module/binary.

The implementation for unix is straightforward as the existing code is used. For Windows, I’ve implemented similar logic using Windows API.

Manual testing of Windows implementation

Here are 2 runs of trivial_windbg_lib.exe with .pdb and without .pdb file:

>./trivial_windbg_lib.exe
 0# boost::stacktrace::basic_stacktrace<std::allocator<boost::stacktrace::frame> >::init at C:\Users\czarneckim\repositories\boost\boost\stacktrace\stacktrace.hpp:110
 1# main at C:\Users\czarneckim\repositories\boost\libs\stacktrace\test\test_trivial.cpp:14
 2# __scrt_common_main_seh at D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
 3# BaseThreadInitThunk in C:\WINDOWS\System32\KERNEL32.DLL
 4# RtlUserThreadStart in C:\WINDOWS\SYSTEM32\ntdll.dll

> mv ./trivial_windbg_lib.pdb{,_} 
> ./trivial_windbg_lib.exe
 0# 0x0000000000001DE3 in C:\Users\czarneckim\repositories\boost\bin.v2\libs\stacktrace\test\trivial_windbg_lib.test\msvc-14.3\release\x86_64\asynch-exceptions-on\cxxstd-latest-iso\debug-symbols-on\threading-multi\trivial_windbg_lib.exe
 1# 0x0000000000001FEE in C:\Users\czarneckim\repositories\boost\bin.v2\libs\stacktrace\test\trivial_windbg_lib.test\msvc-14.3\release\x86_64\asynch-exceptions-on\cxxstd-latest-iso\debug-symbols-on\threading-multi\trivial_windbg_lib.exe
 2# 0x000000000000246C in C:\Users\czarneckim\repositories\boost\bin.v2\libs\stacktrace\test\trivial_windbg_lib.test\msvc-14.3\release\x86_64\asynch-exceptions-on\cxxstd-latest-iso\debug-symbols-on\threading-multi\trivial_windbg_lib.exe
 3# BaseThreadInitThunk in C:\WINDOWS\System32\KERNEL32.DLL
 4# RtlUserThreadStart in C:\WINDOWS\SYSTEM32\ntdll.dll

> mv ./trivial_windbg_lib.pdb{_,}
> winaddr2line.exe -f -e trivial_windbg_lib.pdb 0x0000000000001DE3
boost::stacktrace::basic_stacktrace<std::allocator<boost::stacktrace::frame> >::init
C:\Users\czarneckim\repositories\boost\boost\stacktrace\stacktrace.hpp:110

I’ve also decoded the returned address using x64dbg running trivial_windbg_lib.exe+0x0000000000001DE3:

image

Control

As it was suggested in #180, the new logic is enabled by default, but can be disabled with BOOST_STACKTRACE_DISABLE_OFFSET_ADDR_BASE (With non header-only mode, it requires rebuilding the lib)

Further work

If the implementation would be accepted, I’m happy to do the rest of work, like changes in documentation or test cases. It’s my first contribution to this repo, so I would be very grateful for some guidance what is required.

Copy link
Member

@apolukhin apolukhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Requested a few changes. Do you have ideas, how to autotest the offsets?

include/boost/stacktrace/detail/addr_base_msvc.hpp Outdated Show resolved Hide resolved
include/boost/stacktrace/detail/addr_base_msvc.hpp Outdated Show resolved Hide resolved
include/boost/stacktrace/detail/addr_base_msvc.hpp Outdated Show resolved Hide resolved
include/boost/stacktrace/detail/addr_base_msvc.hpp Outdated Show resolved Hide resolved
include/boost/stacktrace/detail/addr_base_msvc.hpp Outdated Show resolved Hide resolved
include/boost/stacktrace/detail/addr_base_msvc.hpp Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jan 1, 2025

Coverage Status

coverage: 86.371% (+0.02%) from 86.35%
when pulling bf9b566 on McCzarny:develop
into 5ec4591 on boostorg:develop.

@McCzarny
Copy link
Contributor Author

McCzarny commented Jan 2, 2025

Looks good! Requested a few changes. Do you have ideas, how to autotest the offsets?

The simplest solution might be to expect addresses in some range (maybe it's possible to read it from the file itself - ELF?), but this won't be very strict and will miss small differences.

A more complex way would be to test executables that have debug symbols in separate files (.pdb and unstripped/stripped with --only-keep-debug). For Windows, Dbghelp is sufficient to convert RVA to a symbol. The library is available on the windows-latest image with visualstudio2022buildtools installed, not sure about older versions. For Linux, I don't know if it's possible to do the same with a minimal setup.

Another thing that comes to mind is that in an ASLR env we can run an executable multiple times and expect the same result.

How complex could such a test be? I can imagine it being run with a lot of setups, so it should use some platform specific libs or tools.

@apolukhin
Copy link
Member

How complex could such a test be?

Just make a smoke test. Check that the address is less than the size of a binary you are working with. Path to the binary could be obtained from argv[0], and the size could be retrieved via

std::ifstream file(argv[0], std::ios::binary | std::ios::ate);
return file.tellg();

{
const auto frame = to_string(boost::stacktrace::stacktrace(0, 1).as_vector().front());

// Skip the test if the frame does not contain an address
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to use BOOST_STACKTRACE_TEST_SHOULD_OUTPUT_READABLE_NAMES but there were configurations were it was set to 0, but the stacktrace had a function name included. Let me know if you're fine with this part.

@@ -263,7 +264,51 @@ void test_stacktrace_limits()
BOOST_TEST_EQ(boost::stacktrace::stacktrace(1, 1).size(), 1);
}

int main() {
std::size_t get_file_size(const char* file_name) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these helper functions be here or you prefer to move them to test_imp?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is fine to remain here

@apolukhin apolukhin merged commit b170b28 into boostorg:develop Jan 7, 2025
42 checks passed
@apolukhin
Copy link
Member

Many thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants