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)

Submitted by:  Amir Shalem <amirshalem>
Submitted on:  Fri 13 Jan 2012 05:15:51 PM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by amirshalem (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 22 Jan 2012 10:20:06 AM UTCgoldsimonOpen/ClosedOpen=>Closed
    Sun 22 Jan 2012 10:20:05 AM UTCgoldsimonStatusIn Progress=>Fixed
    Sat 21 Jan 2012 06:06:20 PM UTCgoldsimonStatusNone=>In Progress
      Assigned toNone=>goldsimon
      SummaryToo early free of TCP pcb while closing the socket=>pcb may be freed too early on shutdown(WR)
    Fri 20 Jan 2012 06:39:39 AM UTCgoldsimonPlanned Release=>1.4.1

    Back to the top


    Powered by Savane 3.1-cleanup1