pw_rpc: Fix responding with empty buffers
The Channel::OutputBuffer::Contains() method previously did not check if
the OutputBuffer itself was empty. An empty span sent as a raw response
would appear as a span within the OutputBuffer, even though the
OutputBuffer was never allocated. Instead of allocating an OutputBuffer
for the RPC packet, the responder would attempt to encode it into the
empty OutputBuffer, which would fail.
- Update Contains() so it returns false on an empty OutputBuffer.
- Add tests for OutputBuffer::Contains() and sending an empty response.
Change-Id: I089944079655ed6c21f40c8f19c1d94b942ebdd8
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/53420
Reviewed-by: Alexei Frolov <frolv@google.com>
Commit-Queue: Wyatt Hepler <hepler@google.com>
diff --git a/pw_rpc/channel_test.cc b/pw_rpc/channel_test.cc
index 9dc0c20..0fcad33 100644
--- a/pw_rpc/channel_test.cc
+++ b/pw_rpc/channel_test.cc
@@ -124,5 +124,37 @@
EXPECT_EQ(Status::Aborted(), channel.Send(output_buffer, kTestPacket));
}
+TEST(Channel, OutputBuffer_Contains_FalseWhenEmpty) {
+ Channel::OutputBuffer buffer;
+ EXPECT_FALSE(buffer.Contains({}));
+
+ std::byte data[1];
+ EXPECT_FALSE(buffer.Contains(data));
+}
+
+TEST(Channel, OutputBuffer_Contains_TrueIfContained) {
+ TestOutput<16> output;
+ internal::Channel channel(100, &output);
+
+ Channel::OutputBuffer buffer = channel.AcquireBuffer();
+ EXPECT_TRUE(buffer.Contains(output.buffer()));
+
+ channel.Release(buffer);
+}
+
+TEST(Channel, OutputBuffer_Contains_FalseIfOutside) {
+ TestOutput<16> output;
+ internal::Channel channel(100, &output);
+
+ Channel::OutputBuffer buffer = channel.AcquireBuffer();
+ std::span before(output.buffer().data() - 1, 5);
+ EXPECT_FALSE(buffer.Contains(before));
+
+ std::span after(output.buffer().data() + output.buffer().size() - 1, 2);
+ EXPECT_FALSE(buffer.Contains(after));
+
+ channel.Release(buffer);
+}
+
} // namespace
} // namespace pw::rpc::internal
diff --git a/pw_rpc/public/pw_rpc/internal/channel.h b/pw_rpc/public/pw_rpc/internal/channel.h
index 34dc555..97df034 100644
--- a/pw_rpc/public/pw_rpc/internal/channel.h
+++ b/pw_rpc/public/pw_rpc/internal/channel.h
@@ -30,6 +30,7 @@
constexpr Channel(uint32_t id, ChannelOutput* output)
: rpc::Channel(id, output) {}
+ // Represents a buffer acquired from a ChannelOutput.
class OutputBuffer {
public:
constexpr OutputBuffer() = default;
@@ -52,9 +53,9 @@
// Returns a portion of this OutputBuffer to use as the packet payload.
std::span<std::byte> payload(const Packet& packet) const;
- bool Contains(std::span<const std::byte> buffer) const {
- return buffer.data() >= buffer_.data() &&
- buffer.data() + buffer.size() <= buffer_.data() + buffer_.size();
+ bool Contains(std::span<const std::byte> other) const {
+ return !buffer_.empty() && other.data() >= buffer_.data() &&
+ other.data() + other.size() <= buffer_.data() + buffer_.size();
}
bool empty() const { return buffer_.empty(); }
diff --git a/pw_rpc/public/pw_rpc/internal/responder.h b/pw_rpc/public/pw_rpc/internal/responder.h
index c837d78..1aec967 100644
--- a/pw_rpc/public/pw_rpc/internal/responder.h
+++ b/pw_rpc/public/pw_rpc/internal/responder.h
@@ -48,7 +48,7 @@
public:
Responder(const Responder&) = delete;
- ~Responder() { CloseAndSendResponse(OkStatus()); }
+ ~Responder() { CloseAndSendResponse(OkStatus()).IgnoreError(); }
Responder& operator=(const Responder&) = delete;
diff --git a/pw_rpc/raw/method_test.cc b/pw_rpc/raw/method_test.cc
index 6b456c2..4866539 100644
--- a/pw_rpc/raw/method_test.cc
+++ b/pw_rpc/raw/method_test.cc
@@ -234,6 +234,23 @@
EXPECT_EQ(packet.status(), OkStatus());
}
+TEST(RawServerWriter, Write_EmptyBuffer) {
+ const RawMethod& method = std::get<1>(FakeService::kMethods).raw_method();
+ ServerContextForTest<FakeService> context(method);
+
+ method.Invoke(context.get(), context.request({}));
+
+ ASSERT_EQ(last_writer.Write({}), OkStatus());
+
+ const internal::Packet& packet = context.output().sent_packet();
+ EXPECT_EQ(packet.type(), internal::PacketType::SERVER_STREAM);
+ EXPECT_EQ(packet.channel_id(), context.channel_id());
+ EXPECT_EQ(packet.service_id(), context.service_id());
+ EXPECT_EQ(packet.method_id(), context.get().method().id());
+ EXPECT_TRUE(packet.payload().empty());
+ EXPECT_EQ(packet.status(), OkStatus());
+}
+
TEST(RawServerWriter, Write_Closed_ReturnsFailedPrecondition) {
const RawMethod& method = std::get<1>(FakeService::kMethods).raw_method();
ServerContextForTest<FakeService> context(method);
diff --git a/pw_rpc/responder.cc b/pw_rpc/responder.cc
index 575fa3a..063934a 100644
--- a/pw_rpc/responder.cc
+++ b/pw_rpc/responder.cc
@@ -54,6 +54,7 @@
}
Responder& Responder::operator=(Responder&& other) {
+ // If this RPC was running, complete it before moving in the other RPC.
CloseAndSendResponse(OkStatus()).IgnoreError();
// Move the state variables, which may change when the other client closes.
