From 2e54b294c0f4a8303ef8e669c0f24ec78d4cd022 Mon Sep 17 00:00:00 2001 From: Junior Martinez Date: Mon, 13 Mar 2023 13:05:03 -0400 Subject: [PATCH 1/2] Prevent any chance of division by 0 in Colorcontol. Update some C casts to C++ --- .../color-control-server.cpp | 76 ++++++++++++++----- 1 file changed, 55 insertions(+), 21 deletions(-) diff --git a/src/app/clusters/color-control-server/color-control-server.cpp b/src/app/clusters/color-control-server/color-control-server.cpp index d862a11686a34d..723cfc74780150 100644 --- a/src/app/clusters/color-control-server/color-control-server.cpp +++ b/src/app/clusters/color-control-server/color-control-server.cpp @@ -364,9 +364,18 @@ bool ColorControlServer::computeNewColor16uValue(ColorControlServer::Color16uTra } else if (p->finalValue > p->initialValue) { - newValue32u = ((uint32_t)(p->finalValue - p->initialValue)); - newValue32u *= ((uint32_t)(p->stepsRemaining)); - newValue32u /= ((uint32_t)(p->stepsTotal)); + newValue32u = static_cast(p->finalValue - p->initialValue); + newValue32u *= static_cast(p->stepsRemaining); + + /* + stepsTotal should always be at least 1, + still, prevent division by 0 and skips a meaningless division by 1 + */ + if (p->stepsTotal > 1) + { + newValue32u /= static_cast(p->stepsTotal); + } + p->currentValue = static_cast(p->finalValue - static_cast(newValue32u)); if (static_cast(newValue32u) > p->finalValue || p->currentValue > p->highLimit) @@ -376,9 +385,18 @@ bool ColorControlServer::computeNewColor16uValue(ColorControlServer::Color16uTra } else { - newValue32u = ((uint32_t)(p->initialValue - p->finalValue)); - newValue32u *= ((uint32_t)(p->stepsRemaining)); - newValue32u /= ((uint32_t)(p->stepsTotal)); + newValue32u = static_cast(p->initialValue - p->finalValue); + newValue32u *= static_cast(p->stepsRemaining); + + /* + stepsTotal should always be at least 1, + still, prevent division by 0 and skips a meaningless division by 1 + */ + if (p->stepsTotal > 1) + { + newValue32u /= static_cast(p->stepsTotal); + } + p->currentValue = static_cast(p->finalValue + static_cast(newValue32u)); if (p->finalValue > UINT16_MAX - static_cast(newValue32u) || p->currentValue < p->lowLimit) @@ -689,7 +707,15 @@ bool ColorControlServer::computeNewHueValue(ColorControlServer::ColorHueTransiti newHue32 = static_cast(p->isEnhancedHue ? subtractEnhancedHue(p->finalEnhancedHue, p->initialEnhancedHue) : subtractHue(p->finalHue, p->initialHue)); newHue32 *= static_cast(p->stepsRemaining); - newHue32 /= static_cast(p->stepsTotal); + + /* + stepsTotal should always be at least 1, + still, prevent division by 0 and skips a meaningless division by 1 + */ + if (p->stepsTotal > 1) + { + newHue32 /= static_cast(p->stepsTotal); + } if (p->isEnhancedHue) { @@ -705,7 +731,15 @@ bool ColorControlServer::computeNewHueValue(ColorControlServer::ColorHueTransiti newHue32 = static_cast(p->isEnhancedHue ? subtractEnhancedHue(p->initialEnhancedHue, p->finalEnhancedHue) : subtractHue(p->initialHue, p->finalHue)); newHue32 *= static_cast(p->stepsRemaining); - newHue32 /= static_cast(p->stepsTotal); + + /* + stepsTotal should always be at least 1, + still, prevent division by 0 and skips a meaningless division by 1 + */ + if (p->stepsTotal > 1) + { + newHue32 /= static_cast(p->stepsTotal); + } if (p->isEnhancedHue) { @@ -1831,12 +1865,12 @@ bool ColorControlServer::moveColorCommand(app::CommandHandler * commandObj, cons if (rateX > 0) { colorXTransitionState->finalValue = MAX_CIE_XY_VALUE; - unsignedRate = (uint16_t) rateX; + unsignedRate = static_cast(rateX); } else { colorXTransitionState->finalValue = MIN_CIE_XY_VALUE; - unsignedRate = (uint16_t)(rateX * -1); + unsignedRate = static_cast(rateX * -1); } transitionTimeX = computeTransitionTimeFromStateAndRate(colorXTransitionState, unsignedRate); colorXTransitionState->stepsRemaining = transitionTimeX; @@ -1850,12 +1884,12 @@ bool ColorControlServer::moveColorCommand(app::CommandHandler * commandObj, cons if (rateY > 0) { colorYTransitionState->finalValue = MAX_CIE_XY_VALUE; - unsignedRate = (uint16_t) rateY; + unsignedRate = static_cast(rateY); } else { colorYTransitionState->finalValue = MIN_CIE_XY_VALUE; - unsignedRate = (uint16_t)(rateY * -1); + unsignedRate = static_cast(rateY * -1); } transitionTimeY = computeTransitionTimeFromStateAndRate(colorYTransitionState, unsignedRate); colorYTransitionState->stepsRemaining = transitionTimeY; @@ -2465,8 +2499,6 @@ void ColorControlServer::levelControlColorTempChangeCommand(EndpointId endpoint) if (colorMode == COLOR_MODE_TEMPERATURE) { - uint16_t tempCoupleMin = getTemperatureCoupleToLevelMin(endpoint); - app::DataModel::Nullable currentLevel; EmberAfStatus status = LevelControl::Attributes::CurrentLevel::Get(endpoint, currentLevel); @@ -2475,13 +2507,14 @@ void ColorControlServer::levelControlColorTempChangeCommand(EndpointId endpoint) currentLevel.SetNonNull((uint8_t) 0x7F); } - uint16_t tempPhysMax = MAX_TEMPERATURE_VALUE; - Attributes::ColorTempPhysicalMaxMireds::Get(endpoint, &tempPhysMax); + uint32_t tempCoupleMin = getTemperatureCoupleToLevelMin(endpoint); + uint32_t tempPhysMax = MAX_TEMPERATURE_VALUE; + Attributes::ColorTempPhysicalMaxMireds::Get(endpoint, reinterpret_cast(&tempPhysMax)); // Scale color temp setting between the coupling min and the physical max. // Note that mireds varies inversely with level: low level -> high mireds. // Peg min/MAX level to MAX/min mireds, otherwise interpolate. - uint16_t newColorTemp; + uint32_t newColorTemp; if (currentLevel.Value() <= MIN_CURRENT_LEVEL) { newColorTemp = tempPhysMax; @@ -2492,13 +2525,14 @@ void ColorControlServer::levelControlColorTempChangeCommand(EndpointId endpoint) } else { - uint32_t tempDelta = (((uint32_t) tempPhysMax - (uint32_t) tempCoupleMin) * currentLevel.Value()) / - (uint32_t)(MAX_CURRENT_LEVEL - MIN_CURRENT_LEVEL + 1); - newColorTemp = (uint16_t)((uint32_t) tempPhysMax - tempDelta); + uint32_t tempDelta = + ((tempPhysMax - tempCoupleMin) * currentLevel.Value()) / (MAX_CURRENT_LEVEL - MIN_CURRENT_LEVEL + 1); + + newColorTemp = tempPhysMax - tempDelta; } // Apply new color temp. - moveToColorTemp(endpoint, newColorTemp, 0); + moveToColorTemp(endpoint, static_cast(newColorTemp), 0); } } From ec3f4a2ebf2c9854455b08315e5b102f04779142 Mon Sep 17 00:00:00 2001 From: Junior Martinez Date: Mon, 13 Mar 2023 16:35:13 -0400 Subject: [PATCH 2/2] Address comments --- .../color-control-server/color-control-server.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/app/clusters/color-control-server/color-control-server.cpp b/src/app/clusters/color-control-server/color-control-server.cpp index 723cfc74780150..17ddae6413abef 100644 --- a/src/app/clusters/color-control-server/color-control-server.cpp +++ b/src/app/clusters/color-control-server/color-control-server.cpp @@ -2507,14 +2507,14 @@ void ColorControlServer::levelControlColorTempChangeCommand(EndpointId endpoint) currentLevel.SetNonNull((uint8_t) 0x7F); } - uint32_t tempCoupleMin = getTemperatureCoupleToLevelMin(endpoint); - uint32_t tempPhysMax = MAX_TEMPERATURE_VALUE; - Attributes::ColorTempPhysicalMaxMireds::Get(endpoint, reinterpret_cast(&tempPhysMax)); + uint16_t tempCoupleMin = getTemperatureCoupleToLevelMin(endpoint); + uint16_t tempPhysMax = MAX_TEMPERATURE_VALUE; + Attributes::ColorTempPhysicalMaxMireds::Get(endpoint, &tempPhysMax); // Scale color temp setting between the coupling min and the physical max. // Note that mireds varies inversely with level: low level -> high mireds. // Peg min/MAX level to MAX/min mireds, otherwise interpolate. - uint32_t newColorTemp; + uint16_t newColorTemp; if (currentLevel.Value() <= MIN_CURRENT_LEVEL) { newColorTemp = tempPhysMax; @@ -2525,14 +2525,15 @@ void ColorControlServer::levelControlColorTempChangeCommand(EndpointId endpoint) } else { + uint32_t u32TempPhysMax = static_cast(tempPhysMax); // use a u32 to prevent overflows in next steps. uint32_t tempDelta = - ((tempPhysMax - tempCoupleMin) * currentLevel.Value()) / (MAX_CURRENT_LEVEL - MIN_CURRENT_LEVEL + 1); + ((u32TempPhysMax - tempCoupleMin) * currentLevel.Value()) / (MAX_CURRENT_LEVEL - MIN_CURRENT_LEVEL + 1); - newColorTemp = tempPhysMax - tempDelta; + newColorTemp = static_cast(tempPhysMax - tempDelta); } // Apply new color temp. - moveToColorTemp(endpoint, static_cast(newColorTemp), 0); + moveToColorTemp(endpoint, newColorTemp, 0); } }