buglwIP - A Lightweight TCP/IP stack - Bugs: bug #46696, accepts_pending not decreased when...

 
 

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

bug #46696: accepts_pending not decreased when TCP_EVENT_ACCEPT returns error

Submitter:  Joel Cunningham <jcunningham>
Submitted:  Wed 16 Dec 2015 03:37:22 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Fri 25 Mar 2016 04:13:17 PM UTC, comment #22: 

Fixed by implementing the flag and always calling tcp_backlog_accepted() from tcp_pcb_purge().

Simon Goldschmidt <goldsimon>
Group administrator
Fri 25 Mar 2016 09:41:59 AM UTC, comment #21: 


> All that should be needed is a flag in the PCB


I already had implemented it like that (that's why I had to do an additional "fix" commit): tcp_flagst_t gets u16_t always. I just thought it wasn't necessary, but now I see it is.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 24 Mar 2016 11:02:41 PM UTC, comment #20: 

Ambroz,

I agree we could also store some state in the child PCB that it's part of the current listener backlog count.  That would eliminate the need to maintain a linked list

Joel Cunningham <jcunningham>
Group Member
Thu 24 Mar 2016 05:14:53 PM UTC, comment #19: 

Hey,

I think that another linked list is unwarranted. All that should be needed is a flag in the PCB indicating if the PCB has an accepts_pending reference. Or use the "listener" pointer for this as I suggested (if it's not NULL, we have a reference), but rename it to something like backlog_ref_listener to avoid confusion.

Ambroz Bizjak <abizjak>
Thu 24 Mar 2016 03:12:47 PM UTC, comment #18: 

I also took a look at the changes and agree the inconsistent state situations seems to come about because the upper layers are currently required to perform a matching pair of tcp_backlog_delayed() and tcp_backlog_accepted() calls.  If the child PCB is freed internally before this happens and is in state ESTABLISHED, we have a leak in the listener.

The bulk of the problem seems to stem from not knowing if the child PCB (once in state ESTABLISHED) being freed is actually on the backlog or not.

One way I could think to solve this would be to maintain a linked list of child PBCs in the listener PCB in-addition to the count.  Inserts into the list would be O(1) by inserting at head, removals would be more expensive in we would have to traverse the list.  Operations to check the backlog size could use the count in order to avoid a traversal

This would then allow TCP internal to free PCBs without the matching tcp_backlog_accepted() call from the upper layers

Joel Cunningham <jcunningham>
Group Member
Thu 24 Mar 2016 07:33:29 AM UTC, comment #17: 

The internal backlog should be OK I think.

The external 'delayed/accepted' mechanism might really get inconsistent. I'll have to think about a fix. Setting 'listener' to NULL in 'accepted' doesn't seem right, as it is already used internally. Plus the listener pointer might (in future?) be used for other stuff...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 23 Mar 2016 09:47:40 PM UTC, comment #16: 

Ah, forget about the part regarding tcp_abandon(), I missed that it calls TCP_PCB_REMOVE_ACTIVE-->tcp_pcb_remove-->tcp_pcb_purge.

Ambroz Bizjak <abizjak>
Wed 23 Mar 2016 09:42:19 PM UTC, comment #15: 

Hello Simon,

I looked at your change and I think there is an issue with PCB errors as indicated by the tcp_err callback. If there is an error after the application has used tcp_backlog_delayed() (and before tcp_backlog_accepted()):

- It is generally unsafe to call tcp_backlog_accepted() from within the error callback, because the PCB memory may have been already deallocated (this is the case in 2 out of 4 possible error sources, just search for TCP_EVENT_ERR calls).

- The netconn code does not use tcp_backlog_delayed() in its error callback (err_tcp). This leads to a reference leak if there is an error while PCB is in the accept queue.

I think there is also a pre-existing reference leak of accept_pending in tcp_abandon(). This is triggered from tcp_abort(), tcp_alloc()/tcp_kill_state() and in tcp_listen_input() if tcp_enqueue_flags fails.

Regarding solving these issues, I suggest to change tcp_backlog_accepted() to also set pcb->listener to NULL (enabling safe usage if called redundantly), and call this from relevant places (tcp_pcb_purge, tcp_abandon). Maybe also in tcp_close_shutdown in some cases but I'm not familiar with the closing logic.

Ambroz Bizjak <abizjak>
Wed 23 Mar 2016 07:45:20 AM UTC, comment #14: 

Fixed by changing accept and backlog: instead of having an accept callback in the cnnection pcb, there is now a pointer to the listening pcb. That way, accepts_pending can be decreased very fast. This is now done internally in the stack ('tcp_accepted()' got useless now).

An application can delay decreasing 'accepts_pending' by manually calling 'tcp_backlog_delayed()' and 'tcp_backlog_accepted()' (which is what netconns/sockets now do).

The only downside is that when a listener is closed, all tcp pcb lists have to be traversed to reset the 'listener' pointer, but that should happen so often anyway...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 06 Mar 2016 05:18:06 PM UTC, comment #13: 

I'd be glad to get a better working version of the "accepts_pending" implementation. The current way of letting the user application keeping track of the corresponding listen pcb is really bad.

I'd favour an implementation where the stack decreases "accepts_pending" before calling the "accept" callback and the application has to call API functions (on the connection pcb) to increase/decrease "accepts_pending" (or a 2nd counter) when connections are put on a queue.

One possible improvement would be to remove the "accept" callback from non-listening pcbs and giving them a pointer to their listen-pcb instead (which has the accept callback pointer). Closing a listen-pcb would require walking all pcb lists, but that could be worth it.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 21 Jan 2016 05:59:07 PM UTC, comment #12: 

Also netconn should be releasing the reference in the error paths in accept_function(). Assuming that the lwip core is not changed with respect to releasing the reference itself.

Ambroz Bizjak <abizjak>
Thu 21 Jan 2016 05:56:51 PM UTC, comment #11: 

Hey Joel,

Isn't the problem with not having the listener reference only in tcp_accept_null? That one can just do the same thing as tcp_pcb_purge does - find the listener. In my patch this is the function tcp_pcb_decrement_accepts_pending_of_listener(). So all that needs to be done is to fix tcp_accept_null:


static err_t tcp_accept_null(void *arg, struct tcp_pcb *pcb, err_t err)
{
  LWIP_UNUSED_ARG(arg);
  LWIP_UNUSED_ARG(pcb);
  LWIP_UNUSED_ARG(err);

  tcp_pcb_decrement_accepts_pending_of_listener(pcb);
  tcp_abort(pcb);

  return ERR_ABRT;
}

On the other hand I'm not so sure the netconn is handling the accept reference right. For one thing it doesn't seem to release it in the PCB's error callback (err_tcp). Also I find the tcp_accepted() in lwip_netconn_do_recv() suspicions.

Please be aware though that I don't have a real interest in netconn...

Ambroz Bizjak <abizjak>
Thu 21 Jan 2016 04:58:43 PM UTC, comment #10: 

Ambroz,

I think we both proposed the same solution, sorry if I wasn't clear :)

I agree that the accept callback could also take responsibility for decrementing accepts_pending in the failure case.  As you mentioned, we would also need a reference to the listener PCB.  I'm not sure how easily we can change the accept callback function signature to pass that because it would break existing raw applications, also we would need to do a look up in tcp_process()

The netconn layer actually does already have the reference in accept_function (api_msg.c) because it passed the listener netconn via tcp_arg(), but we would want to make the listener easily accessible for raw applications as well


Joel Cunningham <jcunningham>
Group Member
Tue 19 Jan 2016 06:00:02 PM UTC, comment #9: 

Also, I wouldn't complicate the situation by interpreting errors from the accept callback differently as long as the accepts_pending reference is concerned. The accept callback should assume ownership of a single reference regardless of what error code it returns.

Ambroz Bizjak <abizjak>
Tue 19 Jan 2016 05:55:06 PM UTC, comment #8: 

I think it doesn't matter if you manipulate the backlog or accepts_pending. If you want to have separate control over the backlog, incrementing/decrementing the accepts_pending instead of decrementing/incrementing the backlog makes sense.

But then the normal flow would be suboptimal:
(1) When SYN is received accepts_pending would be incremented.
(2) When SYN-ACK is received accepts_pending would be decremented just before the accept callback.
(3) The accept callback would increment it right back.
(4) When the socket is passed upward, accepts_pending would finally be decremented.

So maybe we should just not decrement accepts_pending in step (2), and effectively pass the reference to the accept callback. Just fix the tcp_accept_null to call tcp_accepted() or more specificaly an equivalent which find the lpcb, since we don't have it at hand.

Anything that keeps a queue of accepted connections between lwIP core and the application can deal with this reference properly. So:
- When finally passing the connections into accept, call tcp_accepted().
- If the PCB is killed by lwip before that, tcp_accepted() must be called from the PCB's error callback.

Note that this doesn't need any change in tcp_pcb_purge(). This one would keep being responsible just for decrementing accepts_pending of SYN_RCVD PCBs.

Ambroz Bizjak <abizjak>
Tue 19 Jan 2016 05:30:26 PM UTC, comment #7: 

Ambroz,

I understand the suggestion (thanks!) and have a couple of thoughts:

If the solution is to have the accept callbacks manage controlling the "connected, but not accepted" connections via manipulating the backlog value, then we could also implement a more clear and easier solution by having them manipulate accepts_pending if the callback fails to take the connection (returns !ERR_OK). Currently that's not a responsibility of the accept callback and where this issue arises from.  Even the default tcp_accept_null() has the same issue

The backlog value stored within the TCB would no longer be the total count of both "in-progress connections" and "connected, but not accepted" queues.  I have another patchset posted (https://savannah.nongnu.org/patch/?8764) that allows updating of the backlog value via another call to listen() which behaves as most BSD implementations do.  This suggested solution doesn't play well with this feature because it could lead to some cases where the application increments the backlog while there are "connected, but not accepted" connections, this would then artificially inflate the backlog when those are accepted

The suggested patch also reduces performance by having every passive connection run through the listen PCB list where before this was avoided, at least for sockets/netconn, not sure about users of raw API.

I like the solution of having TCP core handle bookkeeping on accepts_pending when the accept_callback() refuses/fails to take the connection.

Joel Cunningham <jcunningham>
Group Member
Mon 18 Jan 2016 09:45:38 PM UTC, comment #6: 

Note you don't even need to change core lwIP code to do this, just cast the tcp_pcb* into tcp_pcb_listen* and have fun with the ->backlog.

Attached is a patch implementing my solution in comment #3.

(file #36087)

Ambroz Bizjak <abizjak>
Mon 18 Jan 2016 09:37:42 PM UTC, comment #5: 

Hey Joel,

I wasn't considering that. But the solution is fine for applications which use the raw API directly and do not delay accepting the connection.

Actually, I think there is a minimally invasive way to go from this design to one which limits SYN_RCVD plus the number of ESTABLISHED but not "fully accepted" connections. That is:
- In the accept callback, decrement lpcb->backlog as you queue up the connection for passing up to the application.
- Once you do finally pass the connection to the higher layer (the final "accept"), increment lpcb->backlog.
- Also take care to increment lpcb->backlog if you abandon the connection before passing it up.

By temporarily decrementing the lpcb->backlog, you effectively make lwIP consider your half-accepted connections in the backlog size check when SYN packets are received.

Actually, in my application I play with lpcb->backlog to limit the total number of PCBs used for a service, preventing unintended closing of connections for other services. To do this, I decrement lpcb->backlog in the accept callback, and only increment it back when the connection is closed.

Ambroz Bizjak <abizjak>
Mon 18 Jan 2016 05:19:38 PM UTC, comment #4: 

Ambroz,

Other network stacks maintain two queues of connections for the backlog: one for in-progress connections (SYN_RCVD) and a separate queue for completed connections (ESTABLISHED) that have not been accepted by the application.  As the stack completes connections from the in-progress queue, the are moved to the completed queue and the application is notified it can call accept().  Once accept() is called, the completed queue is decremented.  The backlog value is the count of both queues.

LwIP is in-affect implementing the same mechanism, but without actually maintaining queues/lists.  The backlog value (accepts_pending) covers the count of both queues.

With your suggestion, we would be removing the queue of "completed but not accepted" connections from the backlog tracking

I think the problem is that since we aren't maintaining a list of PCBs that are ESTABLISHED but not accepted, we don't know in tcp_pcb_purge() when to decrement the accepts_pending value other than for PCBs in SYC_RCVD

Joel Cunningham <jcunningham>
Group Member
Sun 17 Jan 2016 05:58:03 PM UTC, comment #3: 

Hey,
I think there is a simple solution for this:
- In tcp_process(), decrement accepts_pending just as pcb->state is set to ESTABLISHED (from SYN_RCVD).
- Remove the decrement of accepts_pending in tcp_accepted().

My thinking is that accepts_pending should always be equal to the number of PCBs in SYN_RCVD state associated with the listener. So it makes sense to decrement it at the transition SYN_RCVD->ESTABLISHED. Also consider that tcp_pcb_purge() decrements it only when the PCB is in SYN_RCVD state.

Ambroz Bizjak <abizjak>
Wed 16 Dec 2015 09:39:19 PM UTC, comment #2: 

Thinking about this more, the tcp_accept_null() function also has the same problem (actually any accept callback that calls tcp_abort and returns ERR_ABRT).  It calls tcp_abort(), but since the connection is in ESTABLISHED state, accepts_pending won't be decremented

The problem seems to stem from the fact that the connections are in the ESTABLISHED state and still in the backlog, but this knowledge isn't tracked at the TCP level and instead relies on upper layers (netconn's accept mbox) to associate the established connections with the listener.  The upper layer is then responsible for calling tcp_accepted() when the pending connections are pulled off the backlog.

For this same reason we can't just adjust the tcp_pcb_purge() to check for ESTABLISHED PCBs in the backlog because TCP doesn't know which ESTABLISHED PCBs are in the backlog and which aren't.  The SYN_RCVD check in tcp_pcb_purge() learns which connections are still in the backlog by matching the local port with a listener and using the SYN_RCVD state.

Maybe we need an actual backlog queue of PCBs in LwIP?  I know other stacks have multiple queues depending on the state of the pending connection


Joel Cunningham <jcunningham>
Group Member
Wed 16 Dec 2015 05:44:12 PM UTC, comment #1: 

Attached is a patch for solution #2

Joel Cunningham <jcunningham>
Group Member
Wed 16 Dec 2015 03:37:22 PM UTC, original submission:  

I'm running LwIP master branch and in my product (NO_SYS = 0, sockets/netconn enabled), I've encountered the following behavior:

With a TCP listener, if a connection in SYN_RCVD receives its ACK, but calling TCP_EVENT_ACCEPT() returns an error (ERR_MEM in my case) then the new connection is aborted, but accepts_pending is not decreased on the parent listener PCB, causing a leak in accepts_pending

In the call tree of tcp_abort(), tcp_pcb_purge() does get called, which contains logic to decrease accepts_pending on the listener PCB, but only if the PCB is in SYN_RCVD and in our case, we advance the state to ESTABLISHED right before calling TCP_EVENT_ACCEPT

The referenced code is in tcp_in.c, tcp_process()


case SYN_RCVD:
    if (flags & TCP_ACK) {
      /* expected ACK number? */
      if (TCP_SEQ_BETWEEN(ackno, pcb->lastack+1, pcb->snd_nxt)) {
        pcb->state = ESTABLISHED;
        LWIP_DEBUGF(TCP_DEBUG, ("TCP connection established %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
#if LWIP_CALLBACK_API
        LWIP_ASSERT("pcb->accept != NULL", pcb->accept != NULL);
#endif
        /* Call the accept function. */
        TCP_EVENT_ACCEPT(pcb, ERR_OK, err);
        if (err != ERR_OK) {
          /* If the accept function returns with an error, we abort
           * the connection. */
          /* Already aborted? */
          if (err != ERR_ABRT) {
            tcp_abort(pcb);
          }
          return ERR_ABRT;
        }


I had a couple of ideas:

  1. Don't advance the state to ESTABLISHED until after calling TCP_EVENT_ACCEPT
  2. Revert the state to SYN_RVCD before calling tcp_abort() in the error case
  3. Manually perform the booking keeping on the listener PCB in the error case of SYN_RCVD


I'm leaning towards #2 as that doesn't change anything when TCP_EVENT_ACCEPT() is called and doesn't introduce duplicated code

Joel Cunningham <jcunningham>
Group Member

 

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

Attached Files
file #36087:  lwip-backlog.patch added by abizjak (5KiB - text/x-patch - Track accepts_pending based on the number of SYN_RCVD connections.)
file #35765:  0001-Fix-for-accepts_pending-leak-on-failed-TCP_EVENT_ACC.patch added by jcunningham (1KiB - application/octet-stream - Added patch for solution #2)

 

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 abizjak (Posted a comment)
  • -email is unavailable- added by jcunningham (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-03-25 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-03-25 goldsimon StatusFixed In Progress
    2016-03-24 goldsimon Open/ClosedClosed Open
    2016-03-23 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-03-06 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
        Planned Release 1.5.0
    2016-01-18 abizjak Attached File- Added lwip-backlog.patch, #36087
    2015-12-16 jcunningham Attached File- Added 0001-Fix-for-accepts_pending-leak-on-failed-TCP_EVENT_ACC.patch, #35765

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code