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
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       No canned response available

 

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>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

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
  •  

    There are 100 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    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.9