Address review comments

src/ble/BtpEngine.cpp

@@ -34,6 +34,7 @@
 #include <lib/support/BitFlags.h>
 #include <lib/support/BufferReader.h>
 #include <lib/support/CodeUtils.h>
+#include <lib/support/SafeInt.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <system/SystemPacketBuffer.h>
 
@@ -346,6 +347,7 @@ CHIP_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle &&
     if (rx_flags.Has(HeaderFlags::kEndMessage))
     {
         // Trim remainder, if any, of the received packet buffer based on sender-specified length of reassembled message.
+        VerifyOrExit(CanCastTo<uint16_t>(mRxBuf->DataLength()), err = CHIP_ERROR_MESSAGE_TOO_LONG);
         int padding = static_cast<uint16_t>(mRxBuf->DataLength()) - mRxLength;
 
         if (padding > 0)
@@ -426,8 +428,9 @@ bool BtpEngine::HandleCharacteristicSend(System::PacketBufferHandle data, bool s
             return false;
         }
 
-        mTxBuf    = std::move(data);
-        mTxState  = kState_InProgress;
+        mTxBuf   = std::move(data);
+        mTxState = kState_InProgress;
+        VerifyOrReturnError(CanCastTo<uint16_t>(mTxBuf->DataLength()), false);
         mTxLength = static_cast<uint16_t>(mTxBuf->DataLength());
 
         ChipLogDebugBtpEngine(Ble, ">>> CHIPoBle preparing to send whole message:");

src/ble/tests/TestBtpEngine.cpp

@@ -67,7 +67,7 @@ TEST_F(TestBtpEngine, HandleCharacteristicReceivedOnePacket)
     };
 
     auto packet0 = System::PacketBufferHandle::NewWithData(packetData0, sizeof(packetData0));
-    EXPECT_EQ(packet0->DataLength(), 5);
+    EXPECT_EQ(packet0->DataLength(), static_cast<size_t>(5));
 
     SequenceNumber_t receivedAck;
     bool didReceiveAck;
@@ -81,15 +81,15 @@ TEST_F(TestBtpEngine, HandleCharacteristicReceivedTwoPacket)
     constexpr uint8_t packetData1[] = { to_underlying(BtpEngine::HeaderFlags::kEndMessage), 0x02, 0xff };
 
     auto packet0 = System::PacketBufferHandle::NewWithData(packetData0, sizeof(packetData0));
-    EXPECT_EQ(packet0->DataLength(), 5);
+    EXPECT_EQ(packet0->DataLength(), static_cast<size_t>(5));
 
     SequenceNumber_t receivedAck;
     bool didReceiveAck;
     EXPECT_EQ(mBtpEngine.HandleCharacteristicReceived(std::move(packet0), receivedAck, didReceiveAck), CHIP_NO_ERROR);
     EXPECT_EQ(mBtpEngine.RxState(), BtpEngine::kState_InProgress);
 
     auto packet1 = System::PacketBufferHandle::NewWithData(packetData1, sizeof(packetData1));
-    EXPECT_EQ(packet1->DataLength(), 3);
+    EXPECT_EQ(packet1->DataLength(), static_cast<size_t>(3));
 
     EXPECT_EQ(mBtpEngine.HandleCharacteristicReceived(std::move(packet1), receivedAck, didReceiveAck), CHIP_NO_ERROR);
     EXPECT_EQ(mBtpEngine.RxState(), BtpEngine::kState_Complete);
@@ -102,21 +102,21 @@ TEST_F(TestBtpEngine, HandleCharacteristicReceivedThreePacket)
     constexpr uint8_t packetData2[] = { to_underlying(BtpEngine::HeaderFlags::kEndMessage), 0x03, 0xff };
 
     auto packet0 = System::PacketBufferHandle::NewWithData(packetData0, sizeof(packetData0));
