Skip to content

Commit

Permalink
CBL-5661 : Fix a released captured context may be used in observer ca…
Browse files Browse the repository at this point in the history
…llback

* Implemented ContextManager class for retaining and mapping callback context objects with their pointer value. When getting the object back, the retained object will be returned so the object can be safely used in the callback.

* Updated ListenerToken<CBLQueryChangeListener> to use ContextManager for it’s C4QueryObserver’s callback lamda so that the callback can validate and safely use its captured context.

* Added CBLQuery_SetListenerCallbackDelay() private API for testing the fix.

* Updated LiteCore to 3.1.7-1 to get the fix for CBSE-16662.
  • Loading branch information
pasin committed Apr 22, 2024
1 parent a5ff22e commit 07b2f4d
Show file tree
Hide file tree
Showing 17 changed files with 284 additions and 16 deletions.
8 changes: 8 additions & 0 deletions CBL_C.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@
4022546E29355577000FBAC8 /* assets in Resources */ = {isa = PBXBuildFile; fileRef = 4022546D29355576000FBAC8 /* assets */; };
4022546F29355577000FBAC8 /* assets in Resources */ = {isa = PBXBuildFile; fileRef = 4022546D29355576000FBAC8 /* assets */; };
4022547029355577000FBAC8 /* assets in Resources */ = {isa = PBXBuildFile; fileRef = 4022546D29355576000FBAC8 /* assets */; };
407222872BD31BC500A6B560 /* ContextManager.cc in Sources */ = {isa = PBXBuildFile; fileRef = 407222852BD31BC500A6B560 /* ContextManager.cc */; };
407222882BD31BC500A6B560 /* ContextManager.hh in Headers */ = {isa = PBXBuildFile; fileRef = 407222862BD31BC500A6B560 /* ContextManager.hh */; };
40D1862529B6D1A50061AA85 /* Collection.hh in Headers */ = {isa = PBXBuildFile; fileRef = FCC064BD287CBD95000C5BD7 /* Collection.hh */; };
40EF697C2B7C34D600F0CB50 /* PrivacyInfo.xcprivacy in Resources */ = {isa = PBXBuildFile; fileRef = 40EF696D2B7C34BE00F0CB50 /* PrivacyInfo.xcprivacy */; };
42D1B69D2978AD31003B9871 /* CBLUserAgent.mm in Sources */ = {isa = PBXBuildFile; fileRef = 42D1B68F2978AD31003B9871 /* CBLUserAgent.mm */; };
Expand Down Expand Up @@ -464,6 +466,8 @@
27DBD096246C99AF002FD7A7 /* mergeIntoStaticLib.sh */ = {isa = PBXFileReference; lastKnownFileType = text.script.sh; path = mergeIntoStaticLib.sh; sourceTree = "<group>"; };
27DBD097246C9DE7002FD7A7 /* CBLDatabase+Apple.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = "CBLDatabase+Apple.mm"; sourceTree = "<group>"; };
4022546D29355576000FBAC8 /* assets */ = {isa = PBXFileReference; lastKnownFileType = folder; path = assets; sourceTree = "<group>"; };
407222852BD31BC500A6B560 /* ContextManager.cc */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = ContextManager.cc; sourceTree = "<group>"; };
407222862BD31BC500A6B560 /* ContextManager.hh */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.h; path = ContextManager.hh; sourceTree = "<group>"; };
40EF696D2B7C34BE00F0CB50 /* PrivacyInfo.xcprivacy */ = {isa = PBXFileReference; lastKnownFileType = text.xml; path = PrivacyInfo.xcprivacy; sourceTree = "<group>"; };
42D1B68F2978AD31003B9871 /* CBLUserAgent.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = CBLUserAgent.mm; sourceTree = "<group>"; };
932062DA26BC6B43006917A5 /* CBLQuery.cc */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = CBLQuery.cc; sourceTree = "<group>"; };
Expand Down Expand Up @@ -666,6 +670,8 @@
42D1B68F2978AD31003B9871 /* CBLUserAgent.mm */,
27D11BEE2351043B00C58A70 /* ConflictResolver.cc */,
27D11BED2351043B00C58A70 /* ConflictResolver.hh */,
407222862BD31BC500A6B560 /* ContextManager.hh */,
407222852BD31BC500A6B560 /* ContextManager.cc */,
271C2A7421CC4BD60045856E /* Internal.cc */,
271C2A7921CC756A0045856E /* Internal.hh */,
27886C8C21F64C1400069BEA /* Listener.cc */,
Expand Down Expand Up @@ -887,6 +893,7 @@
27DBCF2F246B4352002FD7A7 /* CBLQuery_Internal.hh in Headers */,
27D11BEF2351043B00C58A70 /* ConflictResolver.hh in Headers */,
FC5FBBA62821B3450066157F /* CBLCollection_Internal.hh in Headers */,
407222882BD31BC500A6B560 /* ContextManager.hh in Headers */,
93C70CE026C4D3F80093E927 /* CBLEncryptable.h in Headers */,
271C2A3321CAC98F0045856E /* CBLDocument.h in Headers */,
FCD988BB2821B10300512BBD /* CBLCollection.h in Headers */,
Expand Down Expand Up @@ -1511,6 +1518,7 @@
27B61D5621D5ABA60027CCDB /* CBLQuery_CAPI.cc in Sources */,
277FEE7521ED3C4900B60E3C /* CBLReplicator_CAPI.cc in Sources */,
FCD829B32835EE39004AA814 /* CBLScope_CAPI.cc in Sources */,
407222872BD31BC500A6B560 /* ContextManager.cc in Sources */,
27D11BF02351043B00C58A70 /* ConflictResolver.cc in Sources */,
27886C8E21F64C1400069BEA /* Listener.cc in Sources */,
FCB96E8329007D37001C4DED /* CBLDefaults_CAPI.cc in Sources */,
Expand Down
6 changes: 6 additions & 0 deletions src/CBLPrivate.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,10 @@ CBL_CAPI_BEGIN

