buglwIP - A Lightweight TCP/IP stack - Bugs: bug #65388, MQTT infintie loop if incoming...

 
 

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

bug #65388: MQTT infintie loop if incoming pbuf chained

Submitter:  Alexander <pronkin>
Submitted:  Fri 01 Mar 2024 08:38:15 AM UTC
   
 
Category:  apps Severity:  3 - Normal
Item Group:  None Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
lwIP version:  2.2.0

Fri 01 Mar 2024 08:38:15 AM UTC, original submission:  

in this commit the optimisation added in line 891

/* Adjust cpy_len to ensure zero-copy operation for remaining parts of current message */
if (client->msg_idx >= MQTT_VAR_HEADER_BUFFER_LEN) {
  if (cpy_len > (p->len - in_offset))
    cpy_len = p->len - in_offset;
}

this cause to infintie loop if p is chained (two or more pbufs), because when whe move on next pbuf
cpy_len become zero and we never processed more data. here is the log:

mqtt_parse_incoming: msg_idx: 9596, cpy_len: 124, remaining 598149
mqtt_parse_incoming: msg_idx: 9720, cpy_len: 124, remaining 598025
mqtt_parse_incoming: msg_idx: 9844, cpy_len: 124, remaining 597901
mqtt_parse_incoming: msg_idx: 9968, cpy_len: 124, remaining 597777
mqtt_parse_incoming: msg_idx: 10092, cpy_len: 124, remaining 597653
mqtt_parse_incoming: msg_idx: 10216, cpy_len: 124, remaining 597529
mqtt_parse_incoming: msg_idx: 10340, cpy_len: 124, remaining 597405
mqtt_parse_incoming: msg_idx: 10400, cpy_len: 60, remaining 597345
mqtt_parse_incoming: msg_idx: 10400, cpy_len: 0, remaining 597345
mqtt_parse_incoming: msg_idx: 10400, cpy_len: 0, remaining 597345
mqtt_parse_incoming: msg_idx: 10400, cpy_len: 0, remaining 597345
mqtt_parse_incoming: msg_idx: 10400, cpy_len: 0, remaining 597345
mqtt_parse_incoming: msg_idx: 10400, cpy_len: 0, remaining 597345
[inline loop]

Proper version of this must look like (same as check in pbuf_get_contiguous)

/* Adjust cpy_len to ensure zero-copy operation for remaining parts of current message */
q = pbuf_skip(p, in_offset, &out_offset);
  if (q->len < (out_offset + cpy_len)) {
    cpy_len = q->len - out_offset;
}

with this fix, I successfully received large files without fails.

The next is optimization part (my proposals)

Since zero copy always perform so pbuf_get_contiguous never use our rx_buffer

var_hdr_payload = (u8_t*)pbuf_get_contiguous(p, client->rx_buffer + fixed_hdr_len,
                                                buffer_space, cpy_len, in_offset);


it may be replaced with

var_hdr_payload = (u8_t*)pbuf_get_contiguous(p, NULL, 0, cpy_len, in_offset);

More over, since our rx_buffer is not used, it is not necessary to limit cpy_len to available space in buffer

/* Limit to available space in buffer */
buffer_space = MQTT_VAR_HEADER_BUFFER_LEN - fixed_hdr_len;
if (cpy_len > buffer_space) {
   cpy_len = buffer_space;
}

this leads to the following that bigger parts of data process directly from pbufs in mqtt_message_received and user app (not by peaces).

However there is a LWIP_ERROR in mqtt_message_received that check data fits in rx_buffer

LWIP_ERROR("buffer length mismatch", fixed_hdr_len + length <= MQTT_VAR_HEADER_BUFFER_LEN,
             return MQTT_CONNECT_DISCONNECTED);

I found out that no problems occur to process bigger data.
Also docs mention that app callbacks data valid only before callback returns without limiting its size.
So I deleted this check and ran some tests.

Everything works ok. I received larger peaces of data in app callback
and overall download time of large file (~600K) decreased from 5min to 1min.

Alexander <pronkin>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by pronkin (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.

     

    No changes have been made to this item

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code