Skip to content

Commit

Permalink
Fix check-then-act race in GetServiceFromFactory (#664)
Browse files Browse the repository at this point in the history 
* Added test case

Signed-off-by: The MathWorks, Inc. <alchrist@mathworks.com>

* Fix check-then-act race for GetServiceFromFactory

Signed-off-by: The MathWorks, Inc. <alchrist@mathworks.com>

* Fixed test names and cleaned up tests

Signed-off-by: The MathWorks, Inc. <alchrist@mathworks.com>

* More cleanup

* Made changes requested by reviewers
  • Loading branch information
@achristoforides
achristoforides committed May 11, 2022
1 parent 47a3f92 commit 0ad5875
Show file tree
Hide file tree
Showing 7 changed files with 177 additions and 74 deletions.
5 changes: 3 additions & 2 deletions framework/src/service/ServiceReferenceBase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,10 @@ Bundle ServiceReferenceBase::GetBundle() const

auto l = p->registration->Lock();
US_UNUSED(l);
if (p->registration->bundle == nullptr)
if (p->registration->bundle.lock() == nullptr) {
return Bundle();
return MakeBundle(p->registration->bundle->shared_from_this());
}
return MakeBundle(p->registration->bundle.lock()->shared_from_this());
}

std::vector<Bundle> ServiceReferenceBase::GetUsingBundles() const
Expand Down
90 changes: 53 additions & 37 deletions framework/src/service/ServiceReferenceBasePrivate.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@

#include "cppmicroservices/Bundle.h"
#include "cppmicroservices/FrameworkEvent.h"
#include "cppmicroservices/SecurityException.h"
#include "cppmicroservices/ServiceException.h"
#include "cppmicroservices/ServiceFactory.h"
#include "cppmicroservices/SecurityException.h"
#include "cppmicroservices/SharedLibraryException.h"

