some precautions to prevent buffer out-of-bounds access and concurrency problems

* make sure that filenames can hold 32chars of segment.name
* fix logic error in Segment::drawCharacter
* protect free(Segment::_globalLeds) with a critical section
This commit is contained in:
Frank
2025-11-14 22:43:44 +01:00
parent 47f96fdd4a
commit 9f31f2444f
2 changed files with 21 additions and 6 deletions

View File

@@ -101,7 +101,7 @@ void WS2812FX::setUpMatrix() {
// content of the file is just raw JSON array in the form of [val1,val2,val3,...]
// there are no other "key":"value" pairs in it
// allowed values are: -1 (missing pixel/no LED attached), 0 (inactive/unused pixel), 1 (active/used pixel)
char fileName[32]; strcpy_P(fileName, PSTR("/2d-gaps.json")); // reduce flash footprint
char fileName[32]; strcpy_P(fileName, PSTR("/2d-gaps.json")); // reduce flash footprint //WLEDMM you sure?
bool isFile = WLED_FS.exists(fileName);
size_t gapSize = 0;
int8_t *gapTable = nullptr;
@@ -842,11 +842,11 @@ void Segment::drawArc(unsigned x0, unsigned y0, int radius, uint32_t color, uint
//WLEDMM for artifx
bool Segment::jsonToPixels(char * name, uint8_t fileNr) {
if (!isActive()) return true; // segment not active, nothing to do
char fileName[32] = { '\0' };
char fileName[42] = { '\0' }; // we need up to 40 bytes (seg.name is 32 bytes max)
//WLEDMM: als support segment name ledmaps
bool isFile = false;
// strcpy_P(fileName, PSTR("/mario"));
snprintf(fileName, sizeof(fileName), "/%s%d.json", name, fileNr); //WLEDMM: trick to not include 0 in ledmap.json
snprintf(fileName, sizeof(fileName)-1, "/%s%d.json", name, fileNr); //WLEDMM: trick to not include 0 in ledmap.json
// strcat(fileName, ".json");
isFile = WLED_FS.exists(fileName);
@@ -928,7 +928,7 @@ void Segment::drawCharacter(unsigned char chr, int16_t x, int16_t y, uint8_t w,
for (int j = 0; j<w; j++) { // character width
int16_t x0 = x + (w-1) - j;
if ((x0 >= 0) || (x0 < cols)) {
if (unsigned(x0) < cols) { // WLEDMM same as "x0 > 0 && x0 < cols"
if ((bits>>(j+(8-w))) & 0x01) { // bit set & drawing on-screen
setPixelColorXY(x0, y0, fgCol);
} else {

View File

@@ -8,7 +8,10 @@
#include "FX.h"
#include "palettes.h"
#ifdef ARDUINO_ARCH_ESP32
#include <esp_timer.h> // WLEDMM to get esp_timer_get_time()
#include <esp_timer.h> // WLEDMM to get esp_timer_get_time()
#include "freertos/FreeRTOS.h"
#include "freertos/portmacro.h"
static portMUX_TYPE s_wled_strip_mux = portMUX_INITIALIZER_UNLOCKED; // to protect deleting Segment::_globalLeds
#endif
/*
@@ -953,7 +956,9 @@ static void xyFromBlock(uint16_t &x,uint16_t &y, uint16_t i, uint16_t vW, uint16
x = vW / 2 - vStrip - 1;
y = vH / 2 + vStrip - i2 * vStrip * 2;
}
// softhack007 not sure if clamping is necessary
//x = min(x, uint16_t(vW-1)); // clamp x at vW-1
//y = min(y, uint16_t(vH-1)); // clamp y at vH-1
}
void IRAM_ATTR_YN WLED_O2_ATTR __attribute__((hot)) Segment::setPixelColor(int i, uint32_t col) //WLEDMM: IRAM_ATTR conditionally
@@ -1847,9 +1852,19 @@ void WS2812FX::finalizeInit(void)
//initialize leds array. TBD: realloc if nr of leds change
if (Segment::_globalLeds) {
// DONG - Valkyrie is about to die
// this is a critical section that will be removed with PR #278 which removes _globalLeds
// problem: suspendStripService provides interlocking, but theres a window before service() observes it,
// and ESP32 is dual-core. A critical section closes that window so the pointer swap is atomic across cores.
#if defined(ARDUINO_ARCH_ESP32)
taskENTER_CRITICAL(&s_wled_strip_mux);
#endif
free(Segment::_globalLeds);
Segment::_globalLeds = nullptr;
purgeSegments(true); // WLEDMM moved here, because it seems to improve stability.
#if defined(ARDUINO_ARCH_ESP32)
taskEXIT_CRITICAL(&s_wled_strip_mux);
#endif
}
if (useLedsArray && getLengthTotal()>0) { // WLEDMM avoid malloc(0)
size_t arrSize = sizeof(CRGB) * getLengthTotal();