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/services/vr/bufferhubd/consumer_channel.cpp b/services/vr/bufferhubd/consumer_channel.cpp
index 08b2790..ac6896a 100644
--- 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
bool ConsumerChannel::OnProducerPosted() {
if (ignored_) {
- handled_ = true;
+ acquired_ = false;
+ released_ = true;
return false;
} else {
- handled_ = false;
+ acquired_ = false;
+ released_ = false;
SignalAvailable();
return true;
}