#include "BundlePrivate.h"
Expand Down Expand Up @@ -72,15 +72,17 @@ InterfaceMapConstPtr ServiceReferenceBasePrivate::GetServiceFromFactory(
factory->GetService(MakeBundle(bundle->shared_from_this()),
ServiceRegistrationBase(registration));
if (!smap || smap->empty()) {
std::string message =
"ServiceFactory returned an empty or nullptr interface map.";
registration->bundle->coreCtx->listeners.SendFrameworkEvent(
FrameworkEvent(FrameworkEvent::Type::FRAMEWORK_ERROR,
MakeBundle(bundle->shared_from_this()),
message,
std::make_exception_ptr(ServiceException(
message, ServiceException::Type::FACTORY_ERROR))));
return smap;
if (auto bundle_ = registration->bundle.lock()) {
std::string message =
"ServiceFactory returned an empty or nullptr interface map.";
bundle_->coreCtx->listeners.SendFrameworkEvent(
FrameworkEvent(FrameworkEvent::Type::FRAMEWORK_ERROR,
MakeBundle(bundle->shared_from_this()),
message,
std::make_exception_ptr(ServiceException(
message, ServiceException::Type::FACTORY_ERROR))));
return smap;
}
}
std::vector<std::string> classes =
(registration->properties.Lock(),
Expand All @@ -89,41 +91,50 @@ InterfaceMapConstPtr ServiceReferenceBasePrivate::GetServiceFromFactory(
for (auto clazz : classes) {
if (smap->find(clazz) == smap->end() &&
clazz != "org.cppmicroservices.factory") {
std::string message(
"ServiceFactory produced an object that did not implement: " + clazz);
registration->bundle->coreCtx->listeners.SendFrameworkEvent(
FrameworkEvent(
if (auto bundle_ = registration->bundle.lock()) {
std::string message(
"ServiceFactory produced an object that did not implement: " +
clazz);
bundle_->coreCtx->listeners.SendFrameworkEvent(FrameworkEvent(
FrameworkEvent::Type::FRAMEWORK_WARNING,
MakeBundle(bundle->shared_from_this()),
message,
std::make_exception_ptr(std::logic_error(message.c_str()))));
}
return nullptr;
}
}
s = smap;
} catch (const cppmicroservices::SharedLibraryException& ex) {
registration->bundle->coreCtx->listeners.SendFrameworkEvent(
FrameworkEvent(FrameworkEvent::Type::FRAMEWORK_ERROR,
ex.GetBundle(),
"Failed to load shared library",
std::current_exception()));
if (auto bundle = registration->bundle.lock()) {
bundle->coreCtx->listeners.SendFrameworkEvent(
FrameworkEvent(FrameworkEvent::Type::FRAMEWORK_ERROR,
ex.GetBundle(),
"Failed to load shared library",
std::current_exception()));
}
throw;
} catch (const cppmicroservices::SecurityException& ex) {
bundle->coreCtx->listeners.SendFrameworkEvent(FrameworkEvent{
FrameworkEvent::Type::FRAMEWORK_ERROR,
ex.GetBundle(),
std::string("Failed to load shared library due to a security exception"),
std::current_exception() });
if (bundle) {
bundle->coreCtx->listeners.SendFrameworkEvent(FrameworkEvent{
FrameworkEvent::Type::FRAMEWORK_ERROR,
ex.GetBundle(),
std::string(
"Failed to load shared library due to a security exception"),
std::current_exception() });
}
throw;
} catch (const std::exception& ex) {
s.reset();
std::string message = "ServiceFactory threw an unknown exception.";
registration->bundle->coreCtx->listeners.SendFrameworkEvent(
FrameworkEvent(FrameworkEvent::Type::FRAMEWORK_ERROR,
MakeBundle(bundle->shared_from_this()),
message,
std::make_exception_ptr(ServiceException(
ex.what(), ServiceException::Type::FACTORY_EXCEPTION))));
if (auto bundle_ = registration->bundle.lock()) {
bundle_->coreCtx->listeners.SendFrameworkEvent(FrameworkEvent(
FrameworkEvent::Type::FRAMEWORK_ERROR,
MakeBundle(bundle->shared_from_this()),
message,
std::make_exception_ptr(ServiceException(
ex.what(), ServiceException::Type::FACTORY_EXCEPTION))));
}
}
return s;
}
Expand Down Expand Up @@ -226,7 +237,10 @@ InterfaceMapConstPtr ServiceReferenceBasePrivate::GetServiceInterfaceMap(
std::make_exception_ptr(ServiceException(
msg, ServiceException::FACTORY_RECURSION)));

registration->bundle->coreCtx->listeners.SendFrameworkEvent(fwEvent);
if (auto bundle = registration->bundle.lock()) {
bundle->coreCtx->listeners.SendFrameworkEvent(fwEvent);
}

return nullptr;
}

Expand Down Expand Up @@ -295,14 +309,15 @@ bool ServiceReferenceBasePrivate::UngetPrototypeService(
sf->UngetService(
MakeBundle(bundle), ServiceRegistrationBase(registration), service);
} catch (const std::exception& ex) {
std::string message("ServiceFactory threw an exception");
registration->bundle->coreCtx->listeners.SendFrameworkEvent(
FrameworkEvent(
if (auto bundle_ = registration->bundle.lock()) {
std::string message("ServiceFactory threw an exception");
bundle_->coreCtx->listeners.SendFrameworkEvent(FrameworkEvent(
FrameworkEvent::Type::FRAMEWORK_ERROR,
MakeBundle(bundle->shared_from_this()),
message,
std::make_exception_ptr(ServiceException(
ex.what(), ServiceException::Type::FACTORY_EXCEPTION))));
}
}

auto l = registration->Lock();
Expand Down Expand Up @@ -377,14 +392,15 @@ bool ServiceReferenceBasePrivate::UngetService(
sf->UngetService(
MakeBundle(bundle), ServiceRegistrationBase(registration), sfi);
} catch (const std::exception& ex) {
std::string message("ServiceFactory threw an exception");
registration->bundle->coreCtx->listeners.SendFrameworkEvent(
FrameworkEvent(
if (auto bundle_ = registration->bundle.lock()) {
std::string message("ServiceFactory threw an exception");
bundle_->coreCtx->listeners.SendFrameworkEvent(FrameworkEvent(
FrameworkEvent::Type::FRAMEWORK_ERROR,
MakeBundle(bundle->shared_from_this()),
message,
std::make_exception_ptr(ServiceException(
ex.what(), ServiceException::Type::FACTORY_EXCEPTION))));
}
}
}

Expand Down
73 changes: 43 additions & 30 deletions framework/src/service/ServiceRegistrationBase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,10 @@ void ServiceRegistrationBase::SetProperties(const ServiceProperties& props)
}

// This calls into service event listener hooks. We must not hold any locks here
d->bundle->coreCtx->listeners.GetMatchingServiceListeners(
modifiedEndMatchEvent, before);
if (auto bundle = d->bundle.lock()) {
bundle->coreCtx->listeners.GetMatchingServiceListeners(
modifiedEndMatchEvent, before);
}

int old_rank = 0;
int new_rank = 0;
Expand Down Expand Up @@ -176,17 +178,20 @@ void ServiceRegistrationBase::SetProperties(const ServiceProperties& props)
}
if (old_rank != new_rank) {
auto classes = any_cast<std::vector<std::string>>(objectClasses);
d->bundle->coreCtx->services.UpdateServiceRegistrationOrder(classes);
if (auto bundle = d->bundle.lock()) {
bundle->coreCtx->services.UpdateServiceRegistrationOrder(classes);
}
}

// Notify listeners, we must not hold any locks here
ServiceListeners::ServiceListenerEntries matchingListeners;
d->bundle->coreCtx->listeners.GetMatchingServiceListeners(modifiedEvent,
matchingListeners);
d->bundle->coreCtx->listeners.ServiceChanged(
matchingListeners, modifiedEvent, before);

d->bundle->coreCtx->listeners.ServiceChanged(before, modifiedEndMatchEvent);
if (auto bundle = d->bundle.lock()) {
bundle->coreCtx->listeners.GetMatchingServiceListeners(modifiedEvent,
matchingListeners);
bundle->coreCtx->listeners.ServiceChanged(
matchingListeners, modifiedEvent, before);
bundle->coreCtx->listeners.ServiceChanged(before, modifiedEndMatchEvent);
}
}

void ServiceRegistrationBase::Unregister()
Expand All @@ -203,12 +208,14 @@ void ServiceRegistrationBase::Unregister()
bool isUnregistering(false); // expected state
if (atomic_compare_exchange_strong(
&d->unregistering, &isUnregistering, true)) {
{
auto l1 = d->bundle->coreCtx->services.Lock();
US_UNUSED(l1);
d->bundle->coreCtx->services.RemoveServiceRegistration_unlocked(*this);
if (auto bundle = d->bundle.lock()) {
{
auto l1 = bundle->coreCtx->services.Lock();
US_UNUSED(l1);
bundle->coreCtx->services.RemoveServiceRegistration_unlocked(*this);
}
coreContext = bundle->coreCtx;
}
coreContext = d->bundle->coreCtx;
}

if (isUnregistering) {
Expand All @@ -235,9 +242,11 @@ void ServiceRegistrationBase::Unregister()
US_UNUSED(l);
d->available = false;
auto factoryIter = d->service->find("org.cppmicroservices.factory");
if (d->bundle && factoryIter != d->service->end()) {
serviceFactory =
std::static_pointer_cast<ServiceFactory>(factoryIter->second);
if (auto bundle = d->bundle.lock() && factoryIter != d->service->end()) {
if (bundle) {
serviceFactory =
std::static_pointer_cast<ServiceFactory>(factoryIter->second);
}
}
if (serviceFactory) {
prototypeServiceInstances = d->prototypeServiceInstances;
Expand All @@ -255,12 +264,14 @@ void ServiceRegistrationBase::Unregister()
} catch (const std::exception& ex) {
std::string message(
"ServiceFactory UngetService implementation threw an exception");
d->bundle->coreCtx->listeners.SendFrameworkEvent(FrameworkEvent(
FrameworkEvent::Type::FRAMEWORK_ERROR,
MakeBundle(d->bundle->shared_from_this()),
message,
std::make_exception_ptr(ServiceException(
ex.what(), ServiceException::Type::FACTORY_EXCEPTION))));
if (auto bundle = d->bundle.lock()) {
bundle->coreCtx->listeners.SendFrameworkEvent(FrameworkEvent(
FrameworkEvent::Type::FRAMEWORK_ERROR,
MakeBundle(bundle->shared_from_this()),
message,
std::make_exception_ptr(ServiceException(
ex.what(), ServiceException::Type::FACTORY_EXCEPTION))));
}
}
}
}
Expand All @@ -273,12 +284,14 @@ void ServiceRegistrationBase::Unregister()
} catch (const std::exception& ex) {
std::string message(
"ServiceFactory UngetService implementation threw an exception");
d->bundle->coreCtx->listeners.SendFrameworkEvent(FrameworkEvent(
FrameworkEvent::Type::FRAMEWORK_ERROR,
MakeBundle(d->bundle->shared_from_this()),
message,
std::make_exception_ptr(ServiceException(
ex.what(), ServiceException::Type::FACTORY_EXCEPTION))));
if (auto bundle = d->bundle.lock()) {
bundle->coreCtx->listeners.SendFrameworkEvent(FrameworkEvent(
FrameworkEvent::Type::FRAMEWORK_ERROR,
MakeBundle(bundle->shared_from_this()),
message,
std::make_exception_ptr(ServiceException(
ex.what(), ServiceException::Type::FACTORY_EXCEPTION))));
}
}
}
}
Expand All @@ -287,7 +300,7 @@ void ServiceRegistrationBase::Unregister()
auto l = d->Lock();
US_UNUSED(l);

