From 001064b7bf403ab254ff6557b2dfa864564f4033 Mon Sep 17 00:00:00 2001 From: Billy Laws Date: Sun, 30 Oct 2022 16:30:04 +0000 Subject: [PATCH] Fix GraphicsBufferProducer recreation We need to use a shared_ptr to ensure that the present callback doesn't do any UAFs, also unlocks the GBP during presentation as if the queue is full a deadlock could a rise where the present callback wouldn't be able to run due to the (waiting) DequeueBuffer thread holding the lock. --- .../hosbinder/GraphicBufferProducer.cpp | 25 +++++++++++-------- .../hosbinder/GraphicBufferProducer.h | 2 +- .../services/hosbinder/IHOSBinderDriver.cpp | 7 +++--- .../services/hosbinder/IHOSBinderDriver.h | 2 +- 4 files changed, 21 insertions(+), 15 deletions(-) diff --git a/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.cpp b/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.cpp index ed288eb2..d3238961 100644 --- a/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.cpp +++ b/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.cpp @@ -264,7 +264,7 @@ namespace skyline::service::hosbinder { return AndroidStatus::BadValue; } - std::scoped_lock lock(mutex); + std::unique_lock lock(mutex); if (slot < 0 || slot >= queue.size()) [[unlikely]] { Logger::Warn("#{} was out of range", slot); return AndroidStatus::BadValue; @@ -383,15 +383,6 @@ namespace skyline::service::hosbinder { throw exception("Application attempting to perform unknown sticky transformation: {:#b}", static_cast(stickyTransform)); } - state.gpu->presentation.Present(buffer.texture, isAutoTimestamp ? 0 : timestamp, swapInterval, crop, scalingMode, transform, fence, [this, &buffer] { - std::unique_lock lock{mutex}; - - buffer.state = BufferState::Free; - bufferEvent->Signal(); - - freeCondition.notify_all(); - }); - buffer.state = BufferState::Queued; buffer.frameNumber = ++frameNumber; @@ -401,6 +392,20 @@ namespace skyline::service::hosbinder { pendingBufferCount = GetPendingBufferCount(); Logger::Debug("#{} - {}Timestamp: {}, Crop: ({}-{})x({}-{}), Scale Mode: {}, Transform: {} [Sticky: {}], Swap Interval: {}, Is Async: {}", slot, isAutoTimestamp ? "Auto " : "", timestamp, crop.left, crop.right, crop.top, crop.bottom, ToString(scalingMode), ToString(transform), ToString(stickyTransform), swapInterval, async); + + // We can present with the mutex locked as if the queue ends up waiting for free space then the lock will never be released as the dequeue callback also locks the lock + lock.unlock(); + + std::weak_ptr weakThis{shared_from_this()}; + state.gpu->presentation.Present(buffer.texture, isAutoTimestamp ? 0 : timestamp, swapInterval, crop, scalingMode, transform, fence, [weakThis, &buffer] { + if (auto gbp{weakThis.lock()}) { + std::scoped_lock lock{gbp->mutex}; + buffer.state = BufferState::Free; + gbp->bufferEvent->Signal(); + gbp->freeCondition.notify_all(); + } + }); + return AndroidStatus::Ok; } diff --git a/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.h b/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.h index 2702f164..544425ed 100644 --- a/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.h +++ b/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.h @@ -57,7 +57,7 @@ namespace skyline::service::hosbinder { * @url https://cs.android.com/android/platform/superproject/+/android-5.1.1_r38:frameworks/native/include/gui/BufferQueueCore.h * @url https://cs.android.com/android/platform/superproject/+/android-5.1.1_r38:frameworks/native/libs/gui/BufferQueueCore.cpp */ - class GraphicBufferProducer { + class GraphicBufferProducer : public std::enable_shared_from_this { private: const DeviceState &state; std::mutex mutex; //!< Synchronizes access to the buffer queue diff --git a/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.cpp b/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.cpp index c6fc4505..44323d0b 100644 --- a/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.cpp +++ b/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.cpp @@ -117,12 +117,13 @@ namespace skyline::service::hosbinder { u64 IHOSBinderDriver::CreateLayer(DisplayId pDisplayId) { if (pDisplayId != displayId) throw exception("Creating layer on unopened display: '{}'", ToString(pDisplayId)); - else if (layer) - throw exception("Creation of multiple layers is not supported"); + else if (layer) // Fallback to replacing previous layer + Logger::Warn("Creation of multiple layers is not supported"); + layerStrongReferenceCount = InitialStrongReferenceCount; layerWeakReferenceCount = 0; - layer.emplace(state, nvMap); + layer = std::make_shared(state, nvMap); return DefaultLayerId; } diff --git a/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.h b/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.h index 80fcc6a1..d2d4ef42 100644 --- a/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.h +++ b/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.h @@ -40,7 +40,7 @@ namespace skyline::service::hosbinder { constexpr static u64 DefaultLayerId{1}; //!< The VI ID of the default (and only) layer in our surface stack constexpr static u32 DefaultBinderLayerHandle{1}; //!< The handle as assigned by SurfaceFlinger of the default layer - std::optional layer; //!< The IGraphicBufferProducer backing the layer (NativeWindow) + std::shared_ptr layer{}; //!< The IGraphicBufferProducer backing the layer (NativeWindow) nvdrv::core::NvMap &nvMap;