Fix asan stack use after scope (fix #4675) #4676
Conversation
Calling soci::use with rvalues can lead to use of an unnamed tepmporary after its lifetime ended
|
Hello @StefanVK thanks for pointing this out. On which platform/environment did you observe this issue? |
|
Hi @ckeshava it was on x86_64 with clang-15 with asan. More details are in the issue #4675 Basically what soci ends up doing is taking a reference to the ravlue in a soci::use_container and then the address in a soci_use_type. It does not copy the value itself and does not extend its lifetime. The evaluation of the statement is triggered by the destructor of a soci::details::once_temp_type which is the result of operator<< on the soci::session. Other temporaries from the same expression can already be beyond its lifetime at this point. Now for trivial types such as int it is undefined behavior to access it after the lifetime has ended but realistically you won't see the memory being overwritten within the same statement. For a std::string however when the lifetime ends, the non trivial destructor is invoked and the backing heap memory (for strings too large for small string optimization) gets freed and could be reused by a different thread (depending on the allocator) or another non trivial destructor in the same thread for another object whose lifetime ends in the same statement (not the case for rippled). I did not observe the memory being overwritten, I only observed asan pointing out that memory is being accessed beyond its lifetime. But I'm convinced that for the std::string case it would be possible for the memory to be overwritten, or for a segmentation fault to occur. We had the same bug in our codebase and I started looking for the same issue in different codebases on github and found rippled. |
|
thanks for the detailed explanation @StefanVK It sounds like this is something that needs to be fixed in the soci codebase, but thank you for the fix. if I want to observe the overwriting of a piece of memory, I'll need to put breakpoints (before and after) and observe certain addresses in the heap right? is there a more efficient way to check this? |
intelliot
requested review from
HowardHinnant
and removed request for
mtrippled
|
@StefanVK: Can you confirm that, from your perspective, this PR still looks good to merge? |
|
@intelliot Yes, as far as I'm concerned this is still a correct fix. |
Address a stack-use-after-scope issue when using rvalues with `soci::use`. Replace rvalues with lvalues to ensure the scope extends beyond the end of the expression. The issue arises from `soci` taking a reference to the rvalue without copying its value or extending its lifetime. `soci` references rvalues in `soci::use_container` and then the address in `soci_use_type`. For types like `int`, memory access post-lifetime is unlikely to cause issues. However, for `std::string`, the backing heap memory can be freed and potentially reused, leading to a potential segmentation fault. This was detected on x86_64 using clang-15 with asan. asan confirms resolution of the issue. Fix XRPLF#4675
Address a stack-use-after-scope issue when using rvalues with `soci::use`. Replace rvalues with lvalues to ensure the scope extends beyond the end of the expression. The issue arises from `soci` taking a reference to the rvalue without copying its value or extending its lifetime. `soci` references rvalues in `soci::use_container` and then the address in `soci_use_type`. For types like `int`, memory access post-lifetime is unlikely to cause issues. However, for `std::string`, the backing heap memory can be freed and potentially reused, leading to a potential segmentation fault. This was detected on x86_64 using clang-15 with asan. asan confirms resolution of the issue. Fix XRPLF#4675
High Level Overview of Change
Fix asan stack-use-after-scope: #4675
Replace ravlues with lvalues for calls to
soci::useto have the scope go beyond the end of the expression.Context of Change
For
insertPeerReservationanddeletePeerReservationwe could be reading nodeIds that get overwritten while we're reading them.Type of Change
I built rippled with asan with and without the fix. Without the fix asan finds stack-use-after-scope with those commands
After the fix, asan no longer reports an issue.