From f047caff3b1651ed67cdebe3c48e7ff534bbf5f7 Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Wed, 6 Oct 2021 11:26:54 +0200 Subject: [PATCH] Addressed first part of review comments --- src/app/server/Mdns.cpp | 10 +++--- src/include/platform/ConfigurationManager.h | 10 ++++-- .../GenericConfigurationManagerImpl.cpp | 20 +++++++----- .../GenericConfigurationManagerImpl.h | 2 +- ...nericThreadStackManagerImpl_OpenThread.cpp | 1 + src/platform/OpenThread/MdnsImpl.cpp | 5 +-- src/platform/fake/ConfigurationManagerImpl.h | 2 +- src/platform/tests/TestConfigurationMgr.cpp | 31 +++++++++++++++++++ 8 files changed, 64 insertions(+), 17 deletions(-) diff --git a/src/app/server/Mdns.cpp b/src/app/server/Mdns.cpp index 3004436240132f..3aeb5b8c3efd7d 100644 --- a/src/app/server/Mdns.cpp +++ b/src/app/server/Mdns.cpp @@ -244,13 +244,14 @@ CHIP_ERROR MdnsServer::AdvertiseOperational() { if (fabricInfo.IsInitialized()) { - uint8_t mac[8]; + uint8_t macBuffer[DeviceLayer::ConfigurationManager::kMACAddressLength]; + MutableByteSpan mac(macBuffer); chip::DeviceLayer::ConfigurationMgr().GetPrimaryMACAddress(mac); const auto advertiseParameters = chip::Mdns::OperationalAdvertisingParameters() .SetPeerId(fabricInfo.GetPeerId()) - .SetMac(chip::ByteSpan(mac, 8)) + .SetMac(mac) .SetPort(GetSecuredPort()) .SetMRPRetryIntervals(Optional(CHIP_CONFIG_MRP_DEFAULT_INITIAL_RETRY_INTERVAL), Optional(CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL)) @@ -282,9 +283,10 @@ CHIP_ERROR MdnsServer::Advertise(bool commissionableNode, chip::Mdns::Commission char pairingInst[chip::Mdns::kKeyPairingInstructionMaxLength + 1]; - uint8_t mac[8]; + uint8_t macBuffer[DeviceLayer::ConfigurationManager::kMACAddressLength]; + MutableByteSpan mac(macBuffer); chip::DeviceLayer::ConfigurationMgr().GetPrimaryMACAddress(mac); - advertiseParameters.SetMac(chip::ByteSpan(mac, 8)); + advertiseParameters.SetMac(mac); uint16_t value; if (DeviceLayer::ConfigurationMgr().GetVendorId(value) != CHIP_NO_ERROR) diff --git a/src/include/platform/ConfigurationManager.h b/src/include/platform/ConfigurationManager.h index 2342fcb385daf3..5cad1192af6158 100644 --- a/src/include/platform/ConfigurationManager.h +++ b/src/include/platform/ConfigurationManager.h @@ -60,6 +60,12 @@ class ConfigurationManager kMaxPairingCodeLength = 16, kMaxSerialNumberLength = 32, kMaxFirmwareRevisionLength = 32, +#if CHIP_DEVICE_CONFIG_ENABLE_THREAD + kMACAddressLength = 8, +#else + kMACAddressLength = 6, +#endif + kMaxMACAddressLength = 8, }; CHIP_ERROR GetVendorName(char * buf, size_t bufSize); @@ -69,7 +75,7 @@ class ConfigurationManager CHIP_ERROR GetProductRevisionString(char * buf, size_t bufSize); CHIP_ERROR GetProductRevision(uint16_t & productRev); CHIP_ERROR GetSerialNumber(char * buf, size_t bufSize, size_t & serialNumLen); - CHIP_ERROR GetPrimaryMACAddress(uint8_t (&buf)[8]); + CHIP_ERROR GetPrimaryMACAddress(MutableByteSpan buf); CHIP_ERROR GetPrimaryWiFiMACAddress(uint8_t * buf); CHIP_ERROR GetPrimary802154MACAddress(uint8_t * buf); CHIP_ERROR GetManufacturingDate(uint16_t & year, uint8_t & month, uint8_t & dayOfMonth); @@ -266,7 +272,7 @@ inline CHIP_ERROR ConfigurationManager::GetSerialNumber(char * buf, size_t bufSi return static_cast(this)->_GetSerialNumber(buf, bufSize, serialNumLen); } -inline CHIP_ERROR ConfigurationManager::GetPrimaryMACAddress(uint8_t (&buf)[8]) +inline CHIP_ERROR ConfigurationManager::GetPrimaryMACAddress(MutableByteSpan buf) { return static_cast(this)->_GetPrimaryMACAddress(buf); } diff --git a/src/include/platform/internal/GenericConfigurationManagerImpl.cpp b/src/include/platform/internal/GenericConfigurationManagerImpl.cpp index 687f888977df00..d4785c3e5cbf5a 100644 --- a/src/include/platform/internal/GenericConfigurationManagerImpl.cpp +++ b/src/include/platform/internal/GenericConfigurationManagerImpl.cpp @@ -231,24 +231,30 @@ CHIP_ERROR GenericConfigurationManagerImpl::_StorePrimaryWiFiMACAddre } template -CHIP_ERROR GenericConfigurationManagerImpl::_GetPrimaryMACAddress(uint8_t (&buf)[8]) +CHIP_ERROR GenericConfigurationManagerImpl::_GetPrimaryMACAddress(MutableByteSpan buf) { - memset(buf, 0, 8); + if (buf.size() != ConfigurationManager::kMACAddressLength) + return CHIP_ERROR_INVALID_ARGUMENT; + + memset(buf.data(), 0, buf.size()); + #if CHIP_DEVICE_CONFIG_ENABLE_THREAD - if (chip::DeviceLayer::ThreadStackMgr().GetPrimary802154MACAddress(buf) == CHIP_NO_ERROR) + if (chip::DeviceLayer::ThreadStackMgr().GetPrimary802154MACAddress(buf.data()) == CHIP_NO_ERROR) { ChipLogDetail(DeviceLayer, "Using Thread extended MAC for hostname."); return CHIP_NO_ERROR; } -#endif - if (DeviceLayer::ConfigurationMgr().GetPrimaryWiFiMACAddress(buf) == CHIP_NO_ERROR) +#else + if (DeviceLayer::ConfigurationMgr().GetPrimaryWiFiMACAddress(buf.data()) == CHIP_NO_ERROR) { ChipLogDetail(DeviceLayer, "Using wifi MAC for hostname"); return CHIP_NO_ERROR; } +#endif + ChipLogError(DeviceLayer, "MAC is not known, using a default."); - uint8_t temp[6] = { 0xEE, 0xAA, 0xBA, 0xDA, 0xBA, 0xD0 }; - memcpy(buf, temp, 6); + uint8_t temp[ConfigurationManager::kMaxMACAddressLength] = { 0xEE, 0xAA, 0xBA, 0xDA, 0xBA, 0xD0, 0xDD, 0xCA }; + memcpy(buf.data(), temp, buf.size()); return CHIP_NO_ERROR; } diff --git a/src/include/platform/internal/GenericConfigurationManagerImpl.h b/src/include/platform/internal/GenericConfigurationManagerImpl.h index b0d6dc0c85e36e..e648a9d9229a7a 100644 --- a/src/include/platform/internal/GenericConfigurationManagerImpl.h +++ b/src/include/platform/internal/GenericConfigurationManagerImpl.h @@ -66,7 +66,7 @@ class GenericConfigurationManagerImpl uint8_t & second); CHIP_ERROR _GetSerialNumber(char * buf, size_t bufSize, size_t & serialNumLen); CHIP_ERROR _StoreSerialNumber(const char * serialNum, size_t serialNumLen); - CHIP_ERROR _GetPrimaryMACAddress(uint8_t (&buf)[8]); + CHIP_ERROR _GetPrimaryMACAddress(MutableByteSpan buf); CHIP_ERROR _GetPrimaryWiFiMACAddress(uint8_t * buf); CHIP_ERROR _StorePrimaryWiFiMACAddress(const uint8_t * buf); CHIP_ERROR _GetPrimary802154MACAddress(uint8_t * buf); diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp index 4a71261f04b434..9b64aa7fabe524 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp @@ -1819,6 +1819,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_ClearSrpHost(co Impl()->LockThreadStack(); VerifyOrExit(aHostName, error = CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrExit(strlen(aHostName) <= SrpClient::kMaxHostNameSize, error = CHIP_ERROR_INVALID_STRING_LENGTH); VerifyOrExit(mSrpClient.mInitializedCallback, error = CHIP_ERROR_INCORRECT_STATE); // Add host and remove it with notifying SRP server to clean old information related to the host. diff --git a/src/platform/OpenThread/MdnsImpl.cpp b/src/platform/OpenThread/MdnsImpl.cpp index 2cf8ab3d2d94a5..0bd2ff2212a2b2 100644 --- a/src/platform/OpenThread/MdnsImpl.cpp +++ b/src/platform/OpenThread/MdnsImpl.cpp @@ -30,10 +30,11 @@ CHIP_ERROR ChipMdnsInit(MdnsAsyncReturnCallback initCallback, MdnsAsyncReturnCal { ReturnErrorOnFailure(ThreadStackMgr().SetSrpDnsCallbacks(initCallback, errorCallback, context)); - uint8_t mac[8]; + uint8_t macBuffer[ConfigurationManager::kMACAddressLength]; + MutableByteSpan mac(macBuffer); char hostname[kMdnsHostNameMaxSize + 1] = ""; ReturnErrorOnFailure(DeviceLayer::ConfigurationMgr().GetPrimaryMACAddress(mac)); - MakeHostName(hostname, sizeof(hostname), chip::ByteSpan(mac, 8)); + MakeHostName(hostname, sizeof(hostname), mac); return ThreadStackMgr().ClearSrpHost(hostname); } diff --git a/src/platform/fake/ConfigurationManagerImpl.h b/src/platform/fake/ConfigurationManagerImpl.h index 5f7024a63328db..7b1b7a4e8dad86 100644 --- a/src/platform/fake/ConfigurationManagerImpl.h +++ b/src/platform/fake/ConfigurationManagerImpl.h @@ -49,7 +49,7 @@ class ConfigurationManagerImpl final : public ConfigurationManager } CHIP_ERROR _GetSerialNumber(char * buf, size_t bufSize, size_t & serialNumLen) { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR _StoreSerialNumber(const char * serialNum, size_t serialNumLen) { return CHIP_ERROR_NOT_IMPLEMENTED; } - CHIP_ERROR _GetPrimaryMACAddress(uint8_t (&buf)[8]) { return CHIP_ERROR_NOT_IMPLEMENTED; } + CHIP_ERROR _GetPrimaryMACAddress(MutableByteSpan buf) { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR _GetPrimaryWiFiMACAddress(uint8_t * buf) { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR _StorePrimaryWiFiMACAddress(const uint8_t * buf) { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR _GetPrimary802154MACAddress(uint8_t * buf) { return CHIP_ERROR_NOT_IMPLEMENTED; } diff --git a/src/platform/tests/TestConfigurationMgr.cpp b/src/platform/tests/TestConfigurationMgr.cpp index d2d325638400df..16a6ae1e864cee 100644 --- a/src/platform/tests/TestConfigurationMgr.cpp +++ b/src/platform/tests/TestConfigurationMgr.cpp @@ -441,6 +441,36 @@ static void TestConfigurationMgr_Breadcrumb(nlTestSuite * inSuite, void * inCont NL_TEST_ASSERT(inSuite, breadcrumb == 12345); } +static void TestConfigurationMgr_GetPrimaryMACAddress(nlTestSuite * inSuite, void * inContext) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + const uint8_t defaultMacAddress[8] = {0xEE, 0xAA, 0xBA, 0xDA, 0xBA, 0xD0, 0xDD, 0xCA }; + uint8_t macBuffer8Bytes[8]; + uint8_t macBuffer6Bytes[6]; + MutableByteSpan mac8Bytes(macBuffer8Bytes); + MutableByteSpan mac6Bytes(macBuffer6Bytes); + + err = ConfigurationMgr().GetPrimaryMACAddress(mac8Bytes); + if (mac8Bytes.size() != ConfigurationManager::kMACAddressLength) { + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); + } else { + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + // Verify default MAC address value + NL_TEST_ASSERT(inSuite, strncmp(reinterpret_cast(mac8Bytes.data()), reinterpret_cast(defaultMacAddress), mac8Bytes.size()) == 0); + } + + err = ConfigurationMgr().GetPrimaryMACAddress(mac6Bytes); + if (mac6Bytes.size() != ConfigurationManager::kMACAddressLength) { + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); + } else { + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + // Verify default MAC address value + NL_TEST_ASSERT(inSuite, strncmp(reinterpret_cast(mac6Bytes.data()), reinterpret_cast(defaultMacAddress), mac6Bytes.size()) == 0); + } +} + /** * Test Suite. It lists all the test functions. */ @@ -467,6 +497,7 @@ static const nlTest sTests[] = { NL_TEST_DEF("Test ConfigurationMgr::RegulatoryLocation", TestConfigurationMgr_RegulatoryLocation), NL_TEST_DEF("Test ConfigurationMgr::CountryCode", TestConfigurationMgr_CountryCode), NL_TEST_DEF("Test ConfigurationMgr::Breadcrumb", TestConfigurationMgr_Breadcrumb), + NL_TEST_DEF("Test ConfigurationMgr::GetPrimaryMACAddress", TestConfigurationMgr_GetPrimaryMACAddress), NL_TEST_SENTINEL() };