pw_transfer: Use correct chunk ID when resource doesn't exist
In transfer protocol v2, session IDs are assigned by the server. When a
client sends its opening chunk, it does not yet know its session ID and
expects a response that is identified by the resource ID it is trying to
transfer.
In response to an opening chunk from the client, the server looks up a
handler for the requested resource. If one is not found, the transfer
thread responds directly without assigning a context for the transfer.
This direct response did not account for the initial session/resource ID
discrepancy, instead sending its NOT_FOUND chunk with the newly assigned
session ID of which the client had no knowledge.
This updates the transfer thread to always use resource ID to identify
an error in response to an opening chunk. In legacy protocol, the two
are the same, whereas in v2, resource ID is expected.
Bug: 252870476
Change-Id: Ia594286976e263b6a46cf0bc582a848122386471
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/113990
Commit-Queue: Alexei Frolov <frolv@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>
diff --git a/pw_transfer/transfer_thread.cc b/pw_transfer/transfer_thread.cc
index 449408c..13d2461 100644
--- a/pw_transfer/transfer_thread.cc
+++ b/pw_transfer/transfer_thread.cc
@@ -156,7 +156,10 @@
// No handler exists for the transfer: return a NOT_FOUND.
next_event_.type = EventType::kSendStatusChunk;
next_event_.send_status_chunk = {
- .session_id = session_id,
+ // Identify the status chunk using the requested resource ID rather
+ // than the session ID. In legacy, the two are the same, whereas in
+ // v2+ the client has not yet been assigned a session.
+ .session_id = resource_id,
.protocol_version = version,
.status = Status::NotFound().code(),
.stream = type == TransferType::kTransmit
diff --git a/pw_transfer/transfer_thread_test.cc b/pw_transfer/transfer_thread_test.cc
index b7524bb..dbfb112 100644
--- a/pw_transfer/transfer_thread_test.cc
+++ b/pw_transfer/transfer_thread_test.cc
@@ -301,5 +301,38 @@
transfer_thread_.EndClientTransfer(4, Status::Cancelled());
}
+TEST_F(TransferThreadTest, VersionTwo_NoHandler) {
+ auto reader_writer = ctx_.reader_writer();
+ transfer_thread_.SetServerReadStream(reader_writer);
+
+ SimpleReadTransfer handler(3, kData);
+ transfer_thread_.AddTransferHandler(handler);
+ transfer_thread_.RemoveTransferHandler(handler);
+
+ transfer_thread_.StartServerTransfer(internal::TransferType::kTransmit,
+ ProtocolVersion::kVersionTwo,
+ /*session_id=*/421,
+ /*resource_id=*/7,
+ {},
+ max_parameters_,
+ std::chrono::seconds(2),
+ 0);
+
+ transfer_thread_.WaitUntilEventIsProcessed();
+
+ EXPECT_FALSE(handler.prepare_read_called);
+
+ ASSERT_EQ(ctx_.total_responses(), 1u);
+ Result<uint32_t> id = Chunk::ExtractIdentifier(ctx_.response());
+ ASSERT_TRUE(id.ok());
+ EXPECT_EQ(id.value(), 7u);
+ auto chunk = DecodeChunk(ctx_.response());
+ EXPECT_EQ(chunk.session_id(), 7u);
+ ASSERT_TRUE(chunk.status().has_value());
+ EXPECT_EQ(chunk.status().value(), Status::NotFound());
+
+ transfer_thread_.RemoveTransferHandler(handler);
+}
+
} // namespace
} // namespace pw::transfer::test