-    EXPECT_EQ(packet0->DataLength(), 5);
+    EXPECT_EQ(packet0->DataLength(), static_cast<size_t>(5));
 
     SequenceNumber_t receivedAck;
     bool didReceiveAck;
     EXPECT_EQ(mBtpEngine.HandleCharacteristicReceived(std::move(packet0), receivedAck, didReceiveAck), CHIP_NO_ERROR);
     EXPECT_EQ(mBtpEngine.RxState(), BtpEngine::kState_InProgress);
 
     auto packet1 = System::PacketBufferHandle::NewWithData(packetData1, sizeof(packetData1));
-    EXPECT_EQ(packet1->DataLength(), 3);
+    EXPECT_EQ(packet1->DataLength(), static_cast<size_t>(3));
 
     EXPECT_EQ(mBtpEngine.HandleCharacteristicReceived(std::move(packet1), receivedAck, didReceiveAck), CHIP_NO_ERROR);
     EXPECT_EQ(mBtpEngine.RxState(), BtpEngine::kState_InProgress);
 
     auto packet2 = System::PacketBufferHandle::NewWithData(packetData2, sizeof(packetData2));
-    EXPECT_EQ(packet2->DataLength(), 3);
+    EXPECT_EQ(packet2->DataLength(), static_cast<size_t>(3));
 
     EXPECT_EQ(mBtpEngine.HandleCharacteristicReceived(std::move(packet2), receivedAck, didReceiveAck), CHIP_NO_ERROR);
     EXPECT_EQ(mBtpEngine.RxState(), BtpEngine::kState_Complete);
@@ -133,7 +133,7 @@ TEST_F(TestBtpEngine, HandleCharacteristicSendOnePacket)
 
     EXPECT_TRUE(mBtpEngine.HandleCharacteristicSend(packet0.Retain(), false));
     EXPECT_EQ(mBtpEngine.TxState(), BtpEngine::kState_Complete);
-    EXPECT_EQ(packet0->DataLength(), 5);
+    EXPECT_EQ(packet0->DataLength(), static_cast<size_t>(5));
 }
 
 TEST_F(TestBtpEngine, HandleCharacteristicSendTwoPacket)
@@ -147,11 +147,11 @@ TEST_F(TestBtpEngine, HandleCharacteristicSendTwoPacket)
 
     EXPECT_TRUE(mBtpEngine.HandleCharacteristicSend(packet0.Retain(), false));
     EXPECT_EQ(mBtpEngine.TxState(), BtpEngine::kState_InProgress);
-    EXPECT_EQ(packet0->DataLength(), 20);
+    EXPECT_EQ(packet0->DataLength(), static_cast<size_t>(20));
 
     EXPECT_TRUE(mBtpEngine.HandleCharacteristicSend(nullptr, false));
     EXPECT_EQ(mBtpEngine.TxState(), BtpEngine::kState_Complete);
-    EXPECT_EQ(packet0->DataLength(), 16);
+    EXPECT_EQ(packet0->DataLength(), static_cast<size_t>(16));
 }
 
 // Send 40-byte payload.
@@ -169,15 +169,15 @@ TEST_F(TestBtpEngine, HandleCharacteristicSendThreePacket)
 
     EXPECT_TRUE(mBtpEngine.HandleCharacteristicSend(packet0.Retain(), false));
     EXPECT_EQ(mBtpEngine.TxState(), BtpEngine::kState_InProgress);
-    EXPECT_EQ(packet0->DataLength(), 20);
+    EXPECT_EQ(packet0->DataLength(), static_cast<size_t>(20));
 
     EXPECT_TRUE(mBtpEngine.HandleCharacteristicSend(nullptr, false));
     EXPECT_EQ(mBtpEngine.TxState(), BtpEngine::kState_InProgress);
-    EXPECT_EQ(packet0->DataLength(), 20);
+    EXPECT_EQ(packet0->DataLength(), static_cast<size_t>(20));
 
     EXPECT_TRUE(mBtpEngine.HandleCharacteristicSend(nullptr, false));
     EXPECT_EQ(mBtpEngine.TxState(), BtpEngine::kState_Complete);
