buglwIP - A Lightweight TCP/IP stack - Bugs: bug #22435, lwip_recvfrom with TCP break;

 
 

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

bug #22435: lwip_recvfrom with TCP break;

Submitted by:  Cui hengbin <phant>
Submitted on:  Thu 28 Feb 2008 08:16:42 AM UTC  
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  Frédéric Bernon <fbernon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

( Jump to the original submission)

Thu 13 Mar 2008 08:06:04 PM UTC, comment #10: 

Ok, it's check in, and documented in the CHANGELOG like Simon asks.

Reopen it if necessary...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 13 Mar 2008 11:45:47 AM UTC, comment #9: 

However we could add some documentation that using recvfrom like this is a bit slow and that using getpeer* is the lwip way to do it  (just in case someone wonders why it is slow...)?

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 13 Mar 2008 11:44:27 AM UTC, comment #8: 

Not from me, I feel perfectly fine with the fix posted in comment #3. As Jonathan said, you can probably count the people using recvfrom on a TCP socket on one hand ;-)

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 12 Mar 2008 08:19:45 PM UTC, comment #7: 

Is there any other comments? I would like to fix this bug tomorrow.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 12 Mar 2008 11:14:43 AM UTC, comment #6: 

I prefer the patch you posted in comment #3. It also means the overhead for retrieving the peer information is only incurred when it's specifically requested. In my experience, I don't think many people would be using recvfrom on a TCP socket to receive and get the peer address - they usually use getpeername() or whatever.

Jonathan Larmour <jifl>
Project Member
Wed 12 Mar 2008 09:47:47 AM UTC, comment #5: 

I'm agree that using netbufs for tcp is not a so good solution, so, perhaps we could add in struct netconn two new fields :

/** A netconn descriptor */
struct netconn {
...
#if LWIP_TCP
struct ip_addr tcp_remote_ip;
u16_t          tcp_remote_port;
#endif /* LWIP_TCP */
}

Since there is most of time only some netconns (less than pbufs), the memory consume will be lower. And we could initialize these fields only one time at the connection. More, it will also fix the netconn api.

Else, in a first time for 1.3.0, I could check in the patch like this, and we could do this change later in post-1.3.0.

Comments?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 11 Mar 2008 06:13:13 PM UTC, comment #4: 

I think the patch is fine. Posting netbufs instead of pbufs would consume more memory at the benefit of faster execution (if anyone uses recvfrom for TCP sockets at all). However, the information in those netbufs would be the same all the time: the remote side information cannot change once the connection has been estabslished!

Given this, I think the lwIP way is already covered by this patch.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 04 Mar 2008 02:18:10 PM UTC, comment #3: 

A first patch file, you can integrate it in a first time, but I will try to do something better if I found free time...

(file #15179)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 04 Mar 2008 01:08:54 PM UTC, comment #2: 

Frédéric: any update on this.  It would be nice to get a fix into 1.3.0 as you mentioned, so if you can do that in the next day or two I'd be happy to take it.

Kieran Mansley <kieranm>
Project Member
Thu 28 Feb 2008 02:44:29 PM UTC, comment #1: 

You're right, but your patch is not very thread-safe, since you use conn->pcb.tcp in the application context, and you can have a err_tcp before you fetch your packet. That's why we have add netconn_getaddr to query the peer.

So, I suggest this patch in lwip_recvfrom :

...
  /* Check to see from where the data was.*/
  if (from && fromlen) {
    struct sockaddr_in sin;

    if (netconn_type(sock->conn) == NETCONN_TCP) {
      addr = (struct ip_addr*)&(sin.sin_addr.s_addr);
      netconn_getaddr(sock->conn, addr, &port, 0);
    } else {
      addr = netbuf_fromaddr(buf);
      port = netbuf_fromport(buf);
    }
...

It got the advantage to avoid an extra thread-context switching for most of case where recv (and not directly recvfrom) is used for a TCP socket (in this case, from and fromlen are NULL).

I will propose a nicest patch this evening to integrate in 1.3.0.

But a better solution will be to post directly netbuf in the recv_tcp like for udp and raw callbacks. But this idea is more for post-1.3.0.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 28 Feb 2008 08:16:42 AM UTC, original submission:  

Hi All:
   when i use lwip_recvfrom() with from!=NULL,and fromlen!=NULL,it  will break .

int lwip_recv_from(...)
{
...
if (from && fromlen) {
    struct sockaddr_in sin;

    addr = netbuf_fromaddr(buf);
    port = netbuf_fromport(buf);

    memset(&sin, 0, sizeof(sin));
    sin.sin_len = sizeof(sin);
    sin.sin_family = AF_INET;
    sin.sin_port = htons(port);
    sin.sin_addr.s_addr = addr->addr;----> break in this line

...
}

i find in netconn_recv has follow codes:

#if LWIP_TCP
...
  buf->p = p;
  buf->ptr = p;
  buf->port = 0;
  buf->addr = NULL;             /* this line may be wrong*/
...
#endif

so should be as follow:

  buf->p = p;
  buf->ptr = p;
  buf-port = conn->pcb.tcp->remote_port;
  buf->addr = (struct ip_addr*)&(conn->pcb.tcp->remote_ip);

FYI

Cui hengbin <phant>

 

Attached Files
file #15179:  sockets.patch added by fbernon (594B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

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

    Only logged-in users can vote.

     

     

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-03-13 fbernon StatusConfirmed => Fixed
        Open/ClosedOpen => Closed
    2008-03-04 fbernon Attached File- => Added sockets.patch, #15179
    2008-03-04 kieranm Planned Release => 1.3.0
    2008-02-28 fbernon StatusNone => Confirmed
        Assigned toNone => fbernon

    Back to the top


    Powered by Savane 3.5