Skip to content

Commit

Permalink
pw_bluetooth: Emboss util fn for copying from a container
Browse files Browse the repository at this point in the history 
Naming is based on Emboss's TryToCopyFrom and UncheckedCopyFrom
function names.

Also fixes ubsan warning so we can re-enable ubsan for
pw_bluetooth_proxy_test.

Bug: 385399636
Change-Id: Ife8837819de5fc584fd1e5f3e955200f23d5d18a
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/259032
Reviewed-by: Austin Foxley <[email protected]>
Lint: Lint 🤖 <[email protected]>
Reviewed-by: Ben Lawson <[email protected]>
Presubmit-Verified: CQ Bot Account <[email protected]>
Commit-Queue: David Rees <[email protected]>
Docs-Not-Needed: David Rees <[email protected]>
  • Loading branch information
@studgeek
studgeek authored and CQ Bot Account committed Jan 9, 2025
1 parent 8adc4c3 commit 818f8c1
Show file tree
Hide file tree
Showing 11 changed files with 197 additions and 31 deletions.
132 changes: 132 additions & 0 deletions pw_bluetooth/emboss_util_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,137 @@ TEST(EmbossUtilTest, MakeEmbossWriterFromSpan) {
EXPECT_EQ(failed_view.status(), pw::Status::InvalidArgument());
}

TEST(CopyToEmbossTest, CopyArrayToEmboss) {
std::array<uint8_t, 7> e_array = {0x00, 0x01, 0x02, 0x03};
auto e_span = pw::span(e_array);
PW_TEST_ASSERT_OK_AND_ASSIGN(
emboss::TestCommandPacketWithArrayPayloadWriter e_view,
MakeEmbossWriter<emboss::TestCommandPacketWithArrayPayloadWriter>(
e_span));

std::array<uint8_t, 4> t1_array = {33, 71, 24, 91};

UncheckedCopyToEmbossStruct(e_view.payload(), t1_array);
EXPECT_TRUE(std::equal(e_view.payload().BackingStorage().begin(),
e_view.payload().BackingStorage().end(),
t1_array.begin(),
t1_array.end()));
}

TEST(CopyToEmbossTest, CopySpanToEmboss) {
std::array<uint8_t, 7> e_array = {0x00, 0x01, 0x02, 0x03};
PW_TEST_ASSERT_OK_AND_ASSIGN(
emboss::TestCommandPacketWithArrayPayloadWriter e_view,
MakeEmbossWriter<emboss::TestCommandPacketWithArrayPayloadWriter>(
pw::span{e_array}));

std::array<uint8_t, 4> t1_array = {33, 71, 24, 91};

UncheckedCopyToEmbossStruct(e_view.payload(), pw::span(t1_array));
EXPECT_TRUE(std::equal(e_view.payload().BackingStorage().begin(),
e_view.payload().BackingStorage().end(),
t1_array.begin(),
t1_array.end()));
}

TEST(CopyToEmbossTest, CopySmallerToEmboss) {
std::array<uint8_t, 7> e_array = {0x00, 0x01, 0x02, 0x03};
PW_TEST_ASSERT_OK_AND_ASSIGN(
emboss::TestCommandPacketWithArrayPayloadWriter e_view,
MakeEmbossWriter<emboss::TestCommandPacketWithArrayPayloadWriter>(
pw::span{e_array}));

std::array<uint8_t, 2> t1_array = {33, 71};

UncheckedCopyToEmbossStruct(e_view.payload(), t1_array);
EXPECT_TRUE(
std::equal(e_view.payload().BackingStorage().begin(),
e_view.payload().BackingStorage().begin() + t1_array.size(),
t1_array.begin(),
t1_array.end()));
}

TEST(CopyToEmbossTest, CopyEmptyToEmboss) {
std::array<uint8_t, 7> e_array = {0x00, 0x01, 0x02, 0x03};
PW_TEST_ASSERT_OK_AND_ASSIGN(
emboss::TestCommandPacketWithArrayPayloadWriter e_view,
MakeEmbossWriter<emboss::TestCommandPacketWithArrayPayloadWriter>(
pw::span{e_array}));

std::array<uint8_t, 0> t1_array = {};

UncheckedCopyToEmbossStruct(e_view.payload(), t1_array);
// Emboss view's underlying array is unchanged.
EXPECT_TRUE(std::equal(e_view.BackingStorage().begin(),
e_view.BackingStorage().end(),
e_array.begin(),
e_array.end()));
}