FLSlice CBLReplicator_UserAgent(const CBLReplicator* repl) CBLAPI;

/** Adding a delay in MS before processing the callback from C4QueryObserver.
This is for testing to ensure that the callback has handle the case that the callback
is called after the query listener token as removed correctly without accessing
any invalidated objects. */
void CBLQuery_SetListenerCallbackDelay(int delay) CBLAPI;

CBL_CAPI_END
6 changes: 6 additions & 0 deletions src/CBLQuery_CAPI.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,12 @@ CBLResultSet* CBLQuery_CopyCurrentResults(const CBLQuery* query,
return listener->resultSet().detach();
}

void CBLQuery_SetListenerCallbackDelay(int delayMS) noexcept {
#ifdef DEBUG
ListenerToken<CBLQueryChangeListener>::setC4QueryObserverCallbackDelay(delayMS);
#endif
}

bool CBLResultSet_Next(CBLResultSet* rs) noexcept {
try {
return rs->next();
Expand Down
49 changes: 42 additions & 7 deletions src/CBLQuery_Internal.hh
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "CBLDatabase_Internal.hh"
#include "Internal.hh"
#include "Listener.hh"
#include "ContextManager.hh"
#include "c4Query.hh"
#include "access_lock.hh"
#include "fleece/Expert.hh"
Expand All @@ -17,6 +18,10 @@
#include <optional>
#include <unordered_map>

#ifdef DEBUG
#include <chrono>
#include <thread>
#endif

CBL_ASSUME_NONNULL_BEGIN

Expand Down Expand Up @@ -180,14 +185,31 @@ namespace cbl_internal {
:CBLListenerToken((const void*)callback, context)
,_query(query)
{
_stoppable = std::make_unique<CBLQueryListenerStoppable>(this);

auto ctx = ContextManager::shared().registerObject(this);

query->_c4query.useLocked([&](C4Query *c4query) {
_c4obs = c4query->observe([this](C4QueryObserver*) { this->queryChanged(); });
_c4obs = c4query->observe([ctx](C4QueryObserver* c4obs) {
#ifdef DEBUG
if (sC4QueryObserverCallbackDelay > 0) {
std::this_thread::sleep_for(std::chrono::milliseconds(sC4QueryObserverCallbackDelay));
}
#endif

// Get and retain object:
auto obj = ContextManager::shared().getObject(ctx);

// Validate and notify:
auto self = dynamic_cast<ListenerToken<CBLQueryChangeListener>*>(obj.get());
if (self && self->_c4obs == c4obs) {
self->queryChanged();
}
});
});

_stoppable = std::make_unique<CBLQueryListenerStoppable>(this);
}

~ListenerToken() = default;
virtual ~ListenerToken() = default;

void setEnabled(bool enabled);

Expand All @@ -198,8 +220,9 @@ namespace cbl_internal {
void call() {
std::lock_guard<std::recursive_mutex> lock(_mutex);
CBLQueryChangeListener cb = callback();
if (cb)
if (cb) {
cb(_context, _query, this);
}
}

Retained<CBLResultSet> resultSet() {
Expand All @@ -208,7 +231,19 @@ namespace cbl_internal {

// CBLListenerToken :

void willRemove() override {setEnabled(false);}
void willRemove() override {
setEnabled(false);
ContextManager::shared().unregisterObject(this);
}

// For Testing :

#ifdef DEBUG
inline static int sC4QueryObserverCallbackDelay;
static void setC4QueryObserverCallbackDelay(int delayMS) {
sC4QueryObserverCallbackDelay = delayMS;
}
#endif

private:
struct CBLQueryListenerStoppable : CBLStoppable {
Expand All @@ -225,7 +260,7 @@ namespace cbl_internal {
void queryChanged(); // defn is in CBLDatabase.cc, to prevent circular hdr dependency

Retained<CBLQuery> _query;
std::unique_ptr<C4QueryObserver> _c4obs;
Retained<C4QueryObserver> _c4obs;
std::unique_ptr<CBLQueryListenerStoppable> _stoppable;
bool _isEnabled {false};
};
Expand Down
54 changes: 54 additions & 0 deletions src/ContextManager.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
//
// ContextManager.cc
//
// Copyright © 2024 Couchbase. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

#include "ContextManager.hh"

using namespace std;
using namespace fleece;

namespace cbl_internal {

ContextManager &ContextManager::shared() {
static ContextManager* sContextManager = new ContextManager();
return *sContextManager;
}

ContextManager::ContextManager() { }

void* ContextManager::registerObject(CBLRefCounted* object) {
std::lock_guard<std::mutex> lock(_mutex);
_contexts.insert({(void*)object, retained(object)});
return object;
}

void ContextManager::unregisterObject(void* ptr) {
std::lock_guard<std::mutex> lock(_mutex);
if (auto i = _contexts.find(ptr); i != _contexts.end()) {
_contexts.erase(i);
}
}

Retained<CBLRefCounted> ContextManager::getObject(void* ptr) {
std::lock_guard<std::mutex> lock(_mutex);
if (auto i = _contexts.find(ptr); i != _contexts.end()) {
return i->second;
}
return nullptr;
}

}
59 changes: 59 additions & 0 deletions src/ContextManager.hh
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
//
// ContextManager.hh
//
// Copyright © 2024 Couchbase. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

#pragma once
#include "Internal.hh"
#include <mutex>
#include <unordered_map>

CBL_ASSUME_NONNULL_BEGIN

namespace cbl_internal {

/**
Thread-safe context manager for retaining and mapping the object with its pointer value which can be used
as a (captured) context for LiteCore's callback which could be either in C++ or C. This would allow the
callback to verify that the context pointer value is still valid or not before using it. The implementation simply
stores the object in a map by using its memory address as the key and returns the memory address as
the pointer value.
@note
There is a chance that a new registered objects may have the same memory address as the ones previously
unregistered. This implementation can be improved to reduce a chance of reusing the same memory address
by generating integer keys with reuseable integer number + cycle count as inspired by the implementation of
C# GCHandle. For now, the context object MUST BE validated for its originality before use. */
class ContextManager {
public:
static ContextManager& shared();

void* registerObject(CBLRefCounted* object);

void unregisterObject(void* ptr);

fleece::Retained<CBLRefCounted> getObject(void* ptr);

private:
ContextManager();

std::mutex _mutex;
std::unordered_map<void*, fleece::Retained<CBLRefCounted>> _contexts;
};

}

CBL_ASSUME_NONNULL_END
2 changes: 2 additions & 0 deletions src/exports/CBL_Exports.txt
Original file line number Diff line number Diff line change
Expand Up @@ -233,5 +233,7 @@ CBLDocument_Generation
CBLError_GetCaptureBacktraces
CBLError_SetCaptureBacktraces

CBLQuery_SetListenerCallbackDelay

CBLLog_BeginExpectingExceptions
CBLLog_EndExpectingExceptions
1 change: 1 addition & 0 deletions src/exports/generated/CBL.def
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ CBLDocument_CanonicalRevisionID
CBLDocument_Generation
CBLError_GetCaptureBacktraces
CBLError_SetCaptureBacktraces
CBLQuery_SetListenerCallbackDelay
CBLLog_BeginExpectingExceptions
CBLLog_EndExpectingExceptions
kCBLDefaultLogFileUsePlainText
Expand Down
1 change: 1 addition & 0 deletions src/exports/generated/CBL.exp
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ _CBLDocument_CanonicalRevisionID
_CBLDocument_Generation
_CBLError_GetCaptureBacktraces
_CBLError_SetCaptureBacktraces
_CBLQuery_SetListenerCallbackDelay
_CBLLog_BeginExpectingExceptions
_CBLLog_EndExpectingExceptions
_kCBLDefaultLogFileUsePlainText
Expand Down
1 change: 1 addition & 0 deletions src/exports/generated/CBL.gnu
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ CBL_C {
CBLDocument_Generation;
CBLError_GetCaptureBacktraces;
CBLError_SetCaptureBacktraces;
CBLQuery_SetListenerCallbackDelay;
CBLLog_BeginExpectingExceptions;
CBLLog_EndExpectingExceptions;
kCBLDefaultLogFileUsePlainText;
Expand Down
1 change: 1 addition & 0 deletions src/exports/generated/CBL_Android.gnu
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ CBL_C {
CBLDocument_Generation;
CBLError_GetCaptureBacktraces;
CBLError_SetCaptureBacktraces;
CBLQuery_SetListenerCallbackDelay;
CBLLog_BeginExpectingExceptions;
CBLLog_EndExpectingExceptions;
kCBLDefaultLogFileUsePlainText;
Expand Down
1 change: 1 addition & 0 deletions src/exports/generated/CBL_EE.def
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ CBLDocument_CanonicalRevisionID
CBLDocument_Generation
CBLError_GetCaptureBacktraces
CBLError_SetCaptureBacktraces
CBLQuery_SetListenerCallbackDelay
CBLLog_BeginExpectingExceptions
CBLLog_EndExpectingExceptions
kCBLDefaultLogFileUsePlainText
Expand Down
1 change: 1 addition & 0 deletions src/exports/generated/CBL_EE.exp
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ _CBLDocument_CanonicalRevisionID
_CBLDocument_Generation
_CBLError_GetCaptureBacktraces
_CBLError_SetCaptureBacktraces
_CBLQuery_SetListenerCallbackDelay
_CBLLog_BeginExpectingExceptions
_CBLLog_EndExpectingExceptions
_kCBLDefaultLogFileUsePlainText
Expand Down
1 change: 1 addition & 0 deletions src/exports/generated/CBL_EE.gnu
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ CBL_C {
CBLDocument_Generation;
CBLError_GetCaptureBacktraces;
CBLError_SetCaptureBacktraces;
CBLQuery_SetListenerCallbackDelay;
CBLLog_BeginExpectingExceptions;
CBLLog_EndExpectingExceptions;
kCBLDefaultLogFileUsePlainText;
Expand Down
1 change: 1 addition & 0 deletions src/exports/generated/CBL_EE_Android.gnu
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ CBL_C {
CBLDocument_Generation;
CBLError_GetCaptureBacktraces;
CBLError_SetCaptureBacktraces;
CBLQuery_SetListenerCallbackDelay;
CBLLog_BeginExpectingExceptions;
CBLLog_EndExpectingExceptions;
kCBLDefaultLogFileUsePlainText;
Expand Down
Loading

0 comments on commit 07b2f4d

Please sign in to comment.