Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 task done
WouterGritter opened this issue Nov 19, 2024 · 4 comments

Comments

@WouterGritter
Copy link
Contributor

WouterGritter commented Nov 19, 2024

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:

  • MQTT_MAX_TOPIC_LEN
  • lots of buffer allocation code in mqtt.cpp and button.cpp
  • maxlength attribute on <input> elements in settings_sync.html

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

  • I agree to follow this project's Code of Conduct
@WouterGritter
Copy link
Contributor Author

Opened a PR for this.

@WouterGritter
Copy link
Contributor Author

WouterGritter commented Nov 21, 2024

!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 blazoncek added the fixed in source This issue is unsolved in the latest release but fixed in master label Nov 21, 2024
@blazoncek
Copy link
Collaborator

You can close this now.

@WouterGritter
Copy link
Contributor Author

WouterGritter commented Nov 22, 2024

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

@netmindz netmindz removed the fixed in source This issue is unsolved in the latest release but fixed in master label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants