espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
611 stars 257 forks source link

Publish data leaking to topic (IDFGH-9290) #247

Closed DEADSEC-SECURITY closed 1 year ago

DEADSEC-SECURITY commented 1 year ago

Whats the problem?

After receiving the event to process the received data I check if the topic is equal to different topics I have to check what function should process it. The problem is that the topic being passed in the event struct has the data appended to it as you can see from the image bellow:

The topic: homeassistant/select/grandmother_pill_dispenser_volume/set The data: High

image

Note: The image has 3 values split by -. The first is the topic received event->topic, the second is the length of the string strlen(event->topic) and the last one is the topic len passed in the variable event->topic_len

As you can see the the topic len is 58 but the char length is actually topic + data which seems to be like data is being appended to the topic or some memory leak is happening.

I went a bit deeper to the mqtt_client.c where the object is created and adding a log to see the data the same thing happens so seems to be a problem with the lib and not my implementation.

image

Commit used: dffabb067f ESP-IDF version 5.

DEADSEC-SECURITY commented 1 year ago

Also after logging the contents of data I get the correct data if the data is "High" but if I send "Low" the log outputs Lowâ”” or Lowh, basically Low with some random chars. (Maybe uninitialized memory?)

DEADSEC-SECURITY commented 1 year ago

Some EMQX logs in case it helps visualize the data being sent:

2023-02-01T12:45:08+00:00 [PUBLISH] 3s31eT307G2DbsDJXJjKCp@10.1.10.7:33767 msg: publish_to, topic: homeassistant/select/grandmother_pill_dispenser_volume/set, payload: Low
2023-02-01T12:48:49+00:00 [SUBSCRIBE] Grandmother Pill Dispenser@10.4.81.207:58148 msg: subscribe, sub_id: Grandmother Pill Dispenser, sub_opts: [nl: 0, qos: 0, rap: 0, rh: 0, sub_props: []], topic: homeassistant/select/grandmother_pill_dispenser_volume/set
2023-02-01T12:48:53+00:00 [SUBSCRIBE] Grandmother Pill Dispenser@10.4.81.207:55043 msg: subscribe, sub_id: Grandmother Pill Dispenser, sub_opts: [nl: 0, qos: 0, rap: 0, rh: 0, sub_props: []], topic: homeassistant/select/grandmother_pill_dispenser_volume/set
ESP-YJM commented 1 year ago

@DEADSEC-SECURITY I think you can't use %s to print msg_topic, you can use following code to print. msg_topic is not end of \0, so you can't use strlen to calcaute its length.

ESP_LOGI(TAG, "Topic is %.*s", msg_topic_len, msg_topic);

You also can refer to example https://github.com/espressif/esp-idf/blob/master/examples/protocols/mqtt/tcp/main/app_main.c#L91

euripedesrocha commented 1 year ago

Hi @DEADSEC-SECURITY as pointed by @ESP-YJM msg_topic isn't null terminated and part of the same buffer. You need to take the size in consideration when handling them, for data if the size is bigger than the internal buffer multiple events are dispatched.

DEADSEC-SECURITY commented 1 year ago

I didnt have time to test this out cuz another bug appeared but once thats fixed ill check this out. Thanks @euripedesrocha and @ESP-YJM