MQTT: don't recycle a queue slot whose publish failed (#2230)

[codypendant]

Addresses #2230. Analysis and the captured device log are in
#2230 (comment)

1. A failed publish released the queue slot

result = MQTT_PublishTopicToClient(mqtt_client, head->topic, head->channel, head->value, head->flags, false);
MQTT_QUEUE_ITEM_SET_REUSABLE(head);   // <-- before the result is checked
g_MqttPublishItemsQueued--;

//Stop if last publish failed
if (result != OBK_PUBLISH_OK) break;

MQTT_QUEUE_ITEM_SET_REUSABLE() clears topic[0], and topic[0] == 0 is
exactly how find_queue_reusable_item() decides a slot is free. So on
OBK_PUBLISH_MEM_FAIL the payload is lost and the slot is immediately
available to the next MQTT_QueuePublish(), which strcpys a new topic,
channel and value into an entry that is still being referenced.

Captured on a BK7231T (1.18.311) publishing HA discovery:

Publishing val (374 bytes) to homeassistant/light/OpenBK7231T_0668EFA8_light_1/config retain=1
Publishing val (1323 bytes) to  retain=0                     <-- empty topic
Publishing val (179 bytes) to ue="2" o,<garbage> retain=1    <-- garbage topic
mqtt_connection_cb: Disconnected, reason: 256(Disconnected)

ue="2" is a fragment of value="2" from the web UI's HTML — recycled memory.
Every corrupted-topic publish is followed immediately by a disconnect, which is
the malformed packet the broker reports.

Moving the release below the result check keeps a failed item queued.

2. Off-by-one in the queue length guard

char topic[MQTT_PUBLISH_ITEM_TOPIC_LENGTH];      // 64
char channel[MQTT_PUBLISH_ITEM_CHANNEL_LENGTH];  // 128
char value[MQTT_PUBLISH_ITEM_VALUE_LENGTH];      // 1512

if ((strlen(topic) > MQTT_PUBLISH_ITEM_TOPIC_LENGTH) || ...) return;
strcpy(newItem->topic, topic);

strcpy writes strlen + 1 bytes, so a string of exactly the buffer length
passes > and then overflows by its terminator into the following field.
Changed to >=.

Relationship to #2222

I don't think #2222 introduced either bug — both look long-standing. What it
changed is that OBK_PUBLISH_MEM_FAIL now leaves items pending instead of
dropping them, so the queue holds more live entries for longer and slots are
recycled under pressure far more often. That appears to be enough to turn a
latent race into something that fires on every discovery publish. 1.18.310 is
clean on the same hardware, broker and config; 1.18.311 is not.

Trade-off worth a maintainer's opinion

With this change, an item that can never be published — for example a payload
larger than MQTT_OUTPUT_RINGBUF_SIZE, so mqtt_output_check_space() always
fails — will now stay at the head of the queue instead of being silently
discarded. That is correct in the sense that nothing is corrupted, but it will
stall the queue rather than lose one message.

If you'd prefer, I'm happy to add a retry counter that drops an item after N
consecutive failures, or to log and drop specifically when the payload cannot
possibly fit. I left it out here to keep the change minimal and focused on the
memory-safety problem.

Relevant to Beken specifically: libraries/mqtt_patched.c sets
MQTT_OUTPUT_RINGBUF_SIZE 2048, but platforms/TXW81X/OpenBeken.mk is the only
makefile that compiles it, so BK7231T uses whatever the Beken SDK configures. If
that is lwip's 256-byte default, a ~1.4 KB discovery payload can never fit,
which would explain the steady ERR_MEM on these devices.

Testing

I have one BK7231T on 1.18.311 reproducing this on every discovery publish, and
an identical one on 1.18.310 as a control, so I can A/B any patch.

I have not been able to build or run this change — I don't have a Windows
toolchain for the simulator selftests. It needs CI and ideally a second pair of
eyes on the queue-stall trade-off above.

[codypendant]

Closing this too. The queue issues look real to me but they aren't what causes #2230, and #2234 fixes that. No point leaving an unrelated PR open on it.

If you want the queue stuff looked at I'll write it up properly as its own issue.