buglwIP - A Lightweight TCP/IP stack - Bugs: bug #61666, A memory leak BUG in function...

 
 

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

bug #61666: A memory leak BUG in function tcp_input().

Submitter:  xuyao hong <shellstudio>
Submitted:  Mon 13 Dec 2021 10:59:37 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
lwIP version:  2.1.3

Jump to the original submission

Mon 16 Jan 2023 02:05:48 PM UTC, comment #8: 

comment #6:

> The current proposed solution lacks an "else" if (TCP_QUEUE_OOSEQ && LWIP_WND_SCALE) is true. I've attached a revised .diff file, Could somebody check it?
>
> (file #53000)


I have also an issue with memory leak for tcp_input().
I have applied the patch, however unfortunately there is no change.

I am happy to debug the issue. Just let me know how-to.

Thanks, Arjan

Arjan van Vught <arjan>
Fri 18 Mar 2022 02:50:17 PM UTC, comment #7: 


comment #1:

> tcp_appcode.c
>
> static err_t app_cb_accept(void* arg, struct tcp_pcb* pcb, err_t err)
> {
>   ...
>   tcp_sent(pcb, app_cb_sent);
>   ...
> }
> static err_t app_cb_sent(void* arg, struct tcp_pcb* pcb, u16_t len)
> {
>   ...
>   tcp_close(pcb);
>   ...
>   return ERR_OK;
> }
>
> tcp_in.c
> 当tcp_input()接收到同时包含PSH+ACK标志的TCP报文,那么将会执行以下流程:
> tcp_input(struct pbuf *p)
> {
>   if (pcb != NULL) {
>     ...
>     inseg.p = p;
>     recv_data = NULL;
>     recv_acked = 0;
>
>     ...
>     tcp_process(pcb);
>     if (err != ERR_ABRT) {
>       if (recv_flags & TF_RESET) {
>         ...
>       } else {
>         ...
>         if (recv_acked > 0) {
>           ...
>           // 到这里,大概率是recv_data==p,并且inseg.p==NULL。
>           TCP_EVENT_SENT(pcb, (u16_t)acked16, err); /* 回调app_cb_sent() */
>           // 只要在app_cb_sent()中执行下面其中一种逻辑,即可触发pbuf泄露:
>           // [1] call tcp_close() and return ERR_OK,
>           // [2] call tcp_abort() and return ERR_ABRT.
>           if (err == ERR_ABRT) {
>             // 此时如果(recv_data != NULL)那么p泄露。
>             goto aborted; // ERROR POINT (1)
>           }
>           ...
>         }
>         if (tcp_input_delayed_close(pcb)) {
>           // 此时如果(recv_data != NULL)那么p泄露。
>           goto aborted;   // ERROR POINT (2)
>         }
>         ...
>       }
>     }
> aborted:
>     tcp_input_pcb = NULL;
>     recv_data = NULL;
>     if (inseg.p != NULL) {
>       pbuf_free(inseg.p);
>       inseg.p = NULL;
>     }
>   }
>   ...
> }
>
>
>

xuyao hong <shellstudio>
Fri 18 Mar 2022 02:16:57 PM UTC, comment #6: 

The current proposed solution lacks an "else" if (TCP_QUEUE_OOSEQ && LWIP_WND_SCALE) is true. I've attached a revised .diff file, Could somebody check it?

(file #53000)

Enrico Murador <hen>
Fri 18 Mar 2022 09:43:53 AM UTC, comment #5: 

I am getting the same issue. I am working with httpd server.

My scenario: after httpd finish sending data in response to a PUSH request from client, tcp_input() receives a packet (the first part of a GET request) for the same pcb that is handling the PUSH data.
tcp_input() calls TCP_EVENT_SENT callback (http_sent), that calls http_send(), then http_check_eof(), that calls http_eof() because it founds the send is done.
http_eof() closes the connection (call sequence: http_close_conn->http_close_or_abort_conn->tcp_close->tcp_close_shutdown).
tcp_close_shutdown calls tcp_rst() because application has still to receive the data (TCP_EVENT_RECV callback has yet to be called, code is after the call to TCP_EVENT_SENT callback.
After the TCP_EVENT_SENT callback return, tcp_input() calls tcp_input_delayed_close() that returns 1 and code jumps to "aborted".

Now, inseg.p is NULL, probably (I have yet to verify it, but it seems the only possibility) because before the call to TCP_EVENT_SENT callback, tcp_input() calls tcp_process() that calls tcp_receive(), that passes the inseg.p pointer to recv_data and NULLs inseg.p (code comment is: "Since this pbuf now is the responsibility of the application, we delete our reference to it so that we won't (mistakingly) deallocate it").

So back in tcp_input(), the code returns after taking the "aborted" route without freeing the pbuf that should be still in charge of tcp, because TCP_EVENT_RECV callback has not been called.

Enrico Murador <hen>
Mon 13 Dec 2021 12:40:05 PM UTC, comment #4: 

If TCP application code like this, very similar to httpd example:

static err_t app_cb_accept(void* arg, struct tcp_pcb* pcb, err_t err)
{
  ...
  tcp_sent(pcb, app_cb_sent);
  ...
}

static err_t app_cb_sent(void* arg, struct tcp_pcb* pcb, u16_t len)
{
  ...
  tcp_close(pcb);
  ...
  return ERR_OK;
}

Now, if tcp_input(struct pbuf *p) received a packet has PSH+ACK flags, the flow like below:

tcp_input(struct pbuf *p)
{
  if (pcb != NULL) {
    ...
    inseg.p = p;
    recv_data = NULL;
    recv_acked = 0;
    ...
    tcp_process(pcb);
    if (err != ERR_ABRT) {
      if (recv_flags & TF_RESET) {
        ...
      } else {
        ...
        if (recv_acked > 0) {
          ...
          // here, recv_data mybe p,and inseg.p is NULL.
          TCP_EVENT_SENT(pcb, (u16_t)acked16, err); /* call app_cb_sent() */
          // if in function app_cb_sent() do any thing like below:
          // [1] call tcp_close() and return ERR_OK,
          // [2] call tcp_abort() and return ERR_ABRT.
          if (err == ERR_ABRT) {
            // need to free recv_data if (recv_data != NULL)
            goto aborted; // ERROR POINT (1)
          }
          ...
        }
        if (tcp_input_delayed_close(pcb)) {
          // need to free recv_data if (recv_data != NULL)
          goto aborted;   // ERROR POINT (2)
        }
        ...
      }
    }
aborted:
    tcp_input_pcb = NULL;
    recv_data = NULL;
    if (inseg.p != NULL) {
      pbuf_free(inseg.p);
      inseg.p = NULL;
    }
  }
  ...
}

My final solution:
tcp_input(struct pbuf *p)
{
...
        if (recv_acked > 0) {
          u16_t acked16;
#if LWIP_WND_SCALE
          /* recv_acked is u32_t but the sent callback only takes a u16_t,
             so we might have to call it multiple times. */
          u32_t acked = recv_acked;
          while (acked > 0) {
            acked16 = (u16_t)LWIP_MIN(acked, 0xffffu);
            acked -= acked16;
#else
          {
            acked16 = recv_acked;
#endif
            TCP_EVENT_SENT(pcb, (u16_t)acked16, err);
            if (err == ERR_ABRT) {
              // need to free recv_data if (recv_data != NULL)
              if (recv_data != NULL) {
                pbuf_free(recv_data);
              }
              goto aborted;
            }
          }
          recv_acked = 0;
        }
        if (tcp_input_delayed_close(pcb)) {
          // need to free recv_data if (recv_data != NULL)
          if (recv_data != NULL) {
            pbuf_free(recv_data);
          }
          goto aborted;
        }
...
}

xuyao hong <shellstudio>
Mon 13 Dec 2021 12:26:54 PM UTC, comment #3: 

Sorry, but I think most of the developers hear can't read chinese. Would you mind doing this in english, please?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 13 Dec 2021 12:03:19 PM UTC, comment #2: 

//最终修复办法:
...

        if (recv_acked > 0) {
          u16_t acked16;
#if LWIP_WND_SCALE
          /* recv_acked is u32_t but the sent callback only takes a u16_t,
             so we might have to call it multiple times. */
          u32_t acked = recv_acked;
          while (acked > 0) {
            acked16 = (u16_t)LWIP_MIN(acked, 0xffffu);
            acked -= acked16;
#else
          {
            acked16 = recv_acked;
#endif
            TCP_EVENT_SENT(pcb, (u16_t)acked16, err);
            if (err == ERR_ABRT) {
              /* 若recv_data不为NULL则需要释放pbuf */
              if (recv_data != NULL) {
                pbuf_free(recv_data);
              }
              goto aborted;
            }
          }
          recv_acked = 0;
        }
        if (tcp_input_delayed_close(pcb)) {
          /* 若recv_data不为NULL则需要释放pbuf */
          if (recv_data != NULL) {
            pbuf_free(recv_data);
          }
          goto aborted;
        }

...

xuyao hong <shellstudio>
Mon 13 Dec 2021 11:02:35 AM UTC, comment #1: 

tcp_appcode.c

static err_t app_cb_accept(void* arg, struct tcp_pcb* pcb, err_t err)
{
  ...
  tcp_sent(pcb, app_cb_sent);
  ...
}
static err_t app_cb_sent(void* arg, struct tcp_pcb* pcb, u16_t len)
{
  ...
  tcp_close(pcb);
  ...
  return ERR_OK;
}

tcp_in.c
当tcp_input()接收到同时包含PSH+ACK标志的TCP报文,那么将会执行以下流程:
tcp_input(struct pbuf *p)
{
  if (pcb != NULL) {
    ...
    inseg.p = p;
    recv_data = NULL;
    recv_acked = 0;

    ...
    tcp_process(pcb);
    if (err != ERR_ABRT) {
      if (recv_flags & TF_RESET) {
        ...
      } else {
        ...
        if (recv_acked > 0) {
          ...
          // 到这里,大概率是recv_data==p,并且inseg.p==NULL。
          TCP_EVENT_SENT(pcb, (u16_t)acked16, err); /* 回调app_cb_sent() */
          // 只要在app_cb_sent()中执行下面其中一种逻辑,即可触发pbuf泄露:
          // [1] call tcp_close() and return ERR_OK,
          // [2] call tcp_abort() and return ERR_ABRT.
          if (err == ERR_ABRT) {
            // 此时如果(recv_data != NULL)那么p泄露。
            goto aborted; // ERROR POINT (1)
          }
          ...
        }
        if (tcp_input_delayed_close(pcb)) {
          // 此时如果(recv_data != NULL)那么p泄露。
          goto aborted;   // ERROR POINT (2)
        }
        ...
      }
    }
aborted:
    tcp_input_pcb = NULL;
    recv_data = NULL;
    if (inseg.p != NULL) {
      pbuf_free(inseg.p);
      inseg.p = NULL;
    }
  }
  ...
}



xuyao hong <shellstudio>
Mon 13 Dec 2021 10:59:37 AM UTC, original submission:  

User's TCP application like this:

static err_t app_cb_accept(void* arg, struct tcp_pcb* pcb, err_t err)
{
  ...
  tcp_sent(pcb, app_cb_sent);
  ...
}

static err_t app_cb_sent(void* arg, struct tcp_pcb* pcb, u16_t len)
{
  ...
  tcp_close(pcb);
  ...
  return ERR_OK;
}

xuyao hong <shellstudio>

 

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

Attached Files
file #53001:  report.htm added by shellstudio (1MiB - text/html - fix bug #61666)
file #53000:  tcp_in_patch20220318_bug61666.diff added by hen (3KiB - application/octet-stream)
file #52496:  tcp_in.c.v2.1.3.diff added by shellstudio (2KiB - application/octet-stream)
file #52497:  分析.c added by shellstudio (2KiB - text/plain)
file #52498:  tcp_in.new.c added by shellstudio (84KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by arjan (Posted a comment)
  • -email is unavailable- added by hen (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by shellstudio (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-03-18 shellstudio Attached File- Added report.htm, #53001
    2022-03-18 hen Attached File- Added tcp_in_patch20220318_bug61666.diff, #53000
    2021-12-13 shellstudio Attached File- Added tcp_in.c.v2.1.3.diff, #52496
        Attached File- Added 分析.c, #52497
        Attached File- Added tcp_in.new.c, #52498

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code