Skip to content

Commit

Permalink
Fixed UB when a CollectionChangeObserver deletes itself in its callback
Browse files Browse the repository at this point in the history
The code that calls observers attempted to protect itself from the
list being mutated while it was iterating, but apparently it didn't
work well enough.
I changed the code so it now iterates the list collecting the
observer pointers, and then calls them afterwards.
While I was debugging this I noticed SequenceTracker doesn't have a
logging identifier, so I added it.
  • Loading branch information
snej committed Aug 27, 2024
1 parent fb78816 commit a462360
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 14 deletions.
30 changes: 16 additions & 14 deletions LiteCore/Database/SequenceTracker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "Logging.hh"
#include "StringUtil.hh"
#include "Error.hh"
#include "SmallVector.hh"
#include <algorithm>
#include <sstream>
#include <utility>
Expand Down Expand Up @@ -242,23 +243,24 @@ namespace litecore {
entry->external = true; // it must have come from addExternalTransaction()
}

// Notify document notifiers:
for ( auto docNotifier : entry->documentObservers ) docNotifier->notify(entry);
// Notify document notifiers. Use a copy in case the callbacks mutate the collection:
if ( !entry->documentObservers.empty() ) {
auto observers = entry->documentObservers;
for ( auto docNotifier : observers ) docNotifier->notify(entry);
}

if ( listChanged && _numPlaceholders > 0 ) {
// Any placeholders right before this change were up to date, should be notified:
bool notified = false;
auto ph = next(_changes.rbegin()); // iterating _backwards_, skipping latest
while ( ph != _changes.rend() && ph->isPlaceholder() ) {
auto nextph = ph;
++nextph; // precompute next pos, in case 'ph' moves itself during the callback
if ( ph->databaseObserver ) {
ph->databaseObserver->notify();
notified = true;
}
ph = nextph;
// Any placeholders right before this change were up to date and should be notified:
// Iterate _backwards_, skipping latest. Call observers after iteration in case the callbacks delete
// the observers or otherwise mutate _changes.
smallVector<CollectionChangeNotifier*, 4> observers;
for ( auto ph = next(_changes.rbegin()); ph != _changes.rend() && ph->isPlaceholder(); ++ph ) {
if ( ph->databaseObserver ) observers.push_back(ph->databaseObserver);
}
if ( !observers.empty() ) {
for ( auto obs : observers ) obs->notify();
removeObsoleteEntries();
}
if ( notified ) removeObsoleteEntries();
}
}

Expand Down
2 changes: 2 additions & 0 deletions LiteCore/Database/SequenceTracker.hh
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ namespace litecore {

slice name() const { return _name; }

std::string loggingIdentifier() const override { return string(_name); }

/** Call this as soon as the database begins a transaction. */
void beginTransaction();

Expand Down

0 comments on commit a462360

Please sign in to comment.