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

diff --git a/libs/vr/libbufferhub/bufferhub_tests.cpp b/libs/vr/libbufferhub/bufferhub_tests.cpp
index fa61c4a..1daa5d6 100644 (file)
--- a/libs/vr/libbufferhub/bufferhub_tests.cpp
+++ b/libs/vr/libbufferhub/bufferhub_tests.cpp
@@ -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;

diff --git a/services/vr/bufferhubd/consumer_channel.cpp b/services/vr/bufferhubd/consumer_channel.cpp
index 08b2790..ac6896a 100644 (file)
--- a/services/vr/bufferhubd/consumer_channel.cpp
+++ b/services/vr/bufferhubd/consumer_channel.cpp
@@ -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 {
-    ClearAvailable();
-    return producer->OnConsumerAcquire(message, metadata_size);
+    auto status = producer->OnConsumerAcquire(message, metadata_size);
+    if (status) {
+      ClearAvailable();
+      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;
   }

diff --git a/services/vr/bufferhubd/consumer_channel.h b/services/vr/bufferhubd/consumer_channel.h
index d84055c..208a002 100644 (file)
--- a/services/vr/bufferhubd/consumer_channel.h
+++ b/services/vr/bufferhubd/consumer_channel.h
@@ -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;

diff --git a/services/vr/bufferhubd/producer_channel.cpp b/services/vr/bufferhubd/producer_channel.cpp
index b9984a0..b2db795 100644 (file)
--- a/services/vr/bufferhubd/producer_channel.cpp
+++ b/services/vr/bufferhubd/producer_channel.cpp
@@ -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",