analyze/fix potential strings out-of-bounds and use-after-free cases

[softhack007]

What happened?

This tickets collects several potential memory access problems that popped up during code reviews of PRs.

case 1 (not a bug)

wled00/util.cpp 404-405: Out‑of‑bounds write: null‑terminator placed past buffer end.

Use maxLen‑1 index.

-        dest[maxLen] = '\0'; // strncpy does not necessarily null terminate string
+        dest[maxLen-1] = '\0'; // ensure termination within bounds

--> not a bug; only called from rotary usermod, with dest[64] and maxLen=63

case 2 (confirmed)

wled00/util.cpp 374-377: Dangling pointer from temporary String (use‑after‑free).
names.substring(...).c_str() returns a pointer to a temporary; it’s invalid by the next statement. Keep a String alive for the copy.

-                if (nameEnd<0) tmpstr = names.substring(nameBegin).c_str(); // did not find ",", last name?
-                else           tmpstr = names.substring(nameBegin, nameEnd).c_str();
-                strlcpy(dest, tmpstr, maxLen); // copy the name into buffer (replacing previous)
+                {
+                  String sub = (nameEnd < 0) ? names. Substring(nameBegin)
+                                             : names.substring(nameBegin, nameEnd);
+                  strlcpy(dest, sub.c_str(), maxLen); // copy name into buffer
+                }

case 3 (not a bug)

wled00/util.cpp 293-299: Fix off‑by‑one NUL write in extractModeName.

When j reaches maxLen, dest[j] writes one past the buffer.

Apply:

-      for (; j < maxLen && j < len; j++) {
+      for (; (j + 1) < maxLen && j < len; j++) {
         if (lineBuffer[j] == '\0' || lineBuffer[j] == '@') break;
         dest[j] = lineBuffer[j];
       }
-      dest[j] = 0; // terminate string
+      dest[j] = '\0'; // terminate string

--> not a bug; called from several usermods; verified that maxLen is always len-1 (safe).

case 4 (confirmed)

wled00/ir.cpp 676-685: Potential buffer overflow in objKey formatting

""0x%lX":" can be up to 13 chars for 0xFFFFFFFF (plus NUL). objKey[10] is too small. Use a larger buffer and snprintf.

-  char objKey[10];
+  char objKey[16];
@@
-  sprintf_P(objKey, PSTR("\"0x%lX\":"), (unsigned long)code);
+  snprintf_P(objKey, sizeof(objKey), PSTR("\"0x%lX\":"), (unsigned long)code);

case 5 (to be analyzed)

wled00/ir.cpp 186-186: possible Bug: writing intensity into speed

Should assign intensity, not speed, when not applying to all segments.

-      strip.getMainSegment().speed = effectIntensity;
+      strip.getMainSegment().intensity = effectIntensity;

case 6 (not a bug)

wled00/util.cpp 162-177: Fix off-by-one overflow in oappend() null terminator check
(from #152 (review))

strcpy() writes a trailing '\0'. The current capacity check doesn’t reserve space for it; when olen + len == SETTINGS_STACK_BUF_SIZE - 1, the '\0' writes out of bounds.

Apply:

-  if ((obuf == nullptr) || (olen + len >= SETTINGS_STACK_BUF_SIZE)) { // sanity checks
+  if ((obuf == nullptr) || (olen + len + 1 > SETTINGS_STACK_BUF_SIZE)) { // reserve room for '\0'
Optionally use strlcpy:

-  strcpy(obuf + olen, finalTxt);
-  olen += len;
+  olen += strlcpy(obuf + olen, finalTxt, SETTINGS_STACK_BUF_SIZE - olen);

--> not a bug, the only "obuf" that's close to the stack buffer limit is in serveSettingsJS() with char buf[SETTINGS_STACK_BUF_SIZE+37]

plus

fixes for usermods, see #270 (comment)

What version/release of MM WLED?

mdev latest in development

Which microcontroller/board are you seeing the problem on?

Other, ESP32

Code of Conduct

• I agree to follow this project's Code of Conduct