Remove use of sprintf() (unsafe API)

[henrygab]

Issues

The current code has a number of stack-based arrays that are used to create temporary strings. Unfortunately, this is currently using sprintf(), which is an inherently unsafe API.
These are fixed-format strings, with known input types, so the maximum buffer size that could be required is known to people (not to the compiler). In fact, the existing buffer sizes were large enough ... but this was not enforced / verified at compilation. Therefore, it raised the possibility of adding errors when maintaining the code that would only be found at runtime.

Goals included

• Enable compile-time detection that stack-allocated buffers are sufficiently sized for common string conversions
• Do not rely on user-provided array size ... force compiler to detect array size
• Consolidate complex checks into one function (per format string)
• Remove use of inherently-unsafe APIs

How

The solution combines C++ features to produce a powerful compile-time assurance:

1. C++ allows references to fixed-size arrays as function parameter ... but the array size must be known at compilation.
2. C++ template parameters can be an integral type (e.g., size_t)
3. static_assert() that the size_t template parameter is sufficiently large

Putting these together, the template functions assure (at compile-time) that the parameter is a reference to an array of at least the minimum size. The caller doesn't pass the array size ... it's inferred by the compiler from the function's argument. This removes a source of error, while keeping the code clean and easy-to-read.

Validation

Validation was done by manually forcing the new templated functions to execute from setup() and loop(), logging results to the console.

Validation -- the path NOT chosen

The following lists the affected code paths. These code paths were not necessarily all exercised with this PR. Rather, the new code was validated to result in identical output. Leaving this in for posterity, but not expanded by default.

Tasks remaining

TODO: Verify each of the following generate identical JSON:

• c_InputAlexa::onMessage()
• c_InputEffectEngine::GetConfig()
• fsm_PlayEffect_state_PlayingEffect::GetStatus()
• c_InputFPPRemotePlayFile::GetStatus()
• fsm_PlayList_state_Paused::GetStatus()
• c_EthernetDriver::GetStatus() (for an ethernet enabled build)

---

[MartinMueller2003]

This is a temporary file that is copied from the ESPressif IDF to allow me to debug Ethernet functionality. It has already been deprecated now that a stable Ethernet release has been identified.

[MartinMueller2003]

General comment

I prefer functions that have one entry and ONE exit. These function have multiple exists making them more difficult to debug. For small functions like this it is not critical to have a single exit but for more complex functions it is very important.

[henrygab]

I prefer functions that have one entry and ONE exit. These function have multiple exists making them more difficult to debug. For small functions like this it is not critical to have a single exit but for more complex functions it is very important.

We are in agreement on this goal. There is only one function exit point at the end of these functions. It has a single if statement for which return code to use. Can you help me understand your alternative (preferred) way to conclude the template functions?

[henrygab]

Note: Testing is being hampered by platformio/platformio-vscode-ide#3207; Most attempts to build are currently failing for reasons unrelated to ESPixelstick code.

[MartinMueller2003]

Note: Testing is being hampered by platformio/platformio-vscode-ide#3207; Most attempts to build are currently failing for reasons unrelated to ESPixelstick code.

Interesting. I am on vscode latest with latest pio. I have also upgraded the pio framework to pick up bug fixes. I do get an sql error if I build a clean directory (rebuild fixes it) and if I make a change in the platformio file I have to restart vscode but the builds work.

[henrygab]

Yes. The frequency of the error changed from rare (as you see it now), to >90% (until Intellisense is stopped for 10 minutes). Cause is not yet known.

I have a repro in a fresh VM, and am setting up remote access for PlatformIO folks to root cause the issue.

[MartinMueller2003]

I prefer functions that have one entry and ONE exit. These function have multiple exists making them more difficult to debug. For small functions like this it is not critical to have a single exit but for more complex functions it is very important.

We are in agreement on this goal. There is only one function exit point at the end of these functions. It has a single if statement for which return code to use. Can you help me understand your alternative (preferred) way to conclude the template functions?

Here is an example of a function with two exits. There are two return statements. A response variable should have been set and then a return Response gets stuck at the bottom of the function. In the case of the example function I would initialize the response with fail and then only set it to ok if everything went well.

int wouldHaveWrittenChars = snprintf(output, N, "#%02x%02x%02x", r, g, b);
if (likely((wouldHaveWrittenChars > 0) && (((size_t)wouldHaveWrittenChars) < N))) {
return ESP_OK;
}
// TODO: assert ((wouldHaveWrittenChars > 0) && (wouldHaveWrittenChars < N))
return ESP_FAIL;

[henrygab]

OK, manually forced execution of the template functions. Ready for final review and, I believe, approval.

[MartinMueller2003]

These changes are fine. I see there are still a few cases of time_t and the use of 1000 in the modified code. Those things just seem to stick around.

[henrygab]

These changes are fine. I see there are still a few cases of time_t and the use of 1000 in the modified code. Those things just seem to stick around.

Yes, I didn't expand the focus of this PR, although I tried to update the functions which I had touched (per your request). time_t changes would be a separate PR.