commit 4e04c3a31253304dbb92a25a8d0c9e3ad5b1fe31
author Vishnu Nair <vishnun@google.com> Fri Dec 09 20:48:00 2022 +0000
committer Vishnu Nair <vishnun@google.com> Fri Dec 09 20:48:13 2022 +0000
tree 37f23069c2f60d0d022b4fe9394478dd0c14db1b
parent 52363196454b0bfca4c395bb711d806c9b33ea44

SurfaceComposerClient: Handle transaction apply sync timeouts

We switched to using transaction complete callbacks to handle
transaction#apply(/*sync*/=true). The callback object was
destroyed after waiting for the callback. In the event of a
timeout, the callback would access an invalid object.

Fix this by using ref counted object to ensure the context
remains valid.

Fixes: 261679196
Test: atest SurfaceFlinger_test
Change-Id: I4f840214672dd4051cb57b9551bf20802cc90890

libs/gui/SurfaceComposerClient.cpp

diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 1e43700..6fe1fb2 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -971,14 +971,16 @@
 
 class SyncCallback {
 public:
-    static void function(void* callbackContext, nsecs_t /* latchTime */,
-                         const sp<Fence>& /* presentFence */,
-                         const std::vector<SurfaceControlStats>& /* stats */) {
-        if (!callbackContext) {
-            ALOGE("failed to get callback context for SyncCallback");
-        }
-        SyncCallback* helper = static_cast<SyncCallback*>(callbackContext);
-        LOG_ALWAYS_FATAL_IF(sem_post(&helper->mSemaphore), "sem_post failed");
+    static auto getCallback(std::shared_ptr<SyncCallback>& callbackContext) {
+        return [callbackContext](void* /* unused context */, nsecs_t /* latchTime */,
+                                 const sp<Fence>& /* presentFence */,
+                                 const std::vector<SurfaceControlStats>& /* stats */) {
+            if (!callbackContext) {
+                ALOGE("failed to get callback context for SyncCallback");
+                return;
+            }
+            LOG_ALWAYS_FATAL_IF(sem_post(&callbackContext->mSemaphore), "sem_post failed");
+        };
     }
     ~SyncCallback() {
         if (mInitialized) {
@@ -1013,10 +1015,11 @@
         return mStatus;
     }
 
-    SyncCallback syncCallback;
+    std::shared_ptr<SyncCallback> syncCallback = std::make_shared<SyncCallback>();
     if (synchronous) {
-        syncCallback.init();
-        addTransactionCommittedCallback(syncCallback.function, syncCallback.getContext());
+        syncCallback->init();
+        addTransactionCommittedCallback(SyncCallback::getCallback(syncCallback),
+                                        /*callbackContext=*/nullptr);
     }
 
     bool hasListenerCallbacks = !mListenerCallbacks.empty();
@@ -1092,7 +1095,7 @@
     clear();
 
     if (synchronous) {
-        syncCallback.wait();
+        syncCallback->wait();
     }
 
     mStatus = NO_ERROR;