Make src/setup_payload compile with -Werror=conversion

src/setup_payload/BUILD.gn

@@ -38,6 +38,8 @@ static_library("setup_payload") {
     "SetupPayloadHelper.h",
   ]
 
+  cflags = [ "-Wconversion" ]
+
   public_deps = [
     "${chip_root}/src/lib/core",
     "${chip_root}/src/lib/support",

src/setup_payload/Base41.cpp

@@ -29,6 +29,8 @@
 
 #include "Base41.h"
 
+#include <climits>
+
 using namespace std;
 
 namespace chip {
@@ -88,12 +90,13 @@ string base41Encode(const uint8_t * buf, size_t buf_len)
     // handle leftover bytes, if any
     if (buf_len != 0)
     {
-        int value = 0;
+        uint64_t value = 0;
+        static_assert(sizeof(value) * CHAR_BIT >= kBytesChunkLen * 8, "value might overflow");
 
-        for (int i = buf_len - 1; i >= 0; i--)
+        for (size_t i = buf_len; i > 0; i--)
         {
             value *= 256;
-            value += buf[i];
+            value += buf[i - 1];
         }
 
         // need to indicate there are leftover bytes, so append at least one encoding char
@@ -193,52 +196,54 @@ CHIP_ERROR base41Decode(string base41, vector<uint8_t> & result)
 {
     result.clear();
 
-    for (int i = 0; base41.length() - i >= kBase41ChunkLen; i += kBase41ChunkLen)
+    for (size_t i = 0; base41.length() - i >= kBase41ChunkLen; i += kBase41ChunkLen)
     {
         uint16_t value = 0;
 
-        for (int iv = i + (kBase41ChunkLen - 1); iv >= i; iv--)
+        for (size_t iv = i + kBase41ChunkLen; iv > i; iv--)
         {
             uint8_t v;
-            CHIP_ERROR err = decodeChar(base41[iv], v);
+            CHIP_ERROR err = decodeChar(base41[iv - 1], v);
 
             if (err != CHIP_NO_ERROR)
             {
                 return err;
             }
 
-            value *= kRadix;
-            value += v;
+            value = static_cast<uint16_t>(value * kRadix + v);
         }
 
-        result.push_back(value % 256);
-        result.push_back(value / 256);
+        result.push_back(static_cast<uint8_t>(value % 256));
+        // Cast is safe, because we divided a uint16_t by 256 to get here,
+        // so what's left has to fit inside uint8_t.
+        result.push_back(static_cast<uint8_t>(value / 256));
     }
 
     if (base41.length() % kBase41ChunkLen != 0) // only 1 or 2 chars left
     {
-        int tail       = (base41.length() % kBase41ChunkLen);
-        int i          = base41.length() - tail;
+        size_t tail    = (base41.length() % kBase41ChunkLen);
+        size_t i       = base41.length() - tail;
         uint16_t value = 0;
 
-        for (int iv = base41.length() - 1; iv >= i; iv--)
+        for (size_t iv = base41.length(); iv > i; iv--)
         {
             uint8_t v;
-            CHIP_ERROR err = decodeChar(base41[iv], v);
+            CHIP_ERROR err = decodeChar(base41[iv - 1], v);
 
             if (err != CHIP_NO_ERROR)
             {
                 return err;
             }
 
-            value *= kRadix;
-            value += v;
+            value = static_cast<uint16_t>(value * kRadix + v);
         }
-        result.push_back(value % 256);
+        result.push_back(static_cast<uint8_t>(value % 256));
         value /= 256;
         if (value != 0)
         {
-            result.push_back(value);
+            // Cast is safe, because we divided a uint16_t by 256 to get here,
+            // so what's left has to fit inside uint8_t.
+            result.push_back(static_cast<uint8_t>(value));
         }
     }
     return CHIP_NO_ERROR;

src/setup_payload/ManualSetupPayloadGenerator.cpp

@@ -24,6 +24,7 @@
 #include "ManualSetupPayloadGenerator.h"
 
 #include <inttypes.h>
+#include <limits>
 
 #include <support/logging/CHIPLogging.h>
 #include <support/verhoeff/Verhoeff.h>
@@ -32,11 +33,15 @@ using namespace chip;
 
 static uint32_t shortPayloadRepresentation(const SetupPayload & payload)
 {
-    int offset      = 1;
-    uint32_t result = payload.requiresCustomFlow ? 1 : 0;
-    result |= (payload.discriminator & kManualSetupDiscriminatorFieldBitMask) << offset;
-    offset += kManualSetupDiscriminatorFieldLengthInBits;
-    result |= payload.setUpPINCode << offset;
+    constexpr int discriminatorOffset = kCustomFlowRequiredFieldLengthInBits;
+    constexpr int pinCodeOffset       = discriminatorOffset + kManualSetupDiscriminatorFieldLengthInBits;
+    uint32_t result                   = payload.requiresCustomFlow ? 1 : 0;
+
+    static_assert(kManualSetupDiscriminatorFieldBitMask <= UINT32_MAX >> discriminatorOffset, "Discriminator won't fit");
+    result |= static_cast<uint32_t>((payload.discriminator & kManualSetupDiscriminatorFieldBitMask) << discriminatorOffset);
+
+    static_assert(pinCodeOffset + kSetupPINCodeFieldLengthInBits <= std::numeric_limits<uint32_t>::digits, "PIN code won't fit");
+    result |= static_cast<uint32_t>(payload.setUpPINCode << pinCodeOffset);
     return result;
 }
 

src/setup_payload/ManualSetupPayloadParser.cpp

@@ -23,6 +23,7 @@
 
 #include "ManualSetupPayloadParser.h"
 
+#include <support/SafeInt.h>
 #include <support/logging/CHIPLogging.h>
 #include <support/verhoeff/Verhoeff.h>
 
@@ -64,11 +65,11 @@ static CHIP_ERROR checkCodeLengthValidity(const string & decimalString, bool isL
 }
 
 // Extract n bits starting at index i and store it in dest
-static CHIP_ERROR extractBits(uint32_t number, uint64_t & dest, int index, int numberBits, int maxBits)
+static CHIP_ERROR extractBits(uint32_t number, uint64_t & dest, size_t index, size_t numberBits, size_t maxBits)
 {
     if ((index + numberBits) > maxBits)
     {
-        ChipLogError(SetupPayload, "Number %u maxBits %d index %d n %d", number, maxBits, index, numberBits);
+        ChipLogError(SetupPayload, "Number %lu maxBits %zu index %zu n %zu", number, maxBits, index, numberBits);
         return CHIP_ERROR_INVALID_STRING_LENGTH;
     }
     dest = (((1 << numberBits) - 1) & (number >> index));
@@ -86,14 +87,14 @@ static CHIP_ERROR toNumber(const string & decimalString, uint64_t & dest)
             return CHIP_ERROR_INVALID_INTEGER_VALUE;
         }
         number *= 10;
-        number += c - '0';
+        number += static_cast<uint64_t>(c - '0');
     }
     dest = number;
     return CHIP_NO_ERROR;
 }
 
 // Populate numberOfChars into dest from decimalString starting at startIndex (least significant digit = left-most digit)
-static CHIP_ERROR readDigitsFromDecimalString(const string & decimalString, int & index, uint64_t & dest,
+static CHIP_ERROR readDigitsFromDecimalString(const string & decimalString, size_t & index, uint64_t & dest,
                                               size_t numberOfCharsToRead)
 {
     if (decimalString.length() < numberOfCharsToRead || (numberOfCharsToRead + index > decimalString.length()))
@@ -102,19 +103,13 @@ static CHIP_ERROR readDigitsFromDecimalString(const string & decimalString, int
         return CHIP_ERROR_INVALID_STRING_LENGTH;
     }
 
-    if (index < 0)
-    {
-        ChipLogError(SetupPayload, "Failed decoding base10. Index was negative. %d", index);
-        return CHIP_ERROR_INVALID_ARGUMENT;
-    }
-
     string decimalSubstring = decimalString.substr(index, numberOfCharsToRead);
     index += numberOfCharsToRead;
     return toNumber(decimalSubstring, dest);
 }
 
 // Populate numberOfBits into dest from number starting at startIndex (LSB = right-most bit)
-static CHIP_ERROR readBitsFromNumber(int32_t number, int & index, uint64_t & dest, size_t numberOfBitsToRead, size_t maxBits)
+static CHIP_ERROR readBitsFromNumber(uint32_t number, size_t & index, uint64_t & dest, size_t numberOfBitsToRead, size_t maxBits)
 {
     uint64_t bits     = 0;
     CHIP_ERROR result = extractBits(number, bits, index, numberOfBitsToRead, maxBits);
@@ -140,34 +135,41 @@ CHIP_ERROR ManualSetupPayloadParser::populatePayload(SetupPayload & outPayload)
         return result;
     }
 
-    int stringOffset = 0;
+    size_t stringOffset = 0;
     uint64_t shortCode;
     result = readDigitsFromDecimalString(representationWithoutCheckDigit, stringOffset, shortCode, kManualSetupShortCodeCharLength);
     if (result != CHIP_NO_ERROR)
     {
         return result;
     }
 
+    if (!CanCastTo<uint32_t>(shortCode))
+    {
+        // Our attempts to extract discriminators and whatnot won't work right.
+        return CHIP_ERROR_INVALID_INTEGER_VALUE;
+    }
+
     bool isLongCode = (shortCode & 1) == 1;
     result          = checkCodeLengthValidity(representationWithoutCheckDigit, isLongCode);
     if (result != CHIP_NO_ERROR)
     {
         return result;
     }
 
-    int numberOffset = 1;
+    size_t numberOffset = 1;
     uint64_t setUpPINCode;
     size_t maxShortCodeBitsLength = 1 + kSetupPINCodeFieldLengthInBits + kManualSetupDiscriminatorFieldLengthInBits;
 
     uint64_t discriminator;
-    result = readBitsFromNumber(shortCode, numberOffset, discriminator, kManualSetupDiscriminatorFieldLengthInBits,
-                                maxShortCodeBitsLength);
+    result = readBitsFromNumber(static_cast<uint32_t>(shortCode), numberOffset, discriminator,
+                                kManualSetupDiscriminatorFieldLengthInBits, maxShortCodeBitsLength);
     if (result != CHIP_NO_ERROR)
     {
         return result;
     }
 
-    result = readBitsFromNumber(shortCode, numberOffset, setUpPINCode, kSetupPINCodeFieldLengthInBits, maxShortCodeBitsLength);
+    result = readBitsFromNumber(static_cast<uint32_t>(shortCode), numberOffset, setUpPINCode, kSetupPINCodeFieldLengthInBits,
+                                maxShortCodeBitsLength);
     if (result != CHIP_NO_ERROR)
     {
         return result;
@@ -195,12 +197,24 @@ CHIP_ERROR ManualSetupPayloadParser::populatePayload(SetupPayload & outPayload)
         {
             return result;
         }
-        outPayload.vendorID  = vendorID;
-        outPayload.productID = productID;
+        // Need to do dynamic checks, because we are reading 5 chars, so could
+        // have 99,999 here or something.
+        if (!CanCastTo<uint16_t>(vendorID))
+        {
+            return CHIP_ERROR_INVALID_INTEGER_VALUE;
+        }
+        outPayload.vendorID = static_cast<uint16_t>(vendorID);
+        if (!CanCastTo<uint16_t>(productID))
+        {
+            return CHIP_ERROR_INVALID_INTEGER_VALUE;
+        }
+        outPayload.productID = static_cast<uint16_t>(productID);
     }
     outPayload.requiresCustomFlow = isLongCode ? 1 : 0;
-    outPayload.setUpPINCode       = setUpPINCode;
-    outPayload.discriminator      = discriminator;
+    static_assert(kSetupPINCodeFieldLengthInBits <= 32, "Won't fit in uint32_t");
+    outPayload.setUpPINCode = static_cast<uint32_t>(setUpPINCode);
+    static_assert(kManualSetupDiscriminatorFieldLengthInBits <= 16, "Won't fit in uint16_t");
+    outPayload.discriminator = static_cast<uint16_t>(discriminator);
 
     return result;
 }

src/setup_payload/QRCodeSetupPayloadGenerator.cpp

@@ -42,10 +42,11 @@ using namespace std;
 using namespace chip::TLV;
 
 // Populates numberOfBits starting from LSB of input into bits, which is assumed to be zero-initialized
-static CHIP_ERROR populateBits(uint8_t * bits, int & offset, uint64_t input, size_t numberOfBits, size_t totalPayloadDataSizeInBits)
+static CHIP_ERROR populateBits(uint8_t * bits, size_t & offset, uint64_t input, size_t numberOfBits,
+                               size_t totalPayloadDataSizeInBits)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
-    int index;
+    size_t index;
 
     VerifyOrExit(offset + numberOfBits <= totalPayloadDataSizeInBits, err = CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrExit(input < 1u << numberOfBits, err = CHIP_ERROR_INVALID_ARGUMENT);
@@ -56,7 +57,7 @@ static CHIP_ERROR populateBits(uint8_t * bits, int & offset, uint64_t input, siz
     {
         if (input & 1)
         {
-            bits[index / 8] |= 1 << index % 8;
+            bits[index / 8] |= static_cast<uint8_t>(1 << index % 8);
         }
         index++;
         input >>= 1;
@@ -65,8 +66,8 @@ static CHIP_ERROR populateBits(uint8_t * bits, int & offset, uint64_t input, siz
     return err;
 }
 
-static CHIP_ERROR populateTLVBits(uint8_t * bits, int & offset, const uint8_t * tlvBuf, size_t tlvBufSizeInBytes,
-                                  int totalPayloadDataSizeInBits)
+static CHIP_ERROR populateTLVBits(uint8_t * bits, size_t & offset, const uint8_t * tlvBuf, size_t tlvBufSizeInBytes,
+                                  size_t totalPayloadDataSizeInBits)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     for (size_t i = 0; i < tlvBufSizeInBytes; i++)
@@ -130,7 +131,7 @@ CHIP_ERROR writeTag(TLVWriter & writer, uint64_t tag, OptionalQRCodeInfoExtensio
 }
 
 CHIP_ERROR QRCodeSetupPayloadGenerator::generateTLVFromOptionalData(SetupPayload & outPayload, uint8_t * tlvDataStart,
-                                                                    uint32_t maxLen, uint32_t & tlvDataLengthInBytes)
+                                                                    uint32_t maxLen, size_t & tlvDataLengthInBytes)
 {
     CHIP_ERROR err                                            = CHIP_NO_ERROR;
     vector<OptionalQRCodeInfo> optionalData                   = outPayload.getAllOptionalVendorData();
@@ -189,11 +190,11 @@ CHIP_ERROR QRCodeSetupPayloadGenerator::generateTLVFromOptionalData(SetupPayload
     return err;
 }
 
-static CHIP_ERROR generateBitSet(SetupPayload & payload, uint8_t * bits, uint8_t * tlvDataStart, uint32_t tlvDataLengthInBytes)
+static CHIP_ERROR generateBitSet(SetupPayload & payload, uint8_t * bits, uint8_t * tlvDataStart, size_t tlvDataLengthInBytes)
 {
-    CHIP_ERROR err             = CHIP_NO_ERROR;
-    int offset                 = 0;
-    int totalPayloadSizeInBits = kTotalPayloadDataSizeInBits + (tlvDataLengthInBytes * 8);
+    CHIP_ERROR err                = CHIP_NO_ERROR;
+    size_t offset                 = 0;
+    size_t totalPayloadSizeInBits = kTotalPayloadDataSizeInBits + (tlvDataLengthInBytes * 8);
     err = populateBits(bits, offset, payload.version, kVersionFieldLengthInBits, kTotalPayloadDataSizeInBits);
     err = populateBits(bits, offset, payload.vendorID, kVendorIDFieldLengthInBits, kTotalPayloadDataSizeInBits);
     err = populateBits(bits, offset, payload.productID, kProductIDFieldLengthInBits, kTotalPayloadDataSizeInBits);
@@ -235,10 +236,10 @@ CHIP_ERROR QRCodeSetupPayloadGenerator::payloadBase41Representation(string & bas
 }
 
 CHIP_ERROR QRCodeSetupPayloadGenerator::payloadBase41Representation(string & base41Representation, uint8_t * tlvDataStart,
-                                                                    size_t tlvDataStartSize)
+                                                                    uint32_t tlvDataStartSize)
 {
-    CHIP_ERROR err                = CHIP_NO_ERROR;
-    uint32_t tlvDataLengthInBytes = 0;
+    CHIP_ERROR err              = CHIP_NO_ERROR;
+    size_t tlvDataLengthInBytes = 0;
 
     VerifyOrExit(mPayload.isValidQRCodePayload(), err = CHIP_ERROR_INVALID_ARGUMENT);
     err = generateTLVFromOptionalData(mPayload, tlvDataStart, tlvDataStartSize, tlvDataLengthInBytes);

src/setup_payload/QRCodeSetupPayloadGenerator.h

@@ -80,11 +80,11 @@ class QRCodeSetupPayloadGenerator
      *               that an error occurred preventing the function from
      *               producing the requested string.
      */
-    CHIP_ERROR payloadBase41Representation(std::string & base41Representation, uint8_t * tlvDataStart, size_t tlvDataStartSize);
+    CHIP_ERROR payloadBase41Representation(std::string & base41Representation, uint8_t * tlvDataStart, uint32_t tlvDataStartSize);
 
 private:
     CHIP_ERROR generateTLVFromOptionalData(SetupPayload & outPayload, uint8_t * tlvDataStart, uint32_t maxLen,
-                                           uint32_t & tlvDataLengthInBytes);
+                                           size_t & tlvDataLengthInBytes);
 };
 
 } // namespace chip