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

Commit d53870c5 authored by Corey Tabaka's avatar Corey Tabaka
Browse files

Fix BufferHub state machine to return errors on invalid transitions. 

The consumer side of the BufferHub flow did not have an adequate
state machine to track transitions and return errors when Acquire
or Release are requested from the wrong state. This bug allowed
other buggy usage to go unnoticed.

- Fix the consumer state machine to correctly validate all requests.
- Add tests to verify correctness of the fix.

Tested BufferHub with the new test before and after the fix and
verified that the test catches the problem and that the fix solves
the problem.

Bug: 62886596
Bug: 63149525
Test: bufferhub_tests passes.
Change-Id: I802679ed74c7f505b9243ba4048824350d4e37be
parent a36bf926

libs/vr/libbufferhub/bufferhub_tests.cpp

+51 −0
Original line number Diff line number Diff line
@@ -62,6 +62,57 @@ TEST_F(LibBufferHubTest, TestBasicUsage) { 
  EXPECT_GE(0, RETRY_EINTR(p->Poll(0)));

}



TEST_F(LibBufferHubTest, TestStateTransitions) {

  std::unique_ptr<BufferProducer> p = BufferProducer::Create(

      kWidth, kHeight, kFormat, kUsage, sizeof(uint64_t));

  ASSERT_TRUE(p.get() != nullptr);

  std::unique_ptr<BufferConsumer> c =

      BufferConsumer::Import(p->CreateConsumer());

  ASSERT_TRUE(c.get() != nullptr);



  uint64_t context;

  LocalHandle fence;



  // The producer buffer starts in gained state.



  // Acquire, release, and gain in gained state should fail.

  EXPECT_EQ(-EBUSY, c->Acquire(&fence, &context));

  EXPECT_EQ(-EBUSY, c->Release(LocalHandle()));

  EXPECT_EQ(-EALREADY, p->Gain(&fence));



  // Post in gained state should succeed.

  EXPECT_EQ(0, p->Post(LocalHandle(), kContext));



  // Post, release, and gain in posted state should fail.

  EXPECT_EQ(-EBUSY, p->Post(LocalHandle(), kContext));

  EXPECT_EQ(-EBUSY, c->Release(LocalHandle()));

  EXPECT_EQ(-EBUSY, p->Gain(&fence));



  // Acquire in posted state should succeed.

  EXPECT_LE(0, c->Acquire(&fence, &context));



  // Acquire, post, and gain in acquired state should fail.

  EXPECT_EQ(-EBUSY, c->Acquire(&fence, &context));

  EXPECT_EQ(-EBUSY, p->Post(LocalHandle(), kContext));

  EXPECT_EQ(-EBUSY, p->Gain(&fence));



  // Release in acquired state should succeed.

  EXPECT_EQ(0, c->Release(LocalHandle()));



  // Release, acquire, and post in released state should fail.

  EXPECT_EQ(-EBUSY, c->Release(LocalHandle()));

  EXPECT_EQ(-EBUSY, c->Acquire(&fence, &context));

  EXPECT_EQ(-EBUSY, p->Post(LocalHandle(), kContext));



  // Gain in released state should succeed.

  EXPECT_EQ(0, p->Gain(&fence));



  // Acquire, release, and gain in gained state should fail.

  EXPECT_EQ(-EBUSY, c->Acquire(&fence, &context));

  EXPECT_EQ(-EBUSY, c->Release(LocalHandle()));

  EXPECT_EQ(-EALREADY, p->Gain(&fence));

}



