[lwip-devel] [bug #65388] MQTT infintie loop if incoming pbuf chained

[Alexander]

URL:
  <https://savannah.nongnu.org/bugs/?65388>

                 Summary: MQTT infintie loop if incoming pbuf chained
                   Group: lwIP - A Lightweight TCP/IP stack
               Submitter: pronkin
               Submitted: Пт 01 мар 2024 08:38:15
                Category: apps
                Severity: 3 - Normal
              Item Group: None
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None
            lwIP version: 2.2.0


    _______________________________________________________

Follow-up Comments:


-------------------------------------------------------
Date: Пт 01 мар 2024 08:38:15   By: Alexander <pronkin>
in
[https://git.savannah.nongnu.org/cgit/lwip.git/commit/src/apps/mqtt/mqtt.c?id=df0699c143842e656176dfe65d89e183e697ef53
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.








    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?65388>

_______________________________________________
Сообщение отправлено по Savannah
https://savannah.nongnu.org/