d->bundle = nullptr;
d->bundle.reset();
d->dependents.clear();
d->service.reset();
d->prototypeServiceInstances.clear();
Expand Down
5 changes: 3 additions & 2 deletions framework/src/service/ServiceRegistrationBasePrivate.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
=============================================================================*/

#include "ServiceRegistrationBasePrivate.h"
#include "BundlePrivate.h"

#include <utility>

Expand All @@ -32,12 +33,12 @@
namespace cppmicroservices {

ServiceRegistrationBasePrivate::ServiceRegistrationBasePrivate(
BundlePrivate* bundle,
BundlePrivate* bundle_,
InterfaceMapConstPtr service,
Properties&& props)
: ref(0)
, service(std::move(service))
, bundle(bundle)
, bundle(bundle_->shared_from_this())
, reference(this)
, properties(std::move(props))
, available(true)
Expand Down
2 changes: 1 addition & 1 deletion framework/src/service/ServiceRegistrationBasePrivate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ class ServiceRegistrationBasePrivate : public detail::MultiThreaded<>
/**
* Bundle registering this service.
*/
BundlePrivate* bundle;
std::weak_ptr<BundlePrivate> bundle;

/**
* Reference object to this service registration.
Expand Down
6 changes: 4 additions & 2 deletions framework/src/service/ServiceRegistry.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,10 @@ void ServiceRegistry::GetRegisteredByBundle(
US_UNUSED(l);

for (auto& sr : serviceRegistrations) {
if (sr.d->bundle == p) {
res.push_back(sr);
if (auto bundle_ = sr.d->bundle.lock()) {
if (bundle_.get() == p) {
res.push_back(sr);
}
}
}
}
Expand Down

0 comments on commit 0ad5875

Please sign in to comment.