buglwIP - A Lightweight TCP/IP stack - Bugs: bug #57734, MQTT broken with muti-segments data

 
 

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

bug #57734: MQTT broken with muti-segments data

Submitter:  David Bourgeois <jaguarondi>
Submitted:  Mon 03 Feb 2020 09:33:19 PM UTC
   
 
Category:  apps Severity:  3 - Normal
Item Group:  Crash Error Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
lwIP version:  git head

Jump to the original submission

Fri 06 Oct 2023 09:42:49 PM UTC, comment #8: 

Ran into this issue recently. mqtt.patch is my take on the fix: new message parser, zero copy in most cases and using 128 bytes less RAM.

Deomid Ryabkov <rojer>
Sat 16 May 2020 01:29:33 PM UTC, comment #7: 

When data are segmented, mqtt_message_receive() (mqtt.c line 913 on master - 3f47b04f16a874c05def88dcda6b3e0a2d6684ce) fails. It returns -1 because it is a short PUBLISH packet (mqtt.c line 729).

mqtt_parse_incoming don't support segmented data.

mqtt_message_received should probably not be called if msg_rem_len != 0. Also, in case of segmented data, the length must be adapted when mqtt_message_received is called.
It was the purpose of cpy_start up to the commit 2cc420e434744f9f217f2e68f39cf08379517a94 it seems. Segmented data was OK until then.

Also, commit df0699c143842e656176dfe65d89e183e697ef53 introduced another problem by calling mqtt_message_received with incomplete data in the case of multiple segments.

Paul RATHGEB <ks156>
Sun 16 Feb 2020 08:53:10 PM UTC, comment #6: 

The topic is null-terminated, see mqtt.c line 755. So I think we should keep the last null byte in the test for the topic. I also tried without without checking the last nul byte and it also fails so the problem isn't there.

For the payload, I indeed didn't include the '\0' in the payload (rx_data) so you're right, this is corrected now in the new patch file.

(file #48422)

David Bourgeois <jaguarondi>
Sun 16 Feb 2020 07:58:09 PM UTC, comment #5: 

For me (testing on windows), the tests fail in test_1_mqtt_incoming_data_cb() called from line 213.

To me it looks like the test check is wrong there: you should not use sizeof(test_message) as that includes the trailing 0 (same goes for test_topic some lines above).

Simon Goldschmidt <goldsimon>
Group administrator
Sun 16 Feb 2020 03:48:57 PM UTC, comment #4: 

I could reproduce the problem, it comes when the topic ends-up being spread over 2 TCP segments. So I added 3 unit tests in the patch attached, a standard single segment incoming publish, and 2 segmented either in the topic or message.

The tests do pass with commit #ed561a5 :


Running suite(s): IPv4

  < Receive publish
    < Topic: "topic" (message len: 62)
    < Data: "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" (len: 62, rem: 0)
> Receive segmented topic
    < Topic: "topic" (message len: 62)
    < Data: "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" (len: 62, rem: 0)
> Receive segmented message
    < Topic: "topic" (message len: 62)
    < Data: "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" (len: 62, rem: 0)
 SOCKETS
100%: Checks: 112, Failures: 0, Errors: 0


- They fail with master:

  < Receive publish
    < Topic: "topic" (message len: 62)
    < Data: "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" (len: 62, rem: 0)
> Receive segmented topic
 SOCKETS
99%: Checks: 112, Failures: 0, Errors: 1
../../../../src/../test/unit/mqtt/test_mqtt.c:317:E:MQTT:segmented_sub_pub:0: (after this point) Received signal 11 (Segmentation fault)



(file #48421)

David Bourgeois <jaguarondi>
Wed 05 Feb 2020 07:09:47 PM UTC, comment #3: 


> What could I use as an example to reproduce this use case?


The best way is to provide a unit test that shows the bug. That way, we can ensure it won't happen again.

See test/unit/mqtt/test_mqtt.c

Simon Goldschmidt <goldsimon>
Group administrator
Wed 05 Feb 2020 08:36:13 AM UTC, comment #2: 

comment #1:

> I can't find the code you mention in git head. Could you send a proposed patch of how to fix this?


That has been changed in commit #2cc420e, at the bottom of this page: #2cc420e


-      pbuf_copy_partial(p, client->rx_buffer + cpy_start, cpy_len, in_offset);
+      pbuf_copy_partial(p, client->rx_buffer + fixed_hdr_len, cpy_len, in_offset);


I have not been able to grasp all the inner workings to patch it and there are multiple changes after this commit for the zero-copy part. So it might be better if I can reproduce the problem in master with a simple example.

What could I use as an example to reproduce this use case? We're using STM32 but the platform is irrelevant, so I better try with what are you using for testing.

David Bourgeois <jaguarondi>
Tue 04 Feb 2020 09:14:06 PM UTC, comment #1: 

I can't find the code you mention in git head. Could you send a proposed patch of how to fix this?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 03 Feb 2020 09:33:19 PM UTC, original submission:  

I have a "Socket error on client..." since commit #2cc420e linked to bug #55607

I found that when I subscribe to a "topic/#" that has a few retained topics/values, the TCP packets are split into multiple segments that each hold multiple MQTT topic/payload.

When the end of the segment contains the beginning of a topic (say 10 chars), then the next segment get's the remaining of the topic and should write it on rx_buffer[10], which was the role of cpy_start and that offset was applied in


pbuf_copy_partial(p, client->rx_buffer + cpy_start, cpy_len, in_offset);


The changes seems to remove this:

-      pbuf_copy_partial(p, client->rx_buffer + cpy_start, cpy_len, in_offset);
+      pbuf_copy_partial(p, client->rx_buffer + fixed_hdr_len, cpy_len, in_offset);


I compared cpy_start with fixed_hdr_len and when the problem appears, cpy_start = 10 and fixed_hdr_len = 2 instead of 10.

I guess the problem can be reproduced by publishing various topics/subtopic with long payload, then subscribing to topic/#.

David Bourgeois <jaguarondi>

 

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

Attached Files
file #55211:  mqtt.patch added by rojer (28KiB - text/x-patch - I ran into this issue as well. I tried to fix existing code but it made my head spin.)
file #48421:  0001-Add-3-tests-of-incoming-publish.patch added by jaguarondi (10KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rojer (Updated the item)
  • -email is unavailable- added by ks156 (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jaguarondi (Submitted the item)
  •  

    There are 0 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.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-10-06 rojer Attached File- Added mqtt.patch, #55211
    2020-02-16 jaguarondi Attached File- Added 0001-Add-3-tests-of-incoming-publish_v2.patch, #48422
    2020-02-16 jaguarondi Attached File- Added 0001-Add-3-tests-of-incoming-publish.patch, #48421

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code