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

Submitted by:  Jonathan Larmour <jifl>
Submitted on:  Mon 10 Mar 2008 01:17:11 PM UTC  
 
Category:  None Severity:  3 - Normal
Item Group:  Faulty Behaviour 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)

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>
Project 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project 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>
Project 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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project 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>
Project Member

 

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
  •  

    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-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 siva.velusamy@gmail.com

    Back to the top


    Powered by Savane 3.5