From: Jaemin Ryu Date: Thu, 19 May 2016 02:28:16 +0000 (+0900) Subject: Fix daemon crash in multi-threaded environment X-Git-Tag: accepted/tizen/common/20160524.150236~9 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=fc999b5370c269faf8c4dcf37c2ccabef185ce06;p=platform%2Fcore%2Fsecurity%2Fdevice-policy-manager.git Fix daemon crash in multi-threaded environment Change-Id: Ibbc67875481776e1b23cd280d8ea95ce8a713397 Signed-off-by: Jaemin Ryu --- diff --git a/common/mainloop.cpp b/common/mainloop.cpp index 5ecb608..ecece29 100644 --- a/common/mainloop.cpp +++ b/common/mainloop.cpp @@ -64,7 +64,7 @@ void Mainloop::addEventSource(const int fd, const Event events, Callback&& callb std::lock_guard lock(mutex); if (callbacks.find(fd) != callbacks.end()) { - throw Exception(GetSystemErrorMessage()); + throw Exception("Event source already registered"); } ::memset(&event, 0, sizeof(epoll_event)); diff --git a/common/rmi/client.cpp b/common/rmi/client.cpp index f870851..de85d85 100644 --- a/common/rmi/client.cpp +++ b/common/rmi/client.cpp @@ -37,6 +37,7 @@ void Client::connect() int Client::unsubscribe(const std::string& provider, int id) { + // file descriptor(id) is automatically closed when mainloop callback is destroyed. mainloop.removeEventSource(id); return 0; } @@ -47,12 +48,8 @@ int Client::subscribe(const std::string& provider, const std::string& name) request.packParameters(name); connection->send(request); - Message reply = connection->dispatch(); - if (reply.isError()) { - return -1; - } - FileDescriptor response; + Message reply = connection->dispatch(); reply.disclose(response); return response.fileDescriptor; diff --git a/common/rmi/client.h b/common/rmi/client.h index 2adbcd1..0d3761a 100644 --- a/common/rmi/client.h +++ b/common/rmi/client.h @@ -98,9 +98,8 @@ Type Client::methodCall(const std::string& method, Args&&... args) request.packParameters(std::forward(args)...); connection->send(request); - Message reply = connection->dispatch(); - Type response; + Message reply = connection->dispatch(); reply.disclose(response); return response; diff --git a/common/rmi/connection.cpp b/common/rmi/connection.cpp index b67e40f..f16a529 100644 --- a/common/rmi/connection.cpp +++ b/common/rmi/connection.cpp @@ -43,9 +43,15 @@ void Connection::send(const Message& message) const Message Connection::dispatch() const { Message message; - std::lock_guard lock(receiveMutex); + message.decode(socket); + if (message.isError()) { + std::string exception; + message.disclose(exception); + + throw runtime::Exception(exception); + } return message; } diff --git a/common/rmi/message.cpp b/common/rmi/message.cpp index 4555218..3a9e0f5 100644 --- a/common/rmi/message.cpp +++ b/common/rmi/message.cpp @@ -81,9 +81,12 @@ Message Message::createReplyMessage() const return Message(id(), Reply, target()); } -Message Message::createErrorMessage() const +Message Message::createErrorMessage(const std::string& message) const { - return Message(id(), Error, target()); + Message error(id(), Error, target()); + error.enclose(message); + + return error; } template<> void Message::enclose(FileDescriptor&& fd) diff --git a/common/rmi/message.h b/common/rmi/message.h index 41f7d35..8a00fba 100644 --- a/common/rmi/message.h +++ b/common/rmi/message.h @@ -55,7 +55,7 @@ public: // [TBD] Take arguments Message createReplyMessage() const; - Message createErrorMessage() const; + Message createErrorMessage(const std::string& message) const; unsigned int id() const { diff --git a/common/rmi/notification.cpp b/common/rmi/notification.cpp index e36198d..d2b2f4e 100644 --- a/common/rmi/notification.cpp +++ b/common/rmi/notification.cpp @@ -47,7 +47,7 @@ SubscriptionId Notification::createSubscriber() } std::lock_guard lock(subscriberLock); - subscribers.emplace_back(std::make_shared(fds[0])); + subscribers.push_back(std::make_shared(fds[0])); return SubscriptionId(fds[0], fds[1]); } @@ -56,13 +56,14 @@ int Notification::removeSubscriber(const int id) { std::lock_guard lock(subscriberLock); - std::vector>::iterator it = subscribers.begin(); + std::list>::iterator it = subscribers.begin(); while (it != subscribers.end()) { if ((*it)->getFd() == id) { subscribers.erase(it); return 0; } + ++it; } return -1; diff --git a/common/rmi/notification.h b/common/rmi/notification.h index 72162aa..7da9e51 100644 --- a/common/rmi/notification.h +++ b/common/rmi/notification.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -47,7 +48,7 @@ public: private: std::string signalName; - std::vector> subscribers; + std::list> subscribers; std::mutex subscriberLock; }; @@ -59,7 +60,7 @@ void Notification::notify(Args&&... args) std::lock_guard lock(subscriberLock); - for (std::shared_ptr& subscriber : subscribers) { + for (const std::shared_ptr& subscriber : subscribers) { try { msg.encode(*subscriber); } catch (runtime::Exception& e) { diff --git a/common/rmi/service.cpp b/common/rmi/service.cpp index 52cae3a..c097ec5 100644 --- a/common/rmi/service.cpp +++ b/common/rmi/service.cpp @@ -22,6 +22,7 @@ #include "exception.h" #include "service.h" #include "message.h" +#include "audit/logger.h" namespace rmi { @@ -60,8 +61,6 @@ void Service::stop() Service::ConnectionRegistry::iterator Service::getConnectionIterator(const int id) { - std::lock_guard lock(stateLock); - return std::find_if(connectionRegistry.begin(), connectionRegistry.end(), [id](const std::shared_ptr& connection) { return id == connection->getFd(); @@ -72,6 +71,8 @@ void Service::setNewConnectionCallback(const ConnectionCallback& connectionCallb { auto callback = [connectionCallback, this](const std::shared_ptr& connection) { auto handle = [&](int fd, runtime::Mainloop::Event event) { + std::lock_guard lock(stateLock); + auto iter = getConnectionIterator(fd); if (iter == connectionRegistry.end()) { return; @@ -91,6 +92,7 @@ void Service::setNewConnectionCallback(const ConnectionCallback& connectionCallb mainloop.addEventSource(connection->getFd(), EPOLLIN | EPOLLHUP | EPOLLRDHUP, handle); + std::lock_guard lock(stateLock); connectionRegistry.push_back(connection); } }; @@ -120,12 +122,12 @@ void Service::createNotification(const std::string& name) throw runtime::Exception("Notification already registered"); } - notificationRegistry.emplace(name, name); + notificationRegistry.emplace(name, Notification(name)); } int Service::subscribeNotification(const std::string& name) { - auto closeHandler = [&, name](int fd, runtime::Mainloop::Event event) { + auto closeHandler = [&, name, this](int fd, runtime::Mainloop::Event event) { if ((event & EPOLLHUP) || (event & EPOLLRDHUP)) { unsubscribeNotification(name, fd); return; @@ -146,6 +148,7 @@ int Service::subscribeNotification(const std::string& name) mainloop.addEventSource(slot.first, EPOLLHUP | EPOLLRDHUP, closeHandler); return slot.second; } catch (runtime::Exception& e) { + ERROR(e.what()); return -1; } @@ -164,34 +167,40 @@ int Service::unsubscribeNotification(const std::string& name, const int id) Notification& notification = notificationRegistry[name]; notificationLock.unlock(); - notification.removeSubscriber(id); - mainloop.removeEventSource(id); + notification.removeSubscriber(id); + return 0; } void Service::onMessageProcess(const std::shared_ptr& connection) { - auto process = [&](Message& request) { + // The connection object can be destroyed in main-thread when peer is closed. + // To make sure that the connection object is valid on that situation, + // we should increase the reference count of the shared_ptr by capturing it as value + auto process = [&, connection](Message& request) { try { - //stateLock.lock(); std::shared_ptr methodDispatcher = methodRegistry.at(request.target()); - //stateLock.unlock(); // [TBD] Request authentication before dispatching method handler. processingContext = ProcessingContext(connection); connection->send((*methodDispatcher)(request)); } catch (std::exception& e) { - std::cerr << e.what() << std::endl; - connection->send(request.createErrorMessage()); + try { + // Forward the exception to the peer + connection->send(request.createErrorMessage(e.what())); + } catch (std::exception& ex) { + // The connection is abnormally closed by the peer. + ERROR(ex.what()); + } } }; try { workqueue.submit(std::bind(process, connection->dispatch())); - } catch (runtime::Exception& e) { - std::cerr << e.what() << std::endl; + } catch (std::exception& e) { + ERROR(e.what()); } } diff --git a/common/rmi/service.h b/common/rmi/service.h index 8b1163a..b2d3bc5 100644 --- a/common/rmi/service.h +++ b/common/rmi/service.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -116,7 +117,7 @@ private: Credentials credentials; }; - typedef std::vector> ConnectionRegistry; + typedef std::list> ConnectionRegistry; typedef std::function& connection)> CallbackDispatcher; typedef std::function MethodDispatcher; @@ -140,6 +141,7 @@ private: runtime::ThreadPool workqueue; std::mutex stateLock; std::mutex notificationLock; + std::mutex methodRegistryLock; static thread_local ProcessingContext processingContext; }; @@ -156,7 +158,7 @@ void Service::setMethodHandler(const std::string& method, return reply; }; - std::lock_guard lock(stateLock); + std::lock_guard lock(methodRegistryLock); if (methodRegistry.count(method)) { throw runtime::Exception("Method handler already registered"); @@ -168,6 +170,8 @@ void Service::setMethodHandler(const std::string& method, template void Service::notify(const std::string& name, Args&&... args) { + std::lock_guard lock(notificationLock); + Notification& slot = notificationRegistry[name]; slot.notify(name, std::forward(args)...); } diff --git a/libs/policy-client.cpp b/libs/policy-client.cpp index 43db759..8a61037 100644 --- a/libs/policy-client.cpp +++ b/libs/policy-client.cpp @@ -32,6 +32,7 @@ DevicePolicyContext::DevicePolicyContext() noexcept DevicePolicyContext::~DevicePolicyContext() noexcept { + disconnect(); } int DevicePolicyContext::connect(const std::string& address) noexcept @@ -64,8 +65,13 @@ int DevicePolicyContext::subscribePolicyChange(const std::string& name, listener(policy.c_str(), state.c_str(), data); }; - return client->subscribe(SUBSCRIBER_REGISTER, - name, listenerDispatcher); + try { + return client->subscribe(SUBSCRIBER_REGISTER, + name, listenerDispatcher); + } catch (runtime::Exception& e) { + std::cout << e.what() << std::endl; + return -1; + } } int DevicePolicyContext::unsubscribePolicyChange(int subscriberId) @@ -81,8 +87,13 @@ int DevicePolicyContext::subscribeSignal(const std::string& name, listener(from.c_str(), object.c_str(), data); }; - return client->subscribe - (SUBSCRIBER_REGISTER, name, listenerDispatcher); + try { + return client->subscribe(SUBSCRIBER_REGISTER, + name, listenerDispatcher); + } catch (runtime::Exception& e) { + std::cout << e.what() << std::endl; + return -1; + } } int DevicePolicyContext::unsubscribeSignal(int subscriberId) diff --git a/libs/restriction.cpp b/libs/restriction.cpp index f0ae728..3ae91d4 100644 --- a/libs/restriction.cpp +++ b/libs/restriction.cpp @@ -172,7 +172,6 @@ int RestrictionPolicy::getLocationState() } } - int RestrictionPolicy::setWifiState(bool enable) { try { diff --git a/tests/api/CMakeLists.txt b/tests/api/CMakeLists.txt index be570e8..2dd7159 100644 --- a/tests/api/CMakeLists.txt +++ b/tests/api/CMakeLists.txt @@ -17,6 +17,7 @@ SET(API_TEST_TARGET "dpm-api-tests") SET(API_TEST_SOURCES main.c testbench.c + context.c bluetooth.c # password.c restriction.c diff --git a/tests/api/context.c b/tests/api/context.c new file mode 100644 index 0000000..8feb3af --- /dev/null +++ b/tests/api/context.c @@ -0,0 +1,153 @@ +// Copyright (c) 2015 Samsung Electronics Co., Ltd. +// +// 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 +#include +#include + +#include + +#include "testbench.h" + +#define MAX_WORKER_THREADS 8 +#define MAX_ITERATIONS 1000 + +volatile int completed = 0; + +void device_policy_context_callback(const char* name, const char* state, void* user_data) +{ + int *triggered = user_data; + printf("*"); + *triggered = 1; +} + +void* getter(void* data) +{ + int i = 0; + dpm_context_h context; + volatile int triggered = 0;; + + printf("Policy receiver %d is ready\n", *((int *)data)); + + while(1) { + context = dpm_context_create(); + if (context == NULL) { + printf("Failed to create client context\n"); + return (void *)TEST_FAILED; + } + + int id; + dpm_context_add_policy_changed_cb(context, "camera", device_policy_context_callback, (void *)&triggered, &id); + + while (!triggered) { + if (completed) { + dpm_context_remove_policy_changed_cb(context, id); + dpm_context_destroy(context); + return (void *)TEST_SUCCESSED; + } + } + + triggered = 0; + + if ((i % 10) == 0) { + printf("\n"); + } + + dpm_context_remove_policy_changed_cb(context, id); + dpm_context_destroy(context); + + printf("G"); + + i++; + } + + return (void *)TEST_SUCCESSED; +} + +void* setter(void *data) +{ + int i; + dpm_context_h context; + + printf("Thread setter %d is ready\n", *((int *)data)); + + for (i = 0; i < MAX_ITERATIONS; i++) { + context = dpm_context_create(); + if (context == NULL) { + printf("Failed to create client context\n"); + completed = 1; + return (void *)TEST_FAILED; + } + + dpm_restriction_policy_h policy = dpm_context_acquire_restriction_policy(context); + + int state = 0; + + dpm_restriction_get_camera_state(policy, &state); + dpm_restriction_set_camera_state(policy, state ? 0 : 1); + + dpm_context_release_restriction_policy(context, policy); + + if ((i % 10) == 0) { + printf("\n"); + } + + printf("S"); + + dpm_context_destroy(context); + + } + printf("\n"); + + completed = 1; + + return (void *)TEST_SUCCESSED; +} + +static int device_policy_context(struct testcase* tc) +{ + pthread_t handle[MAX_WORKER_THREADS]; + int i, ret, status, idx[MAX_WORKER_THREADS]; + + for (i = 0; i < MAX_WORKER_THREADS; i++) { + idx[i] = i; + + if (i == 0) { + pthread_create(&handle[i], NULL, setter, (void *)&idx[i]); + } else { + pthread_create(&handle[i], NULL, getter, (void *)&idx[i]); + } + } + + ret = TEST_SUCCESSED; + for (i = 0; i < MAX_WORKER_THREADS; i++) { + pthread_join(handle[i], (void *)&status); + if (status == TEST_FAILED) { + ret = TEST_FAILED; + } + } + + return ret; +} + +struct testcase device_policy_context_testcase = { + .description = "device policy context", + .handler = device_policy_context +}; + +void TESTCASE_CONSTRUCTOR device_policy_context_build_testcase(void) +{ + testbench_populate_testcase(&device_policy_context_testcase); +}