buglwIP - A Lightweight TCP/IP stack - Bugs: bug #16830, err_tcp() posts to connection...

 
 

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

bug #16830: err_tcp() posts to connection mailbox when no pend on the mailbox is active

Submitter:  None
Submitted:  Mon 12 Jun 2006 12:33:28 PM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  fbernon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Thu 05 Apr 2007 02:10:37 PM UTC, comment #13: 

Yes, in fact, it's done, but if I don't disable whitespace option, diff produce lot of lines and the patch file is not so clear.

Ok, I will commit later...

Frédéric Bernon <fbernon>
Group Member
Thu 05 Apr 2007 02:05:03 PM UTC, comment #12: 

That looks fine to me, although if you can fix up the indent before you checkin that would be nice.

Jonathan Larmour <jifl>
Group Member
Thu 05 Apr 2007 01:57:27 PM UTC, comment #11: 

New patch file with filter on do_send, do_write, do_join_leave_group (not on do_recv).



(file #12405)

Frédéric Bernon <fbernon>
Group Member
Thu 05 Apr 2007 01:29:24 PM UTC, comment #10: 

The "if (msg_copy.conn->err != ERR_OK) {"  in api_msg_input is not a so good idea, because even with an error on "conn->err", you have to execute do_disconnect, do_close & do_delconn. We can move the check at the beginning of do_listen, do_send, do_recv, do_write & do_join_leave_group...

Frédéric Bernon <fbernon>
Group Member
Thu 05 Apr 2007 12:53:14 PM UTC, comment #9: 

Ok, if no other comments, I will commit tonight...


Frédéric Bernon <fbernon>
Group Member
Thu 05 Apr 2007 12:43:51 PM UTC, comment #8: 

Yes I think that sounds like a good idea. Thanks.

Jonathan Larmour <jifl>
Group Member
Thu 05 Apr 2007 12:26:34 PM UTC, comment #7: 


>the tcpip thread will get the message from the program thread anyway to, for example, send data, which it will then try and do even though an error had been indicated for this connection.


Yes, it's right, even if in netconn_recv, netconn_send & netconn_write, there is at a beginning :

  if (conn->err != ERR_OK) {
    return conn->err;
  }

If tcpip_thread invoke err_tcp() just after this checking, you can send a message to api_msg. One "easy" solution is just to add a such checking at api_msg_input() beginning (and to post a null message to unblock calling thread if there is an error).

void
api_msg_input(struct api_msg *msg)
{ struct api_msg_msg msg_copy;
  msg_copy=msg->msg;
  if (msg_copy.conn->err != ERR_OK) {
    sys_mbox_post(msg_copy.conn->mbox, NULL);
  } else {
    decode[msg->type](&msg_copy);
  }
}

It a better solution, even if we add one more check in tcpip_thread. It's not in remplacement from the api_lib ones, because they help to reduce tcpip_thread "interruption"...

What do you think about these idea, and are you agree I update in CVS?

Frédéric Bernon <fbernon>
Group Member
Thu 05 Apr 2007 11:10:53 AM UTC, comment #6: 

Hmm, yes I guess the problem isn't what I thought it was. The thread won't get stuck, but it means that a packet may arrive and be processed by the tcpip thread, but then after that, the tcpip thread will get the message from the program thread anyway to, for example, send data, which it will then try and do even though an error had been indicated for this connection.

Maybe this sort of race condition is unavoidable, and not harmful, though.

Jonathan Larmour <jifl>
Group Member
Thu 05 Apr 2007 10:46:00 AM UTC, comment #5: 

Not sure you're right: if I'm agree with you that an error may be indicated at any time, this is only in during a TCP connect we have to "wake up" a pending "netconn_connect".

Why? Because, first, err_tcp is always invoked in the tcpip_thread context. So, no packet input can be processed during this call (we have to return from api_msg_input before tcpip_thread main loop fetch a packet from its "input" - TCPIP_MSG_INPUT or TCPIP_MSG_ETHINPUT). That's why err_tcp can't be consider really "asynchrone" in sequential api.

So during any path during api_msg_input calls (other than TCP connect), you always got a "sys_mbox_post(msg->conn->mbox, NULL)" to wake up the calling thread. I already check that, but please, if I'm wrong, tell me.

During a TCP connect, if you got an err_tcp, you never got the do_connected. Right? So, with this patch, I don't think it's possible to block any application thread, and I don't think you can got two "sys_mbox_post(msg->conn->mbox, NULL)" for a same netconn_xxx...

Can you confirm if you are agree with that points?




Frédéric Bernon <fbernon>
Group Member
Thu 05 Apr 2007 10:20:45 AM UTC, comment #4: 

I don't think this patch is right I'm afraid. An error may be indicated at any time, not just during connection. And given that, if the connection is already made, a thread will remain stuck waiting for a response from its api_msg_post call (which waits on the connection mbox) when an error happens.

Rereading the initial report in this bug, I think the analysis may be slightly wrong when it says: "It appears that this only occurs when the connection is attempting to connect via a call to netconn_connect()."

I can think of various solutions that only work if we can add an extra field to struct netconn. Perhaps one would be to change thetested "state" member into a u8_t flags field, allowing us to use 3 bits for what is the current "state", and allowing us further bits for other purposes, including one bit to indicate whether there is a thread waiting on the connection's mbox.

Since err_tcp can be set asynchronously, testing/setting that bit would have to be protected. I don't think we can use the connection semaphore for this, so rather than create a new semaphore maybe just SYS_ARCH_PROTECT ?

Or some other alternative solution?


Jonathan Larmour <jifl>
Group Member
Wed 04 Apr 2007 06:56:37 PM UTC, comment #3: 

I propose another patch for these two problems. NETCONN_CONNECT is set only in tcpip_thread context (inside api_msg.c). The idea is the same than the first patch, but only in api_msg.c (avoid the problem with the first patch if a RST appears during time between a successful do_connect and the api_lib "conn->state = NETCONN_NONE;").

Jonathan, I think the same patch is a solution to your problem. Can you confirm?

Thank you about your comments...


(file #12401)

Frédéric Bernon <fbernon>
Group Member
Mon 26 Feb 2007 02:07:20 PM UTC, comment #2: 

I can confirm the problem exists. I can't offer a simple fix - I think a more radical overhaul would be needed given the requirements for thread safety, and probably involving something protected with the SYS_ARCH_PROTECT stuff.

What I did want to say is that this is not the only problem with the err_tcp communication mechanism: errors can result in one of these null messages being left in conn->mbox when netconn_delete is called. OSes do not freeing mboxes with messages in - doing so might imply a memory leak. I am attaching a patch I used in my own code, but I didn't contribute it because of the problem indicated by the FIXME comment.



(file #12041)

Jonathan Larmour <jifl>
Group Member
Mon 26 Feb 2007 01:03:13 PM UTC, comment #1: 

I think this needs some investigation.  Any volunteers to look into it and come up with a better method of error propogation if the problem is as reported?

Kieran Mansley <kieranm>
Group Member
Mon 12 Jun 2006 12:33:28 PM UTC, original submission:  

When a TCP error occurs and err_tcp() is called, the function posts a null message to the connection's mailbox.  Since the mailbox is used for synchronization of connection messages to the TCP thread, posting to it is only valid when a connection function is pending on the result.  It appears that this only occurs when the connection is attempting to connect via a call to netconn_connect(). In all other instances, the connection's mailbox will receive a message from the TCP thread.

Posting an extra message in err_tcp() causes the next netconn_xxx() API call to immediately proceed when it performs its sys_mbox_fetch(). This results in a it prematurely proceeding and freeing the message object it submitted.

Anonymous

 

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

Attached Files
file #12405:  err_tcp.patch added by fbernon (2KiB - text/x-patch)
file #12401:  err_tcp.patch added by fbernon (1KiB - text/x-patch)
file #12041:  lwip.netconndelete.free.mbox.patch added by jifl (1KiB - text/x-patch - Imperfect workaround for similar issue with netconn_delete)
file #10178:  err_tcp_patch.txt added by None (1KiB - text/plain - Simple-minded patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by fbernon (Updated the item)
  • -email is unavailable- added by jifl (Updated the item)
  • -email is unavailable- added by kieranm (Posted a comment)
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2007-04-05 fbernon StatusNone Fixed
        Open/ClosedOpen Closed
    2007-04-05 fbernon Attached File- Added err_tcp.patch, #12405
    2007-04-04 fbernon Attached File- Added err_tcp.patch, #12401
    2007-03-29 fbernon Assigned toNone fbernon
    2007-02-26 jifl Attached File- Added lwip.netconndelete.free.mbox.patch, #12041
    2006-11-25 christiaans CategoryTCP sockets/netconn
    2006-06-12 None Attached File- Added err_tcp_patch.txt, #10178

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code