From ec24f14bad5a291cb8ffde7012ab68336ed3b05e Mon Sep 17 00:00:00 2001
From: Andrew Scull <ascull@google.com>
Date: Mon, 9 Apr 2018 12:19:33 +0100
Subject: Citadeld: handle requests on multiple threads

Ensure that each app only has one client at a time but allow different
apps to be accessed concurrently. The driver is thread safe so there is
no need to synchronize driver access.

This also allows a hard reset request to be processed if there is a
blocking transaction going on.

Bug: 77728719
Change-Id: I75d5ce8d23d7562d7b6e718ff4925ffad0b637a3
---
 citadel/citadeld/main.cpp | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/citadel/citadeld/main.cpp b/citadel/citadeld/main.cpp
index 3fc9243..2afbdb6 100644
--- a/citadel/citadeld/main.cpp
+++ b/citadel/citadeld/main.cpp
@@ -16,6 +16,7 @@
 
 #include <limits>
 #include <thread>
+#include <mutex>
 
 #include <android-base/logging.h>
 #include <binder/IPCThreadState.h>
@@ -43,32 +44,46 @@ using ::android::hardware::citadel::ICitadeld;
 
 namespace {
 
-struct CitadelProxy : public BnCitadeld {
-    NuggetClient& _client;
-
+class CitadelProxy : public BnCitadeld {
+public:
     CitadelProxy(NuggetClient& client) : _client{client} {}
     ~CitadelProxy() override = default;
 
     Status callApp(const int32_t _appId, const int32_t _arg, const std::vector<uint8_t>& request,
                    std::vector<uint8_t>* const response, int32_t* const _aidl_return) override {
-        // AIDL doesn't support 16-bit integers so validate it before casting
-        if (_arg > std::numeric_limits<uint16_t>::max()) {
+        // AIDL doesn't support integers less than 32-bit so validate it before casting
+        if (_appId < 0 || _appId > kMaxAppId) {
+            LOG(ERROR) << "App ID " << _appId << " is outside the app ID range";
+            return Status::fromExceptionCode(Status::EX_ILLEGAL_ARGUMENT);
+        }
+        if (_arg < 0 || _arg > std::numeric_limits<uint16_t>::max()) {
+            LOG(ERROR) << "Argument " << _arg << " is outside the unsigned 16-bit range";
             return Status::fromExceptionCode(Status::EX_ILLEGAL_ARGUMENT);
         }
 
-        const uint32_t appId = static_cast<uint32_t>(_appId);
+        const uint8_t appId = static_cast<uint32_t>(_appId);
         const uint16_t arg =  static_cast<uint16_t>(_arg);
         uint32_t* const appStatus = reinterpret_cast<uint32_t*>(_aidl_return);
 
+        // Make the call to the app while holding the lock for that app
+        std::unique_lock<std::mutex> lock(_appLocks[appId]);
         *appStatus = _client.CallApp(appId, arg, request, response);
         return Status::ok();
     }
 
     Status reset(bool* const _aidl_return) override {
+        // This doesn't use the transport API to talk to any app so doesn't need
+        // to hold any app locks.
         const nos_device& device = *_client.Device();
         *_aidl_return = (device.ops.reset(device.ctx) == 0);
         return Status::ok();
     }
+
+private:
+    static constexpr auto kMaxAppId = std::numeric_limits<uint8_t>::max();
+
+    NuggetClient& _client;
+    std::mutex _appLocks[kMaxAppId + 1];
 };
 
 [[noreturn]] void CitadelEventDispatcher(const nos_device& device) {
@@ -113,8 +128,8 @@ int main() {
     // registered listeners.
     std::thread event_dispatcher(CitadelEventDispatcher, *citadel.Device());
 
-    // The driver only support single threaded access so we only need to process
-    // Binder transactions on a single thread.
+    // Start handling binder requests with multiple threads
+    ProcessState::self()->startThreadPool();
     IPCThreadState::self()->joinThreadPool();
     return 0;
 }
-- 
cgit v1.2.3