-    EXPECT_EQ(packet0->DataLength(), 8);
+    EXPECT_EQ(packet0->DataLength(), static_cast<size_t>(8));
 }
 
 } // namespace

src/inet/TCPEndPointImplLwIP.cpp

@@ -350,7 +350,7 @@ CHIP_ERROR TCPEndPointImplLwIP::DriveSendingImpl()
             do
             {
                 VerifyOrDie(!startOfUnsent.buffer.IsNull());
-
+                VerifyOrDie(CanCastTo<uint16_t>(startOfUnsent.buffer->DataLength()));
                 uint16_t bufDataLen = static_cast<uint16_t>(startOfUnsent.buffer->DataLength());
 
                 // Get a pointer to the start of unsent data within the first buffer on the unsent queue.
@@ -508,7 +508,7 @@ CHIP_ERROR TCPEndPointImplLwIP::AckReceive(size_t len)
     VerifyOrReturnError(IsConnected(), CHIP_ERROR_INCORRECT_STATE);
     CHIP_ERROR res = CHIP_NO_ERROR;
 
-    VerifyOrReturnError(len < UINT16_MAX, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(CanCastTo<uint16_t>(len), CHIP_ERROR_INVALID_ARGUMENT);
 
     // Lock LwIP stack
     LOCK_TCPIP_CORE();
@@ -572,6 +572,7 @@ TCPEndPointImplLwIP::BufferOffset TCPEndPointImplLwIP::FindStartOfUnsent()
     while (leftToSkip > 0)
     {
         VerifyOrDie(!startOfUnsent.buffer.IsNull());
+        VerifyOrDie(CanCastTo<uint16_t>(startOfUnsent.buffer->DataLength()));
         uint16_t bufDataLen = static_cast<uint16_t>(startOfUnsent.buffer->DataLength());
         if (leftToSkip >= bufDataLen)
         {

src/inet/TCPEndPointImplSockets.cpp

@@ -483,7 +483,7 @@ CHIP_ERROR TCPEndPointImplSockets::DriveSendingImpl()
 
     while (!mSendQueue.IsNull())
     {
-        uint32_t bufLen = static_cast<uint32_t>(mSendQueue->DataLength());
+        size_t bufLen = mSendQueue->DataLength();
 
         ssize_t lenSentRaw = send(mSocket, mSendQueue->Start(), bufLen, sendFlags);
 
@@ -496,14 +496,13 @@ CHIP_ERROR TCPEndPointImplSockets::DriveSendingImpl()
             break;
         }
 
-        if (lenSentRaw < 0 || bufLen < static_cast<uint32_t>(lenSentRaw))
+        if (lenSentRaw < 0 || bufLen < static_cast<size_t>(lenSentRaw))
         {
             err = CHIP_ERROR_INCORRECT_STATE;
             break;
         }
 
-        // Cast is safe because bufLen is uint32_t.
-        uint32_t lenSent = static_cast<uint32_t>(lenSentRaw);
+        size_t lenSent = static_cast<size_t>(lenSentRaw);
 
         // Mark the connection as being active.
         MarkActive();

src/inet/UDPEndPointImplOpenThread.cpp

@@ -225,7 +225,7 @@ CHIP_ERROR UDPEndPointImplOT::SendMsgImpl(const IPPacketInfo * aPktInfo, System:
     otMessageInfo messageInfo;
 
     // For now the entire message must fit within a single buffer.
-    VerifyOrReturnError(!msg->HasChainedBuffer(), CHIP_ERROR_MESSAGE_TOO_LONG);
+    VerifyOrReturnError(!msg->HasChainedBuffer() && msg->DataLength() <= UINT16_MAX, CHIP_ERROR_MESSAGE_TOO_LONG);
 
     memset(&messageInfo, 0, sizeof(messageInfo));
 

src/inet/UDPEndPointImplSockets.cpp

@@ -610,7 +610,7 @@ void UDPEndPointImplSockets::HandlePendingIO(System::SocketEvents events)
 
         ssize_t rcvLen = recvmsg(mSocket, &msgHeader, MSG_DONTWAIT);
 
-        if (rcvLen < 0)
+        if (rcvLen == -1)
         {
             lStatus = CHIP_ERROR_POSIX(errno);
         }

src/inet/tests/TestInetLayerCommon.cpp

@@ -35,6 +35,7 @@
 
 #include <inet/IPPacketInfo.h>
 #include <lib/support/CodeUtils.h>
+#include <lib/support/SafeInt.h>
 
 #include "TestInetCommon.h"
 
@@ -259,6 +260,7 @@ static bool HandleDataReceived(const PacketBufferHandle & aBuffer, TransferStats
     // If we are accumulating stats by packet rather than by size,
     // then increment by one (1) rather than the total buffer length.
 
+    VerifyOrReturnError(CanCastTo<uint32_t>(lTotalDataLength), false);
     aStats.mReceive.mActual += ((aStatsByPacket) ? 1 : static_cast<uint32_t>(lTotalDataLength));
 
     return true;

src/lib/core/tests/TestTLV.cpp

@@ -269,12 +269,12 @@ void TestDupBytes(TLVReader & reader, Tag tag, const uint8_t * expectedVal, uint
     chip::Platform::MemoryFree(val);
 }
 
-void TestBufferContents(const System::PacketBufferHandle & buffer, const uint8_t * expectedVal, uint32_t expectedLen)
+void TestBufferContents(const System::PacketBufferHandle & buffer, const uint8_t * expectedVal, size_t expectedLen)
 {
     System::PacketBufferHandle buf = buffer.Retain();
     while (!buf.IsNull())
     {
-        uint32_t len = static_cast<uint32_t>(buf->DataLength());
+        size_t len = buf->DataLength();
         EXPECT_LE(len, expectedLen);
 
         EXPECT_EQ(memcmp(buf->Start(), expectedVal, len), 0);

src/messaging/tests/echo/echo_requester.cpp

@@ -183,7 +183,7 @@ void HandleEchoResponseReceived(chip::Messaging::ExchangeContext * ec, chip::Sys
 
     gEchoRespCount++;
 
-    printf("Echo Response: %" PRIu64 "/%" PRIu64 "(%.2f%%) len=%u time=%.3fs\n", gEchoRespCount, gEchoCount,
+    printf("Echo Response: %" PRIu64 "/%" PRIu64 "(%.2f%%) len=%" PRIu32 "time=%.3fs\n", gEchoRespCount, gEchoCount,
            static_cast<double>(gEchoRespCount) * 100 / static_cast<double>(gEchoCount),
            static_cast<uint32_t>(payload->DataLength()),
            static_cast<double>(chip::System::Clock::Milliseconds32(transitTime).count()) / 1000);

src/messaging/tests/echo/echo_responder.cpp

@@ -49,7 +49,7 @@ chip::SessionHolder gSession;
 // Callback handler when a CHIP EchoRequest is received.
 void HandleEchoRequestReceived(chip::Messaging::ExchangeContext * ec, chip::System::PacketBufferHandle && payload)
 {
-    printf("Echo Request, len=%u ... sending response.\n", static_cast<uint32_t>(payload->DataLength()));
+    printf("Echo Request, len=%" PRIu32 "... sending response.\n", static_cast<uint32_t>(payload->DataLength()));
 }
 
 } // namespace

src/platform/ESP32/nimble/BLEManagerImpl.cpp

@@ -34,6 +34,7 @@
 #include <ble/Ble.h>
 #endif
 #include <lib/support/CodeUtils.h>
+#include <lib/support/SafeInt.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <platform/CommissionableDataProvider.h>
 #include <platform/DeviceInstanceInfoProvider.h>
@@ -601,6 +602,10 @@ bool BLEManagerImpl::SendIndication(BLE_CONNECTION_OBJECT conId, const ChipBleUU
 
     ESP_LOGD(TAG, "Sending indication for CHIPoBLE TX characteristic (con %u, len %u)", conId, data->DataLength());
 
+    // For BLE, the buffer is capped at UINT16_MAX. Nevertheless, have a verify
+    // check before the cast to uint16_t.
+    VerifyOrExit(CanCastTo<uint16_t>(data->DataLength()), err = CHIP_ERROR_MESSAGE_TOO_LONG);
+
     om = ble_hs_mbuf_from_flat(data->Start(), static_cast<uint16_t>(data->DataLength()));
     if (om == NULL)
     {

src/platform/Zephyr/BLEManagerImpl.cpp

@@ -30,6 +30,7 @@
 #include <ble/Ble.h>
 #include <lib/support/CHIPMemString.h>
 #include <lib/support/CodeUtils.h>
+#include <lib/support/SafeInt.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <platform/DeviceInstanceInfoProvider.h>
 #include <platform/internal/BLEManager.h>
@@ -707,7 +708,8 @@ bool BLEManagerImpl::SendIndication(BLE_CONNECTION_OBJECT conId, const ChipBleUU
     params->attr = &sChipoBleAttributes[kCHIPoBLE_CCC_AttributeIndex];
     params->func = HandleTXIndicated;
     params->data = pBuf->Start();
-    params->len  = static_cast<uint16_t>(pBuf->DataLength());
+    VerifyOrExit(CanCastTo<uint16_t>(pBuf->DataLength()), err = CHIP_ERROR_MESSAGE_TOO_LONG);
+    params->len = static_cast<uint16_t>(pBuf->DataLength());
 
     status = bt_gatt_indicate(conId, params);
     VerifyOrExit(status == 0, err = MapErrorZephyr(status));
@@ -885,6 +887,8 @@ ssize_t BLEManagerImpl::HandleC3Read(struct bt_conn * conId, const struct bt_gat
         return 0;
     }
 
+    // For BLE, the max payload size is limited to UINT16_MAX since the length
+    // field is 2 bytes long. So, the cast to uint16_t should be fine.
     return bt_gatt_attr_read(conId, attr, buf, len, offset, sInstance.c3CharDataBufferHandle->Start(),
                              static_cast<uint16_t>(sInstance.c3CharDataBufferHandle->DataLength()));
 }

src/platform/android/BLEManagerImpl.cpp

@@ -384,6 +384,7 @@ bool BLEManagerImpl::SendWriteRequest(BLE_CONNECTION_OBJECT conId, const Ble::Ch
     err = JniReferences::GetInstance().N2J_ByteArray(env, static_cast<const uint8_t *>(charId->bytes), 16, charIdObj);
     SuccessOrExit(err);
 
+    VerifyOrExit(CanCastTo<uint16_t>(pBuf->DataLength()), err = CHIP_ERROR_MESSAGE_TOO_LONG);
     err = JniReferences::GetInstance().N2J_ByteArray(env, pBuf->Start(), static_cast<uint16_t>(pBuf->DataLength()),
                                                      characteristicDataObj);
     SuccessOrExit(err);

src/platform/bouffalolab/common/BLEManagerImpl.cpp

@@ -23,6 +23,7 @@
 
 #include <ble/Ble.h>
 #include <lib/support/CHIPMemString.h>
+#include <lib/support/SafeInt.h>
 #include <platform/DeviceInstanceInfoProvider.h>
 #include <platform/internal/BLEManager.h>
 #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING
@@ -669,7 +670,10 @@ bool BLEManagerImpl::SendIndication(BLE_CONNECTION_OBJECT conId, const ChipBleUU
     params->attr = &sChipoBleAttributes[kCHIPoBLE_CCC_AttributeIndex];
     params->func = HandleTXIndicated;
     params->data = pBuf->Start();
-    params->len  = static_cast<uint16_t>(pBuf->DataLength());
+    // For BLE, the buffer is capped at UINT16_MAX. Nevertheless, have a verify
+    // check before the cast to uint16_t.
+    VerifyOrExit(CanCastTo<uint16_t>(pBuf->DataLength()), err = CHIP_ERROR_MESSAGE_TOO_LONG);
+    params->len = static_cast<uint16_t>(pBuf->DataLength());
 
     status = bt_gatt_indicate(conId, params);
     VerifyOrExit(status == 0, err = MapErrorZephyr(status));
@@ -848,6 +852,8 @@ ssize_t BLEManagerImpl::HandleC3Read(struct bt_conn * conId, const struct bt_gat
         return 0;
     }
 
+    // For BLE, the max payload size is limited to UINT16_MAX since the length
+    // field is 2 bytes long. So, the cast to uint16_t should be fine.
     return bt_gatt_attr_read(conId, attr, buf, len, offset, sInstance.c3CharDataBufferHandle->Start(),
                              static_cast<uint16_t>(sInstance.c3CharDataBufferHandle->DataLength()));
 }

src/platform/mbed/BLEManagerImpl.cpp

@@ -32,6 +32,7 @@
 
 #include <ble/Ble.h>
 #include <lib/support/CodeUtils.h>
+#include <lib/support/SafeInt.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <platform/CommissionableDataProvider.h>
 #include <platform/internal/BLEManager.h>
@@ -984,6 +985,9 @@ bool BLEManagerImpl::SendIndication(BLE_CONNECTION_OBJECT conId, const ChipBleUU
     ble::GattServer & gatt_server = ble::BLE::Instance().gattServer();
     ble::attribute_handle_t att_handle;
 
+    // For BLE, the buffer is capped at UINT16_MAX.
+    VerifyOrExit(CanCastTo<uint16_t>(pBuf->DataLength()), err = CHIP_ERROR_MESSAGE_TOO_LONG);
+
     // No need to do anything fancy here. Only 3 handles are used in this impl.
     if (UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_TX))
     {

src/protocols/secure_channel/CASESession.cpp

@@ -1579,8 +1579,8 @@ CHIP_ERROR CASESession::HandleSigma3a(System::PacketBufferHandle && msg)
     TLV::TLVReader decryptedDataTlvReader;
     TLV::TLVType containerType = TLV::kTLVType_Structure;
 
-    const uint8_t * buf   = msg->Start();
-    const uint32_t bufLen = static_cast<uint32_t>(msg->DataLength());
+    const uint8_t * buf = msg->Start();
+    const size_t bufLen = msg->DataLength();
 
     constexpr size_t kCaseOverheadForFutureTbeData = 128;
 

src/protocols/user_directed_commissioning/UserDirectedCommissioningClient.cpp

@@ -255,7 +255,7 @@ void UserDirectedCommissioningClient::OnMessageReceived(const Transport::PeerAdd
     PayloadHeader payloadHeader;
     ReturnOnFailure(payloadHeader.DecodeAndConsume(msg));
 
-    ChipLogProgress(AppServer, "CommissionerDeclaration DataLength()=%u", static_cast<uint16_t>(msg->DataLength()));
+    ChipLogProgress(AppServer, "CommissionerDeclaration DataLength()=%" PRIu32, static_cast<uint32_t>(msg->DataLength()));
 
     uint8_t udcPayload[IdentificationDeclaration::kUdcTLVDataMaxBytes];
     size_t udcPayloadLength = std::min<size_t>(msg->DataLength(), sizeof(udcPayload));

src/protocols/user_directed_commissioning/UserDirectedCommissioningServer.cpp

@@ -52,7 +52,7 @@ void UserDirectedCommissioningServer::OnMessageReceived(const Transport::PeerAdd
     PayloadHeader payloadHeader;
     ReturnOnFailure(payloadHeader.DecodeAndConsume(msg));
 
-    ChipLogProgress(AppServer, "IdentityDeclaration DataLength()=%u", static_cast<uint16_t>(msg->DataLength()));
+    ChipLogProgress(AppServer, "IdentityDeclaration DataLength()=%" PRIu32, static_cast<uint32_t>(msg->DataLength()));
 
     uint8_t udcPayload[IdentificationDeclaration::kUdcTLVDataMaxBytes];
     size_t udcPayloadLength = std::min<size_t>(msg->DataLength(), sizeof(udcPayload));