Use of unsafe functions

[henrygab]

This is to track issues related to the use of "unsafe" functions.

Example resources:

• Github banned API header file
• Microsoft banned API header file
• Microsoft Secure Development banned APIs documentation

[MartinMueller2003]

Interesting. David K is going to need to update the time generation functions in the FSEQ support files. Never did like those functions.

[henrygab]

This post will focus first on the smaller set of github-banned APIs.

First, a note about the safe versions of time functions.

As with all bounds-checked functions, ctime_s is only guaranteed to be available if __STDC_LIB_EXT1__ is defined by the implementation and if the user defines __STDC_WANT_LIB_EXT1__ to the integer constant 1 before including time.h.

• gmtime() is used only once, in c_FPPDiscovery::BuildFseqResponse()

  • Not thread-safe; specifically:

  "pointer to a static internal tm object on success, or null pointer otherwise. The structure may be shared between gmtime, localtime, and ctime and may be overwritten on each invocation."

  • Should replace with gmtime_r() ... which is exported from the ESP32 ROM
  • Could also replace with gmtime_s() (for parity with ctime_s())

• ctime() is used only once, in c_WebMgr::processCmdSetTime()

  • Should replace with ctime_s()

• strcpy() is used extensively

  • All uses can be replaced with strlcpy() ... it's even in the ESP32 ROM
  • Many functions in WebMgr.cpp
    • All uses access the variable pWebSocketFrameCollectionBuffer
    • No noticed code ensuring pWebSocketFrameCollectionBuffer never overflows
  • c_FPPDiscovery::sendPingPacket() uses it also
    • Implicitly assumes strings (defined in other files) have sufficiently short length

• strcat() is used extensively in WebMgr.cpp

  • Likely should simply replace with strlcat() ... it's even in the ESP32 ROM
  • Same comments as for strcpy() ...
  • Is there something ensuring pWebSocketFrameCollectionBuffer does not overflow?

---

Other github banned APIs do not appear to be issues

• sprintf() is used multiple times; snprintf() has potential for misunderstanding, but is still generally a better option for new code.
  • c_InputAlexa::onMessage() -- stack-allocated buffer; have not noticed any situation where buffer overflow might occur
  • c_InputEffectEngine::GetConfig() -- Same as prior call ... stack-allocated buffer, identical functionality
  • fsm_PlayEffect_state_PlayingEffect::GetStatus() -- stack-allocated buffer, but variable max values are ensured ... not noticing any unsafe possibility
  • fsm_PlayList_state_Paused::GetStatus() -- Substantially same as prior call ... stack-allocated buffer, but variable max values are ensured
  • ETHClass::macAddress() -- stack-allocated buffer, but fixed-format, limited-range variables output fixed-length string ... not noticing any unsafe possibilities.

The remainder of the APIs in the github banned API header are not currently used in the code.

---

[MartinMueller2003]

Yes there are checks built in to make sure the buffer does not overflow. They are in the web socket message start processing and chuck receive processing.