buglwIP - A Lightweight TCP/IP stack - Bugs: bug #59313, Possible faulty behavoir in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #59313: Possible faulty behavoir in mqtt_close() function

Submitted by:  Ondrej Lufinka <ondrej_lufinka>
Submitted on:  Wed 21 Oct 2020 10:52:37 AM UTC  
Votes:  100  
 
Category:  Platform ports Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
lwIP version:  git head

Fri 15 Oct 2021 07:13:06 AM UTC, comment #4: 

Patches fixing this bug for branches git master and git STABLE-2_1_x are now available in patch #10122.

Christian Schlotter <schlotter>
Wed 06 Oct 2021 01:42:40 PM UTC, comment #3: 

Please find attached a patch which applies to origin/STABLE-2_1_x. Please include in v2.1.3 as this fixes an important deviation from the MQTT spec.

If this is not the correct way to contribute, please let me know.

(file #52054)

Christian Schlotter <schlotter>
Tue 28 Sep 2021 01:16:27 PM UTC, comment #2: 

I can confirm Ondrej's code change works, but we would suggest adding the code to mqtt_disconnect() (instead of mqtt_close()):

diff --git a/Firmware/lwip-STABLE-2_1_2_RELEASE/src/apps/mqtt/mqtt.c b/Firmware/lwip-STABLE-2_1_2_RELEASE/src/apps/mqtt/mqtt.c
index 269f4a4..ce640b8 100644
--- a/Firmware/lwip-STABLE-2_1_2_RELEASE/src/apps/mqtt/mqtt.c
+++ b/Firmware/lwip-STABLE-2_1_2_RELEASE/src/apps/mqtt/mqtt.c
@@ -1440,6 +1440,10 @@ mqtt_disconnect(mqtt_client_t *client)
   LWIP_ASSERT("mqtt_disconnect: client != NULL", client);
   /* If connection in not already closed */
   if (client->conn_state != TCP_DISCONNECTED) {
+      if (mqtt_output_check_space(&client->output, 0) != 0) {
+          mqtt_output_append_fixed_header(&client->output, MQTT_MSG_TYPE_DISCONNECT, 0, 0, 0, 0);
+          mqtt_output_send(&client->output, client->conn);
+      }
     /* Set conn_state before calling mqtt_close to prevent callback from being called */
     client->conn_state = TCP_DISCONNECTED;
     mqtt_close(client, (mqtt_connection_status_t)0);

I think it would be important to include this in v2.1.3, as this is fixing an important deviation from the MQTT spec.

Christian Schlotter <schlotter>
Tue 28 Sep 2021 10:28:47 AM UTC, comment #1: 

I can confirm Ondrej's observation. The lwIP MQTT client does not properly disconnect using the MQTT DISCONNECT packet.

Christian Schlotter <schlotter>
Wed 21 Oct 2020 10:52:37 AM UTC, original submission:  

Hi,

I use LwIP on STM32 MCU. I have a working combination of LwIP + MbedTLS + MQTT. I found a possible faulty behavior in the function:

static void mqtt_close(mqtt_client_t *client, mqtt_connection_status_t reason)

According to the MQTT standard, a client should send the DISCONNECT (ID = 14) packet before closing the connection. This function terminates the connection directly without sending this packet. In my case, I connect to the Amazon AWS service, and this behavior led to this error:

1. I set Last Will Testament (LWT) before I call mqtt_connect()
2. I open the connection using mqtt_connect()
3. When the connection is lost (e.g. I unplug the ethernet cable from my device) - LWT is correctly published to AWS
4. But when I close the connection from code (calling mqtt_disconnect(), LWT should not be published
    - according to the MQTT specs http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc398718090 section 3.14.4
    - this is caused because of the missing DISCONNECT packet
5. When I added this code to the begging of the mqtt_close() function, it works OK:

   /* If we have memory -> send disconnect packet */
   if (mqtt_output_check_space(&client->output, 0) != 0) {

     /* Append fixed header for disconnect packet */
     mqtt_output_append_fixed_header(&client->output, MQTT_MSG_TYPE_DISCONNECT, 0, 0, 0, 0);

     /* Send it */
     mqtt_output_send(&client->output, client->conn);
   }

Can you please verify this behavior and possible solution?
Thank you very much.
Ondrej

Ondrej Lufinka <ondrej_lufinka>

 

Attached Files
file #52054:  0001-Fix-bug-59313-Possible-faulty-behavoir-in-mqtt_close.patch added by schlotter (1KiB - application/octet-stream - Patch for this bug)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by schlotter (Suggestion for inclusion into v2.1.3 (includes patch))
  • -email is unavailable- added by schlotter (Posted a comment)
  • -email is unavailable- added by schlotter (Voted in favor of this item)
  • -email is unavailable- added by ondrej_lufinka (Submitted the item)
  • -email is unavailable- added by ondrej_lufinka
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 100 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-10-06 schlotter Attached File- => Added 0001-Fix-bug-59313-Possible-faulty-behavoir-in-mqtt_close.patch, #52054
    2021-09-28 schlotter Carbon-Copy- => Added goldsimon
    2021-09-28 schlotter Carbon-Copy- => Added schlotter
    2020-10-21 ondrej_lufinka Carbon-Copy- => Added -email is unavailable-

    Back to the top


    Powered by Savane 3.7