buglwIP - A Lightweight TCP/IP stack - Bugs: bug #35305, pcb may be freed too early on...

 
 

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

bug #35305: pcb may be freed too early on shutdown(WR)

Submitter:  Amir Shalem <amirshalem>
Submitted:  Fri 13 Jan 2012 05:15:51 PM 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:  CVS Head

Jump to the original submission

Sun 22 Jan 2012 10:20:06 AM UTC, comment #6: 

Fixed by adding "TCP_EVENT_ERR(pcb->errf, pcb->callback_arg, ERR_CLSD)" only "if (!(pcb->flags & TF_RXCLOSED))", i.e. if the application has shut down the TX side but not the RX side.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 21 Jan 2012 06:06:20 PM UTC, comment #5: 

Updated comment to better reflect the bug (it's a bug in shutdown, not in closing).

Simon Goldschmidt <goldsimon>
Group administrator
Fri 20 Jan 2012 09:49:00 PM UTC, comment #4: 

I like the idea of calling TCP_EVENT_ERR with ERR_CLSD: we don't have a defined way of handling SHUT_RDWR at raw API level, yet, and it would be the best way of integrating it into the netconn/socket API.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 20 Jan 2012 06:39:39 AM UTC, comment #3: 

Targeting at 1.4.1 since I want shutdown to be fixed there.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 14 Jan 2012 07:30:43 AM UTC, comment #2: 

1.
Correct.
If the application had called close() instead of a shutdown(SHUT_WR) + close()
The bug condition wouldn't be possible.
Because that's the only way to reach pcb->state == LAST_ACK + recv_flags == TF_CLOSED
And that's the condition where the bug is in tcp_input()

2.
api_msg conn == 'conn->pcb.tcp'
as long as a `conn`ection reference to a pcb, we can't memp_free() it.
which is the root of the bug.
and that's why I suggest adding a single
TCP_EVENT_ERR(pcb->errf, pcb->callback_arg, ERR_CLSD);
to fix this bug condition.


Amir Shalem <amirshalem>
Fri 13 Jan 2012 07:36:32 PM UTC, comment #1: 

If the application called close() instead of shutdown() would this problem still be there?
I'm wondering if it is just another weakness in our implementation of shutdown()

I'm a bit lost in your description; can you give more detail about the "api_msg conn" that still references the connection?

Kieran Mansley <kieranm>
Group Member
Fri 13 Jan 2012 05:15:51 PM UTC, original submission:  

Hey
I found an early free condition of a TCP pcb while the connection is closing, and the netconn is still referencing the PCB.

- Assume pcb->state = ESTABLISHED.

1. Other side sends us FIN.
2. Our pcb moves to state = CLOSE_WAIT.
3. Call TCP_EVENT_CLOSED(pcb, err);

- api_msg gets an empty tcp_recv(), and passes the application layer an empty recv()
- The application realize the connection is dead, and calls a shutdown(SHUT_WR), and afterwards a close()

4. Our application calls shutdown(SHUT_WR)
5. Causes a do_close_internal(SHUT_WR) --> tcp_shutdown(pcb, shut_tx=1) --> tcp_close_shutdown(pcb, 0)
6. Which changes our state to LAST_ACK (we are in CLOSE_WAIT).
7. Sends a fin to the other side as-well.

- ACK arrives to our FIN.

8. tcp_process() handles it, and sets TF_CLOSED.
9. tcp_input() handles the TF_CLOSED, and free our PCB - this is the first free.

But the api_msg conn is still referencing the PCB, which can cause all sorts of problems.

Usually, if no new socket is created nothing bad happens.
Because if the application closes the socket --> tcp_close_shutdown() --> state=LAST_ACK and nothing bad happens.

But, if a new connection arrives between the application close()
to the "ACK for our fin", there will a double-reference for this PCB --> hell.

I'm not sure what is the fix for this bug, maybe adding a

      } else if (recv_flags & TF_CLOSED) {
        /* The connection has been closed and we will deallocate the
           PCB. */
+ TCP_EVENT_ERR(pcb->errf, pcb->callback_arg, ERR_CLSD);
        tcp_pcb_remove(&tcp_active_pcbs, pcb);
        memp_free(MEMP_TCP_PCB, pcb);
      } else {

while handling the TF_CLOSED? (same way of handling TF_RESET)

What do you think?

Amir.

Amir Shalem <amirshalem>

 

(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 kieranm (Posted a comment)
  • -email is unavailable- added by amirshalem (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2012-01-22 goldsimon Open/ClosedOpen Closed
    2012-01-22 goldsimon StatusIn Progress Fixed
    2012-01-21 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
        SummaryToo early free of TCP pcb while closing the socket pcb may be freed too early on shutdown(WR)
    2012-01-20 goldsimon Planned Release 1.4.1

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code