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;

Submitter:  Cui hengbin <phant>
Submitted:  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:  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>
Group Member
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>
Group 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>
Group 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>
Group Member
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>
Group 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>
Group Member
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>
Group 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>
Group Member
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>
Group 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>
Group Member
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>

 

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

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)
  •  

    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
    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.13-758e.
    Corresponding source code