Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 4fa10733 authored by Daniel Nicoara's avatar Daniel Nicoara
Browse files

Fix potential access to invalid memory during shutdown 

Since binder threads have a reference to the callback, need to make sure
it is properly reset before deleting the DvrHwcClient object.

Bug: 63139142
Test: Ran on device to ensure VrCore restart don't result in crashes in
DvrHwcClient
Change-Id: I559844a70d4483fee3148526f704d234994a96d4
parent bf72631a

libs/vr/libdvr/dvr_hardware_composer_client.cpp

+36 −9
Original line number Diff line number Diff line
@@ -6,7 +6,9 @@ 
#include <binder/IServiceManager.h>

#include <private/android/AHardwareBufferHelpers.h>



#include <functional>

#include <memory>

#include <mutex>



struct DvrHwcFrame {

  android::dvr::ComposerView::Frame frame;
@@ -16,10 +18,15 @@ namespace { 


class HwcCallback : public android::dvr::BnVrComposerCallback {

 public:

  explicit HwcCallback(DvrHwcOnFrameCallback callback,

                       void* client_state);

  using CallbackFunction = std::function<int(DvrHwcFrame*)>;



  explicit HwcCallback(const CallbackFunction& callback);

  ~HwcCallback() override;



  // Reset the callback. This needs to be done early to avoid use after free

  // accesses from binder thread callbacks.

  void Shutdown();



  std::unique_ptr<DvrHwcFrame> DequeueFrame();



 private:
@@ -28,26 +35,41 @@ class HwcCallback : public android::dvr::BnVrComposerCallback { 
      const android::dvr::ParcelableComposerFrame& frame,

      android::dvr::ParcelableUniqueFd* fence) override;



  DvrHwcOnFrameCallback callback_;

  void* client_state_;

  // Protects the |callback_| from uses from multiple threads. During shutdown

  // there may be in-flight frame update events. In those cases the callback

  // access needs to be protected otherwise binder threads may access an invalid

  // callback.

  std::mutex mutex_;

  CallbackFunction callback_;



  HwcCallback(const HwcCallback&) = delete;

  void operator=(const HwcCallback&) = delete;

};



HwcCallback::HwcCallback(DvrHwcOnFrameCallback callback, void* client_state)

    : callback_(callback), client_state_(client_state) {}

HwcCallback::HwcCallback(const CallbackFunction& callback)

    : callback_(callback) {}



HwcCallback::~HwcCallback() {}



void HwcCallback::Shutdown() {

  std::lock_guard<std::mutex> guard(mutex_);

  callback_ = nullptr;

}



android::binder::Status HwcCallback::onNewFrame(

    const android::dvr::ParcelableComposerFrame& frame,

    android::dvr::ParcelableUniqueFd* fence) {

  std::lock_guard<std::mutex> guard(mutex_);



  if (!callback_) {

    fence->set_fence(android::base::unique_fd());

    return android::binder::Status::ok();

  }



  std::unique_ptr<DvrHwcFrame> dvr_frame(new DvrHwcFrame());

  dvr_frame->frame = frame.frame();



  fence->set_fence(android::base::unique_fd(callback_(client_state_,

                                                      dvr_frame.release())));

  fence->set_fence(android::base::unique_fd(callback_(dvr_frame.release())));

  return android::binder::Status::ok();

}
@@ -67,7 +89,8 @@ DvrHwcClient* dvrHwcClientCreate(DvrHwcOnFrameCallback callback, void* data) { 
  if (!client->composer.get())

    return nullptr;



  client->callback = new HwcCallback(callback, data);

  client->callback = new HwcCallback(std::bind(callback, data,

                                               std::placeholders::_1));

  android::binder::Status status = client->composer->registerObserver(

      client->callback);

  if (!status.isOk())
@@ -77,6 +100,10 @@ DvrHwcClient* dvrHwcClientCreate(DvrHwcOnFrameCallback callback, void* data) { 
}



void dvrHwcClientDestroy(DvrHwcClient* client) {

  // NOTE: Deleting DvrHwcClient* isn't enough since DvrHwcClient::callback is a

  // shared pointer that could be referenced from a binder thread. But the

  // client callback isn't valid past this calls so that needs to be reset.

  client->callback->Shutdown();

  delete client;

}