TEST(CopyToEmbossTest, TryToCopyEmptyToEmboss) {
std::array<uint8_t, 7> e_array = {0x00, 0x01, 0x02, 0x03};
PW_TEST_ASSERT_OK_AND_ASSIGN(
emboss::TestCommandPacketWithArrayPayloadWriter e_view,
MakeEmbossWriter<emboss::TestCommandPacketWithArrayPayloadWriter>(
pw::span{e_array}));

std::array<uint8_t, 0> t1_array = {};

EXPECT_TRUE(TryToCopyToEmbossStruct(e_view.payload(), t1_array));
// Emboss view's underlying array is unchanged.
EXPECT_TRUE(std::equal(e_view.BackingStorage().begin(),
e_view.BackingStorage().end(),
e_array.begin(),
e_array.end()));
}

TEST(CopyToEmbossTest, TryToCopyToEmboss) {
std::array<uint8_t, 7> e_array = {0x00, 0x01, 0x02, 0x03};
PW_TEST_ASSERT_OK_AND_ASSIGN(
emboss::TestCommandPacketWithArrayPayloadWriter e_view,
MakeEmbossWriter<emboss::TestCommandPacketWithArrayPayloadWriter>(
pw::span{e_array}));

std::array<uint8_t, 4> t1_array = {33, 71, 24, 91};

EXPECT_TRUE(TryToCopyToEmbossStruct(e_view.payload(), t1_array));
EXPECT_TRUE(std::equal(e_view.payload().BackingStorage().begin(),
e_view.payload().BackingStorage().end(),
t1_array.begin(),
t1_array.end()));
}

TEST(CopyToEmbossTest, TryToCopyTooLargeToEmboss) {
std::array<uint8_t, 7> e_array = {0x00, 0x01, 0x02, 0x03};
PW_TEST_ASSERT_OK_AND_ASSIGN(
emboss::TestCommandPacketWithArrayPayloadWriter e_view,
MakeEmbossWriter<emboss::TestCommandPacketWithArrayPayloadWriter>(
pw::span{e_array}));

std::array<uint8_t, 5> t1_array = {33, 71, 24, 91, 99};

EXPECT_FALSE(TryToCopyToEmbossStruct(e_view.payload(), t1_array));
// Emboss view's underlying array is unchanged.
EXPECT_TRUE(std::equal(e_view.BackingStorage().begin(),
e_view.BackingStorage().end(),
e_array.begin(),
e_array.end()));
}

TEST(CopyToEmbossTest, TryToCopyToIncompleteEmboss) {
std::array<uint8_t, 6> e_array = {0x00, 0x01, 0x02, 0x03};
emboss::TestCommandPacketWithArrayPayloadWriter e_view{e_array.data(),
e_array.size()};

std::array<uint8_t, 5> t1_array = {33, 71, 24, 91, 99};

EXPECT_FALSE(TryToCopyToEmbossStruct(e_view.payload(), t1_array));
// Emboss view's underlying array is unchanged.
EXPECT_TRUE(std::equal(e_view.BackingStorage().begin(),
e_view.BackingStorage().end(),
e_array.begin(),
e_array.end()));
}

} // namespace
} // namespace pw::bluetooth::proxy
31 changes: 31 additions & 0 deletions pw_bluetooth/public/pw_bluetooth/emboss_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// the License.
#pragma once

#include <cstring>
#include <type_traits>

#include "pw_result/result.h"
Expand Down Expand Up @@ -84,4 +85,34 @@ constexpr inline Result<EmbossT> MakeEmbossWriter(ContainerT&& buffer) {
return MakeEmbossWriter<EmbossT>(buffer.data(), buffer.size());
}

/// Copy from a container to an Emboss object's backing storage.
///
/// Container needs to support `data()` and `size()` (in bytes) functions.
template <typename EmbossT, typename ContainerT>
constexpr void UncheckedCopyToEmbossStruct(EmbossT emboss_dest,
ContainerT&& src) {
// std::memcpy allows size zero, but ubsan will complain for some empty
// containers so just skip copy in those cases.
if (src.size() == 0) {
return;
}
std::memcpy(emboss_dest.BackingStorage().data(), src.data(), src.size());
}

/// Try to copy from a container to an Emboss object's backing storage.
///
/// Returns false if the Emboss object is not Ok and or can't fit the
/// container's contents.
///
/// Container needs to support `data()` and `size()` (in bytes) functions.
template <typename EmbossT, typename ContainerT>
[[nodiscard]] constexpr bool TryToCopyToEmbossStruct(EmbossT emboss_dest,
ContainerT&& src) {
if (!emboss_dest.IsComplete() || src.size() > emboss_dest.SizeInBytes()) {
return false;
}
UncheckedCopyToEmbossStruct(emboss_dest, src);
return true;
}

} // namespace pw::bluetooth
7 changes: 7 additions & 0 deletions pw_bluetooth/public/pw_bluetooth/hci_test.emb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ struct TestCommandPacket:
$next [+1] UInt payload


