buglwIP - A Lightweight TCP/IP stack - Bugs: bug #41318, Bad memory ref in tcp_input()...

 
 

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

bug #41318: Bad memory ref in tcp_input() after tcp_close()

Submitter:  Per Ekman <perekman>
Submitted:  Thu 23 Jan 2014 10:17:28 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.4.1

Sat 21 Mar 2015 08:19:48 PM UTC, comment #3: 

Re last comment:
The way the patch does it is not a good one since the next timer event can be too late when short in pcbs.

I've fixed it by letting tcp_close_shutdown check for tcp_input_pcb and delaying deallocation via TF_CLOSED.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 13 Feb 2014 07:55:08 AM UTC, comment #2: 

Or tcp_close() shouldn't do the equivalent of an abort
and instead postpone it like the patch attempts to do,
keeping the API and documentation intact.
Possibly?

Per Ekman <perekman>
Fri 07 Feb 2014 01:01:23 PM UTC, comment #1: 

Relevant comment on lwip-devel on 2014-01-24:
"If callback causes RST, then ERR_ABRT must be returned from the
callback and the pcb considered dead.  This is documented for
tcp_abort(), but tcp_close() as currently implemented may effectively
do tcp_abort() internally.  The fact that tcp_close() doesn't really
tell the caller whether it has done RST or not can be considered as an
API bug.

I'm not at the moment prepared to meaningfully pursue this line of
inquiry, though.  Too much of RTFS context is unloaded."

Simon Goldschmidt <goldsimon>
Group administrator
Thu 23 Jan 2014 10:17:28 AM UTC, original submission:  

Hello,

I've observed the following with the rawapi in lwip 1.4.1 :

- tcp_input():356 calls the application receive callback on a pcb
which is in CLOSE_WAIT.

- The application receive callback calls tcp_close()

- tcp_close_shutdown() sees that the pcb is in CLOSE_WAIT and that
  pcb->rcv_wnd != TCP_WND which causes it to free the pcb
  and return ERR_OK

- The application receive callback returns ERR_OK to tcp_input()
  which proceeds to dereference the freed pcb and passes it to
  tcp_output() (tcp_input():386)

It seems like a bug in tcp_close(). It frees the pcb if the pcb is in
CLOSE_WAIT since no RST needs to be sent but this breaks if
tcp_close() is called from the RX callback since the lwip code
invoking the RX callback assumes that the pcb is still valid after the
RX callback has returned (and the RX callback has no way of knowing
that the pcb was freed).

The following fix solved the immediate issue for me.

Sincerely
Per Ekman
H&D Wireless AB


>From 147c37d9d43a496669092c1fc2febe7aaf5fe3df Mon Sep 17 00:00:00 2001

From: Per Ekman <address@hidden>
Date: Fri, 5 Jul 2013 12:52:32 +0200
Subject: [PATCH 1/1] Attempt to fix invalid pcb free in tcp_close().

---
 src/core/tcp.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/core/tcp.c b/src/core/tcp.c
index 8690cd2..b4bd896 100644
--- a/src/core/tcp.c
+++ b/src/core/tcp.c
@@ -185,15 +185,14 @@ tcp_close_shutdown(struct tcp_pcb *pcb, u8_t
rst_on_unacked_data)
       tcp_rst(pcb->snd_nxt, pcb->rcv_nxt, &pcb->local_ip, &pcb->remote_ip,
                pcb->local_port, pcb->remote_port, PCB_ISIPV6(pcb));
 
+      if (pcb->state == CLOSE_WAIT)
+              return ERR_OK;
       tcp_pcb_purge(pcb);
       TCP_RMV_ACTIVE(pcb);
       if (pcb->state == ESTABLISHED) {
         /* move to TIME_WAIT since we close actively */
         pcb->state = TIME_WAIT;
         TCP_REG(&tcp_tw_pcbs, pcb);
-      } else {
-        /* CLOSE_WAIT: deallocate the pcb since we already sent a RST for it */
-        memp_free(MEMP_TCP_PCB, pcb);
       }
       return ERR_OK;
     }
@@ -913,6 +912,12 @@ tcp_slowtmr_start:
         }
       }
     }
+    if (pcb->state == CLOSE_WAIT) {
+      if (pcb->flags & TF_RXCLOSED) {
+          ++pcb_remove;
+          LWIP_DEBUGF(TCP_DEBUG, ("tcp_slowtmr: removing pcb in
CLOSE_WAIT\n"));
+      }
+    }
     /* Check if this PCB has stayed too long in FIN-WAIT-2 */
     if (pcb->state == FIN_WAIT_2) {
       /* If this PCB is in FIN_WAIT_2 because of SHUT_WR don't let it time
out. */
--
1.7.0.4

Per Ekman <perekman>

 

(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 goldsimon (Posted a comment)
  • -email is unavailable- added by perekman (Submitted the item)
  • -email is unavailable- added by perekman
  •  

    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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-03-21 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2014-01-23 perekman Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code