buglwIP - A Lightweight TCP/IP stack - Bugs: bug #22530, api_msg.c's recv_raw() does not...

 
 

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

bug #22530: api_msg.c's recv_raw() does not consume data

Submitter:  Jonathan Larmour <jifl>
Submitted:  Mon 10 Mar 2008 01:17:11 PM UTC
   
 
Category:  None 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

Wed 12 Mar 2008 04:59:53 PM UTC, comment #11: 


> the lwip_recvfrom for a SOCK_RAW should also returns the IP header


If that's the case, everything looks fine to me.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 12 Mar 2008 11:52:27 AM UTC, comment #10: 

Uhmm, there is always a "small" problem: the netbuf::addr is initialized with an address inside the "p" packet, and not inside the "q" one.

In fact, it's the same kind of problem than "[bug #22435] lwip_recvfrom with TCP break".

The fastest way to fix it should be to write :

buf->addr = &(((struct ip_hdr*)(q->payload))->src);

No objections ?

Frédéric Bernon <fbernon>
Group Member
Wed 12 Mar 2008 11:01:50 AM UTC, comment #9: 

It's check in. I think we can close it if there is no objections...

Frédéric Bernon <fbernon>
Group Member
Wed 12 Mar 2008 10:51:13 AM UTC, comment #8: 

Answer : the lwip_recvfrom for a SOCK_RAW should also returns the IP header (thank you jifl)

http://linux.die.net/man/7/raw ("...For receiving the IP header is always included in the packet.")

http://msdn2.microsoft.com/en-us/library/ms740548.aspx ("An application always gets the IP header at the front of each received datagram ...")

So, the patch is good. Note, the contrib/apps/ping is wrong, and I will fix it...


Frédéric Bernon <fbernon>
Group Member
Wed 12 Mar 2008 10:27:00 AM UTC, comment #7: 

I see a "problem" during my tests: what is supposed to return a raw pcb at the netconn/socket layer? Should the payload have to point on the IP header, or on the IP payload ?

I ask that because a hidden problem with the current code is - I suppose - the input processing packet should do a pbuf_header so, sample like the ping.c (with LWIP_SOCKET=1) in the contrib folder failed (see ping.c, line~162) : the code suppose that lwip_recvfrom return the IP payload (so, the ICMP header), but is it the normal behavior?

Is it the stack which should add a "pbuf_header( q, -PBUF_IP_HLEN)" (or better, use IPH_HL), or to the application ?

Frédéric Bernon <fbernon>
Group Member
Mon 10 Mar 2008 07:47:53 PM UTC, comment #6: 

Definitively should go in for 1.3.0 since this is a real bug fix! Good work!

Simon Goldschmidt <goldsimon>
Group administrator
Mon 10 Mar 2008 03:10:27 PM UTC, comment #5: 

Looks good to me. If it's to be checked in before 1.3.0 it would probably be good to test it just in case, as there won't be much more testing after now.

Jonathan Larmour <jifl>
Group Member
Mon 10 Mar 2008 02:31:21 PM UTC, comment #4: 

I attach a patch file. Since I have the case with ping sender&ping receive in my application, I will test it if you want...


(file #15219)

Frédéric Bernon <fbernon>
Group Member
Mon 10 Mar 2008 02:14:44 PM UTC, comment #3: 

Yes, which is why I don't think the API layer should eat the packet, as it's not definite that the API user is the proper (and only) place for the packet.

Jonathan Larmour <jifl>
Group Member
Mon 10 Mar 2008 02:03:05 PM UTC, comment #2: 

If we return 1 in recv_raw (in api_msg.c), I suppose the stack would stop to response to "ping" if one of the application open a raw pcb with IP_PROTO_ICMP proto (by example, to send ping), right?

Frédéric Bernon <fbernon>
Group Member
Mon 10 Mar 2008 01:25:29 PM UTC, comment #1: 

I agree that this is a bug. recv_raw should not assume that if it hasn't "eaten" the data it will continue unmodified.

Although I don't think the suggested fix is correct. That would produce odd behaviour that would change depending on exactly when an application makes a call to receive a packet or not. It would not be reliable.

The correct answer is probably to still not "eat" the packet, but to copy the pbuf entirely into a new pbuf. The reason we don't "eat" the packet is because we don't know whether the data is definitely for us or not. It's the same principle as raw_input() which tries all the raw PCBs in turn until one of them eats the data - it doesn't know which one the data is really for. It would be more wrong to "eat" the data before it reaches the PCB that actually uses it, than to duplicate the data.

Jonathan Larmour <jifl>
Group Member
Mon 10 Mar 2008 01:17:11 PM UTC, original submission:  

From lwip-users:
-=-=-=-=-=-=-=-
I have an application that opens up a raw socket (SOCK_RAW), and is expecting to receive an ICMP packet.

In this case, depending on the kernel scheduling, sometimes the packet received includes the IP header, and sometimes not. This occurs because recv_raw (api_msg.c) posts the received packet into the applications mbox. However, it does not say that the packet has been processed (or "eaten" according to the sorrounding comments), so further layers continue processing the packet. Eventually it reaches icmp_input which moves the payload past the ip header.

It seems like the code in recv_raw should return 1 in cases where there is an application waiting for the input. I can see that this is ok to do for ICMP packets, but I'm not sure if it is generally applicable.

The issue exists in lwip-1.2.0, and at first glance appears to exist in lwip-1.3.0rc1 also.
-=-=-=-=-=-=-=-=-

Jonathan Larmour <jifl>
Group Member

 

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

Attached Files
file #15219:  recv_raw.patch added by fbernon (2KiB - 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 goldsimon (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by jifl (Submitted the item)
  • -email is unavailable- added by jifl
  •  

    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-12 fbernon StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2008-03-12 fbernon Assigned toNone fbernon
        Planned Release 1.3.0
    2008-03-10 fbernon Attached File- Added recv_raw.patch, #15219
    2008-03-10 jifl Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code