Skip to content

Commit

Permalink
Fixed a silly use-after-free error in PendingResults
Browse files Browse the repository at this point in the history
Testing self_heap_ptr for null, and setting it to null after deleting
it, won't work if self_heap_ptr is actually the last valid shared_ptr to
the PendingResults - then delete self_heap_ptr will delete the very
PendingResults that stores self_heap_ptr, and it will be illegal to set
self_heap_ptr to null! The call to delete self_heap_ptr must be the last
statement in the method, since the object it's in might get deleted at
that point. We can still prevent an accidental double-free by just using
a boolean that gets changed before deleting the pointer.
  • Loading branch information
etremel committed Aug 20, 2021
1 parent a4bbe51 commit 0378959
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 4 deletions.
13 changes: 11 additions & 2 deletions include/derecho/core/detail/rpc_utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,13 @@ class PendingResults : public AbstractPendingResults, public std::enable_shared_
*/
std::promise<void> signature_verified_promise;

/**
* A flag set to true the first time delete_self_ptr() is called, to
* prevent it from attempting to delete the pointer again if it is
* mistakenly called twice on the same object.
*/
bool heap_pointer_deleted;

/**
* A raw pointer to a heap-allocated shared_ptr to this PendingResults object,
* which is used by RemoteInvoker to locate this object when a reply arrives.
Expand All @@ -721,6 +728,7 @@ class PendingResults : public AbstractPendingResults, public std::enable_shared_
public:
PendingResults()
: reply_promises_are_ready(promise_for_reply_promises.get_future()),
heap_pointer_deleted(false),
self_heap_ptr(nullptr) {}
virtual ~PendingResults() {}

Expand Down Expand Up @@ -773,10 +781,11 @@ class PendingResults : public AbstractPendingResults, public std::enable_shared_
* method more than once, and it will have no effect after the first call.
*/
void delete_self_ptr() {
if(self_heap_ptr) {
if(!heap_pointer_deleted) {
dbg_default_trace("delete_self_ptr() deleting the shared_ptr at {}", fmt::ptr(self_heap_ptr));
heap_pointer_deleted = true;
//This must be the last statement in the method, since it might result in this object getting deleted
delete self_heap_ptr;
self_heap_ptr = nullptr;
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/core/git_version.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ namespace derecho {
const int MAJOR_VERSION = 2;
const int MINOR_VERSION = 2;
const int PATCH_VERSION = 0;
const int COMMITS_AHEAD_OF_VERSION = 22;
const int COMMITS_AHEAD_OF_VERSION = 23;
const char* VERSION_STRING = "2.2.0";
const char* VERSION_STRING_PLUS_COMMITS = "2.2.0+22";
const char* VERSION_STRING_PLUS_COMMITS = "2.2.0+23";

}

0 comments on commit 0378959

Please sign in to comment.