struct TestCommandPacketWithArrayPayload:
-- Test HCI Command packet with single byte payload.
let hdr_size = hci.CommandHeader.$size_in_bytes
0 [+hdr_size] hci.CommandHeader header
$next [+4] UInt:8[4] payload


struct TestEventPacket:
-- Test HCI Event packet with single byte payload.
let hdr_size = hci.EventHeader.$size_in_bytes
Expand Down
2 changes: 0 additions & 2 deletions pw_bluetooth_proxy/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,6 @@ pw_cc_test(
"rfcomm_test.cc",
"utils_test.cc",
],
# TODO: b/385375184 - Fix this test!
tags = ["noubsan"],
deps = [
":pw_bluetooth_proxy",
":test_utils",
Expand Down
3 changes: 1 addition & 2 deletions pw_bluetooth_proxy/basic_l2cap_channel.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ pw::Status BasicL2capChannel::Write(pw::span<const uint8_t> payload) {
MakeEmbossWriter<emboss::BFrameWriter>(
acl.payload().BackingStorage().data(),
acl.payload().BackingStorage().SizeInBytes()));
std::memcpy(
bframe.payload().BackingStorage().data(), payload.data(), payload.size());
PW_CHECK(TryToCopyToEmbossStruct(bframe.payload(), payload));

return QueuePacket(std::move(h4_packet));
}
Expand Down
7 changes: 4 additions & 3 deletions pw_bluetooth_proxy/gatt_notify_channel.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#include "pw_bluetooth_proxy/gatt_notify_channel.h"

#include "pw_assert/check.h" // IWYU pragma: keep
#include "pw_bluetooth/att.emb.h"
#include "pw_bluetooth/emboss_util.h"
#include "pw_bluetooth/l2cap_frames.emb.h"
Expand Down Expand Up @@ -68,9 +69,9 @@ pw::Status GattNotifyChannel::Write(pw::span<const uint8_t> attribute_value) {
att_size));
att_notify.attribute_opcode().Write(emboss::AttOpcode::ATT_HANDLE_VALUE_NTF);
att_notify.attribute_handle().Write(attribute_handle_);
std::memcpy(att_notify.attribute_value().BackingStorage().data(),
attribute_value.data(),
attribute_value.size());

PW_CHECK(
TryToCopyToEmbossStruct(att_notify.attribute_value(), attribute_value));

