Remove use of sprintf() (unsafe API)

.gitignore

@@ -1,3 +1,6 @@
+/debug/
+/firmware/
+/ESPixelStick/ESPixelStick.ino.cpp
 /.doxygen/doc/
 /.pio/
 /.vscode/

ESPixelStick/src/ESPixelStick.h

@@ -112,6 +112,8 @@ bool setFromJSON (T& OutValue, J& Json, N Name)
     return HasBeenModified;
 };
 
+
+
 #define logcon(msg) \
 { \
     String DN; \

ESPixelStick/src/input/InputAlexa.cpp

@@ -21,6 +21,7 @@
 #include <Int64String.h>
 #include "InputAlexa.h"
 #include "InputEffectEngine.hpp"
+#include "../utility/SaferStringConversion.hpp"
 
 #if defined ARDUINO_ARCH_ESP32
 #   include <functional>
@@ -162,8 +163,9 @@ void c_InputAlexa::onMessage(EspalexaDevice * pDevice)
     {
         OutputMgr.ClearBuffer ();
 
-        char HexColor[] = "#000000 ";
-        sprintf (HexColor, "#%02x%02x%02x", pDevice->getR (), pDevice->getG (), pDevice->getB ());
+        char HexColor[8];
+        ESP_ERROR_CHECK(saferRgbToHtmlColorString(HexColor, pDevice->getR (), pDevice->getG (), pDevice->getB ()));
+
         // DEBUG_V (String ("pDevice->getR: ") + String (pDevice->getR ()));
         // DEBUG_V (String ("pDevice->getG: ") + String (pDevice->getG ()));
         // DEBUG_V (String ("pDevice->getB: ") + String (pDevice->getB ()));

ESPixelStick/src/input/InputEffectEngine.cpp

@@ -17,6 +17,7 @@
 *
 */
 #include "../ESPixelStick.h"
+#include "../utility/SaferStringConversion.hpp"
 #include "InputEffectEngine.hpp"
 
 //-----------------------------------------------------------------------------
@@ -98,8 +99,8 @@ void c_InputEffectEngine::Begin ()
 void c_InputEffectEngine::GetConfig (JsonObject& jsonConfig)
 {
     // DEBUG_START;
-    char HexColor[] = "#000000 ";
-    sprintf (HexColor, "#%02x%02x%02x", EffectColor.r, EffectColor.g, EffectColor.b);
+    char HexColor[8];
+    ESP_ERROR_CHECK(saferRgbToHtmlColorString(HexColor, EffectColor.r, EffectColor.g, EffectColor.b));
     // DEBUG_V ("");
 
     jsonConfig[CN_currenteffect]      = ActiveEffect->name;

ESPixelStick/src/input/InputFPPRemotePlayEffect.hpp

@@ -47,7 +47,7 @@ class c_InputFPPRemotePlayEffect : public c_InputFPPRemotePlayItem
     fsm_PlayEffect_state_PlayingEffect fsm_PlayEffect_state_PlayingEffect_imp;
 
     fsm_PlayEffect_state* pCurrentFsmState = nullptr;
-    time_t PLayEffectEndTime = 0;
+    time_t PlayEffectEndTime = 0;
 
     c_InputEffectEngine EffectsEngine;
 

ESPixelStick/src/input/InputFPPRemotePlayEffectFsm.cpp

@@ -20,6 +20,7 @@
 
 #include "InputFPPRemotePlayEffect.hpp"
 #include "../service/FPPDiscovery.h"
+#include "../utility/SaferStringConversion.hpp"
 
 //-----------------------------------------------------------------------------
 void fsm_PlayEffect_state_Idle::Poll ()
@@ -50,7 +51,7 @@ void fsm_PlayEffect_state_Idle::Start (String & ConfigString, float )
     // DEBUG_START;
 
     // DEBUG_V (String ("ConfigString: '") + ConfigString + "'");
-    p_InputFPPRemotePlayEffect->PLayEffectEndTime = millis () + (1000 * p_InputFPPRemotePlayEffect->PlayDurationSec);
+    p_InputFPPRemotePlayEffect->PlayEffectEndTime = millis () + (1000 * p_InputFPPRemotePlayEffect->PlayDurationSec);
 
     // tell the effect engine what it is supposed to be doing
     DynamicJsonDocument EffectConfig (512);
@@ -123,7 +124,7 @@ void fsm_PlayEffect_state_PlayingEffect::Poll ()
     p_InputFPPRemotePlayEffect->EffectsEngine.SetBufferInfo (OutputMgr.GetBufferUsedSize());
     p_InputFPPRemotePlayEffect->EffectsEngine.Process ();
 
-    if (p_InputFPPRemotePlayEffect->PLayEffectEndTime <= millis ())
+    if (p_InputFPPRemotePlayEffect->PlayEffectEndTime <= millis ())
     {
         // DEBUG_V ("");
         Stop ();
@@ -192,17 +193,14 @@ void fsm_PlayEffect_state_PlayingEffect::GetStatus (JsonObject& jsonStatus)
     // DEBUG_START;
 
     time_t now = millis ();
-    time_t SecondsRemaining = (p_InputFPPRemotePlayEffect->PLayEffectEndTime - now) / 1000;
-    if (now > p_InputFPPRemotePlayEffect->PLayEffectEndTime)
+    time_t SecondsRemaining = (p_InputFPPRemotePlayEffect->PlayEffectEndTime - now) / 1000;
+    if (now > p_InputFPPRemotePlayEffect->PlayEffectEndTime)
     {
         SecondsRemaining = 0;
     }
 
-    time_t MinutesRemaining = min (time_t (999), time_t (SecondsRemaining / 60));
-    SecondsRemaining = SecondsRemaining % 60;
-
-    char buf[10];
-    sprintf (buf, "%02u:%02u", uint32_t(MinutesRemaining), uint32_t(SecondsRemaining));
+    char buf[12];
+    ESP_ERROR_CHECK(saferSecondsToFormattedMinutesAndSecondsString(buf, (uint32_t)SecondsRemaining));
     jsonStatus[CN_TimeRemaining] = buf;
 
     p_InputFPPRemotePlayEffect->EffectsEngine.GetStatus (jsonStatus);

ESPixelStick/src/input/InputFPPRemotePlayFile.cpp

@@ -21,6 +21,7 @@
 #include "InputFPPRemotePlayFile.hpp"
 #include "../service/FPPDiscovery.h"
 #include "../service/fseq.h"
+#include "../utility/SaferStringConversion.hpp"
 
 //-----------------------------------------------------------------------------
 static void TimerPollHandler (void * p)
@@ -177,12 +178,26 @@ void c_InputFPPRemotePlayFile::GetStatus (JsonObject& JsonStatus)
 {
     // xDEBUG_START;
 
+    // NOTE:
+    // variables ending in "Total" reflect total     play time
+    // variables ending in "Rem"   reflect remaining play time
+
+    // File uses milliseconds to store elapsed and total play time
     uint32_t mseconds = FrameControl.ElapsedPlayTimeMS;
-    // BUGBUG -- can overflow, resulting in incorrect time calculation (uint32 * uint32 ==> requires uint64 result)
-    uint32_t msecondsTotal = FrameControl.FrameStepTimeMS * FrameControl.TotalNumberOfFramesInSequence;
+    uint32_t msecondsTotal;
+    if (__builtin_mul_overflow(FrameControl.FrameStepTimeMS, FrameControl.TotalNumberOfFramesInSequence, &msecondsTotal)) {
+        // returned non-zero: there has been an overflow
+        msecondsTotal = MilliSecondsInASecond; // set to one second total when overflow occurs
+    }
 
-    uint32_t secs = mseconds / 1000;
-    uint32_t secsTot = msecondsTotal / 1000;
+    // JsonStatus uses seconds to report elapsed, played, and remaining time
+    uint32_t secs = mseconds / MilliSecondsInASecond;
+    uint32_t secsTot = msecondsTotal / MilliSecondsInASecond;
+    uint32_t secsRem;
+    if (__builtin_sub_overflow(secsTot, secs, &secsRem)) {
+        // returned non-zero: there has been an overflow
+        secsRem = 0; // set to zero remaining seconds when overflow occurs
+    }
 
     JsonStatus[F ("SyncCount")]           = SyncControl.SyncCount;
     JsonStatus[F ("SyncAdjustmentCount")] = SyncControl.SyncAdjustmentCount;
@@ -193,44 +208,18 @@ void c_InputFPPRemotePlayFile::GetStatus (JsonObject& JsonStatus)
     JsonStatus[CN_playlist]          = temp;
     JsonStatus[CN_seconds_elapsed]   = String (secs);
     JsonStatus[CN_seconds_played]    = String (secs);
-    JsonStatus[CN_seconds_remaining] = String (secsTot - secs);
+    JsonStatus[CN_seconds_remaining] = String (secsRem);
     JsonStatus[CN_sequence_filename] = temp;
     JsonStatus[F("PlayedFileCount")] = PlayedFileCount;
 
-    int mins = secs / 60;
-    secs = secs % 60;
-
-    secsTot = secsTot - secs;
-    int minRem = secsTot / 60;
-    secsTot = secsTot % 60;
-
-
-    // Manual calculation of maximum size string buffer required:
-    // mseconds is uint32         , range [0..4294967295]
-    // mins is 1/(60*1000) of that, range [0..71582]
-    // msecondsTotal is uint32    , range [0..4294967295]
-    // minRem is 1/(60*1000)...   , range [0..4294967295] due to potential for overflow (else 71582)
-    // secs                       , range [0..59]
-    // secsTot                    , range [0..59]
-    //
-    // Therefore, maximum string is for time remaining: "4000111222:33", which requires 14 bytes
-    // If overflow is fixed, the maximum string is:     "71582:99", which requires 9 bytes
-
-    // Avoid use of unsafe `sprintf` ... especially with stack-based buffers
-    static const int TMP_BUFFER_CHAR_COUNT = 14;
-    char buf[TMP_BUFFER_CHAR_COUNT];
-
-    int writtenChars = snprintf (buf, TMP_BUFFER_CHAR_COUNT, "%02d:%02d", mins, secs);
-    // TODO: assert ((writtenChars > 0) && (writtenChars < TMP_BUFFER_CHAR_COUNT))
-    if ((writtenChars > 0) && (writtenChars < TMP_BUFFER_CHAR_COUNT)) {
-        JsonStatus[CN_time_elapsed] = buf;
-    }
-
-    writtenChars = snprintf (buf, TMP_BUFFER_CHAR_COUNT, "%02d:%02d", minRem, secsTot);
-    // TODO: assert ((writtenChars > 0) && (writtenChars < TMP_BUFFER_CHAR_COUNT))
-    if ((writtenChars > 0) && (writtenChars < TMP_BUFFER_CHAR_COUNT)) {
-        JsonStatus[CN_time_remaining] = buf;
-    }
+    // After inserting the total seconds and total seconds remaining,
+    // JsonStatus also includes formatted "minutes + seconds" for both
+    // timeElapsed and timeRemaining
+    char buf[12];
+    ESP_ERROR_CHECK(saferSecondsToFormattedMinutesAndSecondsString(buf, secs));
+    JsonStatus[CN_time_elapsed] = buf;
+    ESP_ERROR_CHECK(saferSecondsToFormattedMinutesAndSecondsString(buf, secsRem));
+    JsonStatus[CN_time_remaining] = buf;
 
     JsonStatus[CN_errors] = LastFailedPlayStatusMsg;
 

ESPixelStick/src/input/InputFPPRemotePlayList.hpp

@@ -59,6 +59,24 @@ class c_InputFPPRemotePlayList : public c_InputFPPRemotePlayItem
     c_InputFPPRemotePlayItem * pInputFPPRemotePlayItem = nullptr;
 
     uint32_t PlayListEntryId     = 0;
+
+    // BUGBUG -- time_t creates issues for portable code, and for overflow-safe code
+    // time_t is only required to be a "real" type, which means it can be either a float or an integer.
+    // even when time_t is an integer type, it can be signed or unsigned
+    //
+    // Current code appears to assume that time_t is not a float.
+    //
+    // C++11 does not expose any constant for minimum / maximum value for the time_t type.
+    // This makes it impossible (without type traits) to ensure math operations on time_t types do
+    // not result in undefined behavior (signed types only) and/or overflow (all types).
+    //
+    // Therefore, it is highly recommended to move AWAY from use of time_t wherever possible,
+    // and instead use types that expose their minimum and maximum values, respectively.
+    //
+    // For now, because the code presumes time_t is an integer type, the following assertion
+    // ensures this for all practical purposes.
+    static_assert( (((time_t)1) / 2) == 0 ); // Verify time_t is an integer type (alternative: float)
+
     time_t   PauseEndTime        = 0;
     uint32_t PlayListRepeatCount = 1;
 

ESPixelStick/src/input/InputFPPRemotePlayListFsm.cpp

@@ -20,6 +20,7 @@
 
 #include "InputFPPRemotePlayList.hpp"
 #include "../service/FPPDiscovery.h"
+#include "../utility/SaferStringConversion.hpp"
 #include "../FileMgr.hpp"
 #include "InputFPPRemotePlayEffect.hpp"
 
@@ -357,20 +358,19 @@ void fsm_PlayList_state_Paused::GetStatus (JsonObject& jsonStatus)
     // DEBUG_START;
 
     JsonObject PauseStatus = jsonStatus.createNestedObject (CN_Paused);
-    
+
     time_t now = millis ();
 
-    time_t SecondsRemaining = (pInputFPPRemotePlayList->PauseEndTime - now) / 1000;
+    time_t SecondsRemaining = (pInputFPPRemotePlayList->PauseEndTime - now) / 1000u;
     if (now > pInputFPPRemotePlayList->PauseEndTime)
     {
         SecondsRemaining = 0;
     }
 
-    time_t MinutesRemaining = min(time_t(99), time_t(SecondsRemaining / 60));
-    SecondsRemaining = SecondsRemaining % 60;
-
-    char buf[10];
-    sprintf (buf, "%02u:%02u", uint32_t(MinutesRemaining), uint32_t(SecondsRemaining));
+    char buf[12];
+    // BUGBUG -- casting time_t to integer types is not portable code (can be real ... e.g., float)
+    // BUGBUG -- no portable way to know maximum value of time_t, or if it's signed vs. unsigned (without type traits)
+    ESP_ERROR_CHECK(saferSecondsToFormattedMinutesAndSecondsString(buf, (uint32_t)SecondsRemaining));
     PauseStatus[F ("TimeRemaining")] = buf;
 
     // DEBUG_END;

ESPixelStick/src/utility/SaferStringConversion.hpp

@@ -0,0 +1,93 @@
+#pragma once
+/*
+* ESPixelStick.h
+*
+* Project: ESPixelStick - An ESP8266 / ESP32 and E1.31 based pixel driver
+* Copyright (c) 2016, 2022 Shelby Merrick
+* http://www.forkineye.com
+*
+*  This program is provided free for you to use in any way that you wish,
+*  subject to the laws and regulations where you are using it.  Due diligence
+*  is strongly suggested before using this code.  Please give credit where due.
+*
+*  The Author makes no warranty of any kind, express or implied, with regard
+*  to this program or the documentation contained in this document.  The
+*  Author shall not be liable in any event for incidental or consequential
+*  damages in connection with, or arising out of, the furnishing, performance
+*  or use of these programs.
+*
+*/
+
+#include "../ESPixelStick.h"
+#include "./backported.h"
+
+// The following template functions' first parameter is defined
+// as a **reference** to an array of characters.  The size of
+// the array is the template parameter.  This allows the function
+// to statically assert that the passed-in buffer is large enough
+// to always succeed.  Else, a compilation will occur.
+//
+// Allowing the compiler to deduce the template parameters has
+// multiple benefits:
+// * Users can ignore that the function is a template
+// * Code remains easy to read
+// * Compiler doesn't deduce wrong size
+// * Even if a user manually enters a larger template size,
+//   the compiler will report an error, such as:
+//   error: invalid initialization of reference of type 'char (&)[15]'
+//          from expression of type 'char [12]'
+
+
+
+// Safer RGB to HTML string (e.g., "#FF8833") conversion function
+//
+// Example use:
+//     char foo[8];
+//     ESP_ERROR_CHECK(saferRgbToHtmlColorString(foo, led.r, led.g, led.b));
+//
+template <size_t N>
+inline esp_err_t saferRgbToHtmlColorString(char (&output)[N], uint8_t r, uint8_t g, uint8_t b) {
+    esp_err_t result = ESP_FAIL;
+
+    // Including the trailing null, this string requires eight (8) characters.
+    //
+    // The output is formatted as "#RRGGBB", where RR, GG, and BB are two hex digits
+    // for the red, green, and blue components, respectively.
+    static_assert(N >= 8);
+    static_assert(sizeof(int) <= sizeof(size_t)); // casting non-negative int to size_t is safe
+    int wouldHaveWrittenChars = snprintf(output, N, "#%02x%02x%02x", r, g, b);
+    if (likely((wouldHaveWrittenChars > 0) && (((size_t)wouldHaveWrittenChars) < N))) {
+        result = ESP_OK;
+    } else {
+        // TODO: assert((wouldHaveWrittenChars > 0) && (wouldHaveWrittenChars < N));
+    }
+    return result;
+}
+// Safer seconds to "Minutes:Seconds" string conversion function
+//
+// Example use:
+//     char foo[12];
+//     ESP_ERROR_CHECK(saferSecondsToFormattedMinutesAndSecondsString(foo, seconds));
+//
+template <size_t N>
+inline esp_err_t saferSecondsToFormattedMinutesAndSecondsString(char (&output)[N], uint32_t seconds) {
+    esp_err_t result = ESP_FAIL;
+
+    // Including the trailing null, the string may require up to twelve (12) characters.
+    //
+    // The output is formatted as "{minutes}:{seconds}".
+    // uint32_t seconds is in range [0..4294967295].
+    // therefore, minutes is in range [0..71582788] (eight characters).
+    // seconds is always exactly two characters.
+    static_assert(N >= 12);
+    static_assert(sizeof(int) <= sizeof(size_t)); // casting non-negative int to size_t is safe
+    uint32_t m = seconds / 60u;
+    uint8_t  s = seconds % 60u;
+    int wouldHaveWrittenChars = snprintf(output, N, "%u:%02u", m, s);
+    if (likely((wouldHaveWrittenChars > 0) && (((size_t)wouldHaveWrittenChars) < N))) {
+        result = ESP_OK;
+    } else {
+        // TODO: assert((wouldHaveWrittenChars > 0) && (wouldHaveWrittenChars < N));
+    }
+    return result;
+}