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

[WouterGritter]

See issue #4293.

As seen in wled.h, MQTT_MAX_TOPIC_LEN is currently defined to be 32. All modified lines of code result in the same value by adding a number to MQTT_MAX_TOPIC_LEN, allowing this variable to be increased without the risk of buffer overflows at a later date or by someone compiling the project with the modification themselves.

[WouterGritter]

@softhack007 no it should be + 32, see lines ~150+ in button.cpp:

    if (buttonPublishMqtt && WLED_MQTT_CONNECTED) {
      char subuf[MQTT_MAX_TOPIC_LEN + 32];
      if (buttonType[b] == BTN_TYPE_PIR_SENSOR) sprintf_P(subuf, PSTR("%s/motion/%d"), mqttDeviceTopic, (int)b);
      else sprintf_P(subuf, _mqtt_topic_button, mqttDeviceTopic, (int)b);
      mqtt->publish(subuf, 0, false, !buttonPressedBefore[b] ? "off" : "on");
    }

Here, it puts mqttDeviceTopic at the beginning of /motion/, which is followed by a digit which has nothing to do with the topic length. The only variable part (apart from the digit which is always reasonably short in length, and accounted for in the +32) is the mqttDeviceTopic string.

Elsewhere, in the same file it doesn't use the literal string "%s/motion/%d" but rather a variable _mqtt_topic_button, which is essentially the same as using the literal "%s/button/%d", only converted to a variable (stored in PROGMEM) to omtimize flash storage (line 15):

static const char _mqtt_topic_button[] PROGMEM = "%s/button/%d";  // optimize flash usage

[WouterGritter]

I am now noticing other places that use the mqttGroupTopic rather than mqttDeviceTopic which also use hard-coded char buffer lengths, even though mqttGroupTopic is also of length MQTT_MAX_TOPIC_LEN. Will need to modify these buffer sizes too.

[WouterGritter]

In wled_eeprom.cpp the topic length is currently also hard-coded to a length of 32, something which this PR would also need to modify:

  if (lastEEPROMversion > 8)
  {
    readStringFromEEPROM(2300, mqttServer, 32);
    readStringFromEEPROM(2333, mqttDeviceTopic, 32);
    readStringFromEEPROM(2366, mqttGroupTopic, 32);
  }

Modifying this would corrupt the EEPROM config when upgrading to a version of WLED which has the max topic length increased. It would also mean calculating every hard-coded EEPROM address with an offset that's dependent on MQTT_MAX_TOPIC_LEN... Suddenly this seems really impractical. Thoughts?

[softhack007]

In wled_eeprom.cpp the topic length is currently also hard-coded to a length of 32, something which this PR would also need to modify:
Modifying this would corrupt the EEPROM config when upgrading to a version of WLED which has the max topic length increased. It would also mean calculating every hard-coded EEPROM address with an offset that's dependent on MQTT_MAX_TOPIC_LEN... Suddenly this seems really impractical. Thoughts?

I think you don't need to worry much about EEPROM - this feature is deprecated, and the code is not active unless WLED_ADD_EEPROM_SUPPORT is defined by the buildenv. Today WLED's configuration is stored in cfg.json on LittleFS.