return QueuePacket(std::move(h4_packet));
}
Expand Down
12 changes: 6 additions & 6 deletions pw_bluetooth_proxy/l2cap_coc.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,6 @@ std::optional<H4PacketWithH4> L2capCoc::GenerateNextTxPacket() {
MakeEmbossWriter<emboss::AclDataFrameWriter>(h4_packet.GetHciSpan());
PW_CHECK(acl.ok());

uint8_t* payload_start;
if (!is_continuing_segment_) {
Result<emboss::FirstKFrameWriter> first_kframe_writer =
MakeEmbossWriter<emboss::FirstKFrameWriter>(
Expand All @@ -454,19 +453,20 @@ std::optional<H4PacketWithH4> L2capCoc::GenerateNextTxPacket() {
PW_CHECK(first_kframe_writer.ok());
first_kframe_writer->sdu_length().Write(sdu_span.size());
PW_CHECK(first_kframe_writer->Ok());
payload_start = first_kframe_writer->payload().BackingStorage().data();
PW_CHECK(TryToCopyToEmbossStruct(
/*emboss_dest=*/first_kframe_writer->payload(),
/*src=*/sdu_span.subspan(tx_sdu_offset_, sdu_bytes_in_segment)));
} else {
Result<emboss::SubsequentKFrameWriter> subsequent_kframe_writer =
MakeEmbossWriter<emboss::SubsequentKFrameWriter>(
acl->payload().BackingStorage().data(),
acl->payload().SizeInBytes());
PW_CHECK(subsequent_kframe_writer.ok());
payload_start = subsequent_kframe_writer->payload().BackingStorage().data();
PW_CHECK(TryToCopyToEmbossStruct(
/*emboss_dest=*/subsequent_kframe_writer->payload(),
/*src=*/sdu_span.subspan(tx_sdu_offset_, sdu_bytes_in_segment)));
}

std::memcpy(/*__dest=*/payload_start,
/*__src=*/sdu_span.subspan(tx_sdu_offset_).data(),
/*__n=*/sdu_bytes_in_segment);
tx_sdu_offset_ += sdu_bytes_in_segment;

if (tx_sdu_offset_ == sdu_span.size()) {
Expand Down
6 changes: 3 additions & 3 deletions pw_bluetooth_proxy/l2cap_coc_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#include <cstdint>

#include "pw_assert/check.h" // IWYU pragma: keep
#include "pw_bluetooth_proxy/h4_packet.h"
#include "pw_bluetooth_proxy/l2cap_channel_common.h"
#include "pw_bluetooth_proxy_private/test_utils.h"
Expand Down Expand Up @@ -1977,9 +1978,8 @@ TEST_F(L2capCocReassemblyTest, OneSegmentRx) {
bframe->acl.writer.payload().BackingStorage().data(),
bframe->acl.writer.payload().SizeInBytes());
kframe.sdu_length().Write(capture.expected_payload.size());
std::memcpy(/*__dest=*/kframe.payload().BackingStorage().data(),
/*__src=*/capture.expected_payload.data(),
/*__n=*/capture.expected_payload.size());
PW_CHECK(TryToCopyToEmbossStruct(/*emboss_dest=*/kframe.payload(),
/*src=*/capture.expected_payload));

proxy.HandleH4HciFromController(std::move(h4_packet));

Expand Down
6 changes: 3 additions & 3 deletions pw_bluetooth_proxy/rfcomm_channel.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,9 @@ pw::Status RfcommChannel::Write(pw::span<const uint8_t> payload) {
return Status::ResourceExhausted();
}
PW_CHECK(rfcomm.information().SizeInBytes() == payload.size());
std::memcpy(rfcomm.information().BackingStorage().data(),
payload.data(),
payload.size());
PW_CHECK(TryToCopyToEmbossStruct(
/*emboss_dest=*/rfcomm.information(),
/*src=*/payload));

// UIH frame type:
// FCS should be calculated over address and control fields.
Expand Down
5 changes: 2 additions & 3 deletions pw_bluetooth_proxy/rfcomm_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,8 @@ Status SendRfcommFromController(ProxyHost& proxy,
}

EXPECT_EQ(rfcomm.information().SizeInBytes(), payload.size());
std::memcpy(rfcomm.information().BackingStorage().data(),
payload.data(),
payload.size());
EXPECT_TRUE(TryToCopyToEmbossStruct(/*emboss_dest=*/rfcomm.information(),
/*src=*/payload));
rfcomm.fcs().Write(fcs);
auto hci_span = bframe.acl.hci_span();
H4PacketWithHci packet{emboss::H4PacketType::ACL_DATA, hci_span};
Expand Down
17 changes: 8 additions & 9 deletions pw_bluetooth_proxy/test_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ Result<KFrameWithStorage> SetupKFrame(uint16_t handle,
segment_pdu_length +
emboss::BasicL2capHeader::IntrinsicSizeInBytes()));

uint8_t* kframe_payload_start;
if (segment_no == 0) {
PW_TRY_ASSIGN(kframe.writer,
MakeEmbossWriter<emboss::FirstKFrameWriter>(
Expand All @@ -129,8 +128,10 @@ Result<KFrameWithStorage> SetupKFrame(uint16_t handle,
first_kframe_writer.channel_id().Write(channel_id);
first_kframe_writer.sdu_length().Write(payload.size());
EXPECT_TRUE(first_kframe_writer.Ok());
kframe_payload_start =
first_kframe_writer.payload().BackingStorage().data();
PW_CHECK(TryToCopyToEmbossStruct(
/*emboss_dest=*/first_kframe_writer.payload(),
/*src=*/payload.subspan(payload_offset,
segment_pdu_length - sdu_length_field_offset)));
} else {
PW_TRY_ASSIGN(kframe.writer,
MakeEmbossWriter<emboss::SubsequentKFrameWriter>(
Expand All @@ -141,14 +142,12 @@ Result<KFrameWithStorage> SetupKFrame(uint16_t handle,
subsequent_kframe_writer.pdu_length().Write(segment_pdu_length);
subsequent_kframe_writer.channel_id().Write(channel_id);
EXPECT_TRUE(subsequent_kframe_writer.Ok());
kframe_payload_start =
subsequent_kframe_writer.payload().BackingStorage().data();
PW_CHECK(TryToCopyToEmbossStruct(
/*emboss_dest=*/subsequent_kframe_writer.payload(),
/*src=*/payload.subspan(payload_offset,
segment_pdu_length - sdu_length_field_offset)));
}

std::memcpy(/*__dest=*/kframe_payload_start,
/*__src=*/payload.data() + payload_offset,
/*__n=*/segment_pdu_length - sdu_length_field_offset);

return kframe;
}

Expand Down

0 comments on commit 818f8c1

Please sign in to comment.