TEST_F(LibBufferHubTest, TestWithCustomMetadata) {

  struct Metadata {

    int64_t field1;

services/vr/bufferhubd/consumer_channel.cpp

+28 −18
Original line number Diff line number Diff line
@@ -8,9 +8,9 @@ 
#include <private/dvr/bufferhub_rpc.h>

#include "producer_channel.h"



using android::pdx::ErrorStatus;

using android::pdx::BorrowedHandle;

using android::pdx::Channel;

using android::pdx::ErrorStatus;

using android::pdx::Message;

using android::pdx::Status;

using android::pdx::rpc::DispatchRemoteMethod;
@@ -22,8 +22,6 @@ ConsumerChannel::ConsumerChannel(BufferHubService* service, int buffer_id, 
                                 int channel_id,

                                 const std::shared_ptr<Channel> producer)

    : BufferHubChannel(service, buffer_id, channel_id, kConsumerType),

      handled_(true),

      ignored_(false),

      producer_(producer) {

  GetProducer()->AddConsumer(this);

}
@@ -34,7 +32,7 @@ ConsumerChannel::~ConsumerChannel() { 
           channel_id(), buffer_id());



  if (auto producer = GetProducer()) {

    if (!handled_)  // Producer is waiting for our Release.

    if (!released_)  // Producer is waiting for our Release.

      producer->OnConsumerIgnored();

    producer->RemoveConsumer(this);

  }
@@ -108,15 +106,20 @@ ConsumerChannel::OnConsumerAcquire(Message& message, 
  if (!producer)

    return ErrorStatus(EPIPE);



  if (ignored_ || handled_) {

  if (acquired_ || released_) {

    ALOGE(

        "ConsumerChannel::OnConsumerAcquire: Acquire when not posted: "

        "ignored=%d handled=%d channel_id=%d buffer_id=%d",

        ignored_, handled_, message.GetChannelId(), producer->buffer_id());

        "ignored=%d acquired=%d released=%d channel_id=%d buffer_id=%d",

        ignored_, acquired_, released_, message.GetChannelId(),

        producer->buffer_id());

    return ErrorStatus(EBUSY);

  } else {

    auto status = producer->OnConsumerAcquire(message, metadata_size);

    if (status) {

      ClearAvailable();

    return producer->OnConsumerAcquire(message, metadata_size);

      acquired_ = true;

    }

    return status;

  }

}
@@ -127,17 +130,21 @@ Status<void> ConsumerChannel::OnConsumerRelease(Message& message, 
  if (!producer)

    return ErrorStatus(EPIPE);



  if (ignored_ || handled_) {

  if (!acquired_ || released_) {

    ALOGE(

        "ConsumerChannel::OnConsumerRelease: Release when not acquired: "

        "ignored=%d handled=%d channel_id=%d buffer_id=%d",

        ignored_, handled_, message.GetChannelId(), producer->buffer_id());

        "ignored=%d acquired=%d released=%d channel_id=%d buffer_id=%d",

        ignored_, acquired_, released_, message.GetChannelId(),

        producer->buffer_id());

    return ErrorStatus(EBUSY);

  } else {

    ClearAvailable();

    auto status =

        producer->OnConsumerRelease(message, std::move(release_fence));

    handled_ = !!status;

    if (status) {

      ClearAvailable();

      acquired_ = false;

      released_ = true;

    }

    return status;

  }

}
@@ -149,12 +156,13 @@ Status<void> ConsumerChannel::OnConsumerSetIgnore(Message&, bool ignored) { 
    return ErrorStatus(EPIPE);



  ignored_ = ignored;

  if (ignored_ && !handled_) {

  if (ignored_ && acquired_) {

    // Update the producer if ignore is set after the consumer acquires the

    // buffer.

    ClearAvailable();

    producer->OnConsumerIgnored();

    handled_ = false;

    acquired_ = false;

    released_ = true;

  }



  return {};
@@ -162,10 +170,12 @@ Status<void> ConsumerChannel::OnConsumerSetIgnore(Message&, bool ignored) { 


bool ConsumerChannel::OnProducerPosted() {

  if (ignored_) {

    handled_ = true;

    acquired_ = false;

    released_ = true;

    return false;

  } else {

    handled_ = false;

    acquired_ = false;

    released_ = false;

    SignalAvailable();

    return true;

  }

services/vr/bufferhubd/consumer_channel.h

+3 −2
Original line number Diff line number Diff line
@@ -38,8 +38,9 @@ class ConsumerChannel : public BufferHubChannel { 
                                      LocalFence release_fence);

  pdx::Status<void> OnConsumerSetIgnore(Message& message, bool ignore);



  bool handled_;  // True if we have processed RELEASE.

  bool ignored_;  // True if we are ignoring events.

  bool acquired_{false};

  bool released_{true};

  bool ignored_{false};  // True if we are ignoring events.

  std::weak_ptr<Channel> producer_;



  ConsumerChannel(const ConsumerChannel&) = delete;

services/vr/bufferhubd/producer_channel.cpp

+3 −3
Original line number Diff line number Diff line
@@ -122,7 +122,7 @@ bool ProducerChannel::HandleMessage(Message& message) { 
}



Status<NativeBufferHandle<BorrowedHandle>> ProducerChannel::OnGetBuffer(

    Message& message) {

    Message& /*message*/) {

  ATRACE_NAME("ProducerChannel::OnGetBuffer");

  ALOGD_IF(TRACE, "ProducerChannel::OnGetBuffer: buffer=%d", buffer_id());

  return {NativeBufferHandle<BorrowedHandle>(buffer_, buffer_id())};
@@ -204,7 +204,7 @@ Status<void> ProducerChannel::OnProducerPost( 
  return {};

}



Status<LocalFence> ProducerChannel::OnProducerGain(Message& message) {

Status<LocalFence> ProducerChannel::OnProducerGain(Message& /*message*/) {

  ATRACE_NAME("ProducerChannel::OnGain");

  ALOGD_IF(TRACE, "ProducerChannel::OnGain: buffer_id=%d", buffer_id());

  if (producer_owns_) {
@@ -224,7 +224,7 @@ Status<LocalFence> ProducerChannel::OnProducerGain(Message& message) { 
}



Status<std::pair<BorrowedFence, BufferWrapper<std::uint8_t*>>>

ProducerChannel::OnConsumerAcquire(Message& message,

ProducerChannel::OnConsumerAcquire(Message& /*message*/,

                                   std::size_t metadata_size) {

  ATRACE_NAME("ProducerChannel::OnConsumerAcquire");

  ALOGD_IF(TRACE, "ProducerChannel::OnConsumerAcquire: buffer_id=%d",