I think it would be best to just keep the EEPROM code as it is - just add a warning like this in wled_EEPROM.cpp (after #include "wled.h")

#if defined(WLED_ENABLE_MQTT) && MQTT_MAX_TOPIC_LEN > 32
#error "MQTT topics length > 32 is not supported by the EEPROM module!"
#endif

[WouterGritter]

@softhack007 great! I just saw that the wled_eeprom.cpp file is only used for reading older settings indeed. I don't think a warning is even needed, we can just read it as a hardcoded 32-byte length and put it in the larger mqttDeviceTopic string. This would cause issues when MQTT_MAX_TOPIC_LEN is set to less than 32 though, maybe we can set a warning or add a comment not to set it lower than 32.

I think I'll continue this fix then. Will update the other files that touch mqttDeviceTopic/mqttGroupTopic tomorrow, and I'll resolve the threads about the + 32 as per my comment above.

[WouterGritter]

All other references to mqttDeviceTopic and mqttGroupTopic are outside the wled00 source folder, mainly in the usermods. I think fixing those is outside of the concern of this PR and issue.

I've added a comment explaining this, that's also warning against setting the value of MQTT_MAX_TOPIC_LEN lower than 32. If you'll approve the PR it's ready for merging now!

#define MQTT_MAX_TOPIC_LEN 32 // should not be less than 32. might cause trouble when increased with usermods active that do not handle this correctly.

[softhack007]

#define MQTT_MAX_TOPIC_LEN 32 // should not be less than 32. might cause trouble when increased with usermods active that do not handle this correctly.

I would actually prefer a compile-time #warning added in mqtt.cpp.

[WouterGritter]

I've replaced the comment with a warning and an error:

• LEN > 32: warning in MQTT that this might cause problems when using usermods.
• LEN < 32: error in eeprom (if mqtt enabled) that this is not supported. len < 32 will cause buffer overflow when writing eeprom data to memory. len > 32 does not matter as it will only fill a max. of 32 chars.

Should be ready to merge now!

[softhack007]

Thanks, looks good to me
code reviewed but not tested from my side, as I don't have a MQTT broker setup at home.

I'd suggest we include this PR into the next feature update after 0.15.0 was released.

[netmindz]

Thank you for putting this together, magic numbers are horrible so great to remove them. However I still see some magic numbers within your change. If these could be eliminated as well please

E.g MQTT_MAX_TOPIC_LEN + 32

That is better than just 64, but you still have the magic number of 32 so it not really resolved the magic number issue

[WouterGritter]

Thank you for putting this together, magic numbers are horrible so great to remove them. However I still see some magic numbers within your change. If these could be eliminated as well please

E.g MQTT_MAX_TOPIC_LEN + 32

That is better than just 64, but you still have the magic number of 32 so it not really resolved the magic number issue

I get what you're saying, but IMO this is way less worse than the situation was previously. With these "magic numbers", you see the code that uses it within 10 lines of the addition to the buffer length. You see the mqtt prefix variable, a static string, and a strcpy call to copy them both to the buffer.

This is in contrast to the use of MQTT_MAX_TOPIC_LEN, which refers to the max length of the mqttDeviceTopic variable which was used across multiple files and lots of places with the assumption that MQTT_MAX_TOPIC_LEN was always 32, even though it could be changed.

I'm also not sure as how to get rid of these effectively, storing the cstring that's added to the topic prefix, and calculating the length before generating a buffer that holds the composite of both strings seems overkill and would add (imo unnecessary) CPU time.

[WouterGritter]

Thanks, looks good to me code reviewed but not tested from my side, as I don't have a MQTT broker setup at home.

I'd suggest we include this PR into the next feature update after 0.15.0 was released.

For what it's worth coming from a first-time collaborator, the code works fine on my end. I have been controlling some WLED lights through MQTT with a build where the max length was increased to 64 using this PR without any weirdness.

[WouterGritter]

I suggest we merge this and possibly look at a more big-picture removal of magic numbers at a later date.

[netmindz]

Thank you for putting this together, magic numbers are horrible so great to remove them. However I still see some magic numbers within your change. If these could be eliminated as well please
E.g MQTT_MAX_TOPIC_LEN + 32
That is better than just 64, but you still have the magic number of 32 so it not really resolved the magic number issue

I get what you're saying, but IMO this is way less worse than the situation was previously. With these "magic numbers", you see the code that uses it within 10 lines of the addition to the buffer length. You see the mqtt prefix variable, a static string, and a strcpy call to copy them both to the buffer.

This is in contrast to the use of MQTT_MAX_TOPIC_LEN, which refers to the max length of the mqttDeviceTopic variable which was used across multiple files and lots of places with the assumption that MQTT_MAX_TOPIC_LEN was always 32, even though it could be changed.

I'm also not sure as how to get rid of these effectively, storing the cstring that's added to the topic prefix, and calculating the length before generating a buffer that holds the composite of both strings seems overkill and would add (imo unnecessary) CPU time.

It's definitely an improvement
Can you not just define another constant for what 32 actually refers to?

Thank you for putting this together, magic numbers are horrible so great to remove them. However I still see some magic numbers within your change. If these could be eliminated as well please
E.g MQTT_MAX_TOPIC_LEN + 32
That is better than just 64, but you still have the magic number of 32 so it not really resolved the magic number issue

This is in contrast to the use of MQTT_MAX_TOPIC_LEN, which refers to the max length of the mqttDeviceTopic variable which was used across multiple files and lots of places with the assumption that MQTT_MAX_TOPIC_LEN was always 32, even though it could be changed.

I'm also not sure as how to get rid of these effectively, storing the cstring that's added to the topic prefix, and calculating the length before generating a buffer that holds the composite of both strings seems overkill and would add (imo unnecessary) CPU time.

It's definitely an improvement, can you not just define a constant with a nice name and a value of 32 and the same for your magic 6 ?