Aircoookie / WLED

Control WS2812B and many more types of digital RGB LEDs with an ESP8266 or ESP32 over WiFi!
https://kno.wled.ge
MIT License
15.04k stars 3.25k forks source link

"magic numbers" for MQTT topic buffer length in mqtt.cpp and button.cpp (use already defined MQTT_MAX_TOPIC_LEN) #4293

Open WouterGritter opened 2 days ago

WouterGritter commented 2 days ago

What happened?

I was modifying the source code on a fork for my custom MQTT topic structure and noticed MQTT_MAX_TOPIC_LEN is not used in all places the associated variables mqttDeviceTopic and mqttGroupTopic are used.

When increasing this defined variable, buffer overflows will occur when trying to generate topics to publish on in mqtt.cpp and button.cpp. I personally organize all MQTT devices in a <building>/<room>/<device> topic structure, and this could potentially be more than the current 32 max topic length.

Currently, when trying to increase this variable, the following things need to be changed:

It's bug-prone to have to manually adjust the buffer sizes in both cpp files. However it might be nice to have to also automatically populate the maxlength attribute on the <input> element. Not sure if this easy though.


Not applicable headers removed.

What version of WLED?

0.15.0-b7

Anything else?

~If no one else is one this after a while I'll try to submit a PR myself. Bare with me if I do that, because I've never contributed to open source before!~

I'm submitting a PR for the fix.

Code of Conduct

WouterGritter commented 2 days ago

Opened a PR for this.

WouterGritter commented 1 day ago

!4295 uses MQTT_MAX_TOPIC_LEN to allocate topic string buffers.

Currently, when trying to increase this variable, the following things need to be changed:

  • maxlength attribute on <input> elements in settings_sync.html

This was already implemented but the feature was broken. !4296 fixes this.

blazoncek commented 15 hours ago

You can close this now.

WouterGritter commented 1 hour ago

!4295 (the one that fixes magic numbers) is still open. Two PRs were spawned from this issue.