pw_rpc: Release acquired buffer in RawServerWriter destructor
This fixes a bug in the RawServerWriter where it would crash if the user
acquired a payload buffer but didn't release it.
Change-Id: I684f3c62a6f24d9a827e3d5258502f0bbfd0ddd4
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/23380
Commit-Queue: Alexei Frolov <frolv@google.com>
Pigweed-Auto-Submit: Alexei Frolov <frolv@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>
diff --git a/pw_rpc/base_server_writer.cc b/pw_rpc/base_server_writer.cc
index 942cf32..8c57ee1 100644
--- a/pw_rpc/base_server_writer.cc
+++ b/pw_rpc/base_server_writer.cc
@@ -78,6 +78,15 @@
return call_.channel().Send(response_, ResponsePacket(payload));
}
+Status BaseServerWriter::ReleasePayloadBuffer() {
+ if (!open()) {
+ return Status::FailedPrecondition();
+ }
+
+ call_.channel().Release(response_);
+ return Status::Ok();
+}
+
void BaseServerWriter::Close() {
if (!open()) {
return;
diff --git a/pw_rpc/nanopb/public/pw_rpc/internal/nanopb_method.h b/pw_rpc/nanopb/public/pw_rpc/internal/nanopb_method.h
index 8f8c4c5..7c5dd9a 100644
--- a/pw_rpc/nanopb/public/pw_rpc/internal/nanopb_method.h
+++ b/pw_rpc/nanopb/public/pw_rpc/internal/nanopb_method.h
@@ -299,7 +299,7 @@
return ReleasePayloadBuffer(buffer.first(result.size()));
}
- ReleasePayloadBuffer({});
+ ReleasePayloadBuffer();
return Status::Internal();
}
diff --git a/pw_rpc/public/pw_rpc/internal/base_server_writer.h b/pw_rpc/public/pw_rpc/internal/base_server_writer.h
index ef36d38..4b9dbad 100644
--- a/pw_rpc/public/pw_rpc/internal/base_server_writer.h
+++ b/pw_rpc/public/pw_rpc/internal/base_server_writer.h
@@ -70,8 +70,12 @@
std::span<std::byte> AcquirePayloadBuffer();
+ // Releases the buffer, sending a packet with the specified payload.
Status ReleasePayloadBuffer(std::span<const std::byte> payload);
+ // Releases the buffer without sending a packet.
+ Status ReleasePayloadBuffer();
+
private:
friend class rpc::Server;
diff --git a/pw_rpc/public/pw_rpc/internal/channel.h b/pw_rpc/public/pw_rpc/internal/channel.h
index 870e425..6533eda 100644
--- a/pw_rpc/public/pw_rpc/internal/channel.h
+++ b/pw_rpc/public/pw_rpc/internal/channel.h
@@ -56,6 +56,8 @@
buffer.data() + buffer.size() <= buffer_.data() + buffer_.size();
}
+ bool empty() const { return buffer_.empty(); }
+
private:
friend class Channel;
@@ -76,6 +78,11 @@
}
Status Send(OutputBuffer& output, const internal::Packet& packet);
+
+ void Release(OutputBuffer& buffer) {
+ buffer.buffer_ = {};
+ output().SendAndReleaseBuffer(0);
+ }
};
} // namespace pw::rpc::internal
diff --git a/pw_rpc/raw/public/pw_rpc/internal/raw_method.h b/pw_rpc/raw/public/pw_rpc/internal/raw_method.h
index e0a514f..a1e3b66 100644
--- a/pw_rpc/raw/public/pw_rpc/internal/raw_method.h
+++ b/pw_rpc/raw/public/pw_rpc/internal/raw_method.h
@@ -24,6 +24,10 @@
class RawServerWriter : public internal::BaseServerWriter {
public:
RawServerWriter() = default;
+ RawServerWriter(RawServerWriter&&) = default;
+ RawServerWriter& operator=(RawServerWriter&&) = default;
+
+ ~RawServerWriter();
// Returns a buffer in which a response payload can be built.
ByteSpan PayloadBuffer() { return AcquirePayloadBuffer(); }
diff --git a/pw_rpc/raw/raw_method.cc b/pw_rpc/raw/raw_method.cc
index 1b2075f..b23ff5b 100644
--- a/pw_rpc/raw/raw_method.cc
+++ b/pw_rpc/raw/raw_method.cc
@@ -21,6 +21,12 @@
namespace pw::rpc {
+RawServerWriter::~RawServerWriter() {
+ if (!buffer().empty()) {
+ ReleasePayloadBuffer();
+ }
+}
+
Status RawServerWriter::Write(ConstByteSpan response) {
if (buffer().Contains(response)) {
return ReleasePayloadBuffer(response);
@@ -29,7 +35,7 @@
std::span<std::byte> buffer = AcquirePayloadBuffer();
if (response.size() > buffer.size()) {
- ReleasePayloadBuffer({});
+ ReleasePayloadBuffer();
return Status::OutOfRange();
}
diff --git a/pw_rpc/raw/raw_method_test.cc b/pw_rpc/raw/raw_method_test.cc
index 25b6e95..9b03402 100644
--- a/pw_rpc/raw/raw_method_test.cc
+++ b/pw_rpc/raw/raw_method_test.cc
@@ -174,5 +174,24 @@
EXPECT_EQ(last_writer.Write(data), Status::OutOfRange());
}
+TEST(RawServerWriter,
+ Destructor_ReleasesAcquiredBufferWithoutSendingAndCloses) {
+ const RawMethod& method = std::get<1>(FakeService::kMethods).raw_method();
+ ServerContextForTest<FakeService> context(method);
+
+ method.Invoke(context.get(), context.packet({}));
+
+ {
+ RawServerWriter writer = std::move(last_writer);
+ auto buffer = writer.PayloadBuffer();
+ buffer[0] = std::byte{'!'};
+ // Don't release the buffer.
+ }
+
+ auto output = context.output();
+ EXPECT_EQ(output.packet_count(), 1u);
+ EXPECT_EQ(output.sent_packet().type(), PacketType::SERVER_STREAM_END);
+}
+
} // namespace
} // namespace pw::rpc::internal
