buglwIP - A Lightweight TCP/IP stack - Bugs: bug #26658, ping tool ICMP checksum error.

 
 

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

bug #26658: ping tool ICMP checksum error.

Submitted by:  hanhui <hanhui03>
Submitted on:  Sun 24 May 2009 12:24:03 PM UTC  
 
Category: ContribSeverity: 3 - Normal
Item Group: Change RequestStatus: Invalid
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.3.0

Tue 02 Jun 2009 11:07:27 AM UTC, comment #5:

hanhui: could you open another bug for the ++ issue to ensure it doesn't get forgotten.

Kieran Mansley <kieranm>
Project Administrator
Mon 25 May 2009 06:49:51 AM UTC, comment #4:

Ah, thanks for clarifying this, I didn't get whether the ++ is wrong or the macro or whatever.

In fact, hanhui is correct with his observation: the ++ should be moved out of the function call as (at least how I see it) the port is allowed to implement this as a macro (can be fast if the processor supports single-instruction shifts). I wouldn't want to restrict ports here.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sun 24 May 2009 10:16:31 PM UTC, comment #3:

hanhui's comment #1 refers to the issue that a macro parameter with side effects (such as pre-increment or post-increment) will cause the side effect to occur multiple times if the parameter is referenced more than once in the macro.

In this case, assuming htons() is defined like this:

#define htons(x) LWIP_PLATFORM_HTONS(x)

#define LWIP_PLATFORM_HTONS(x) (u16_t)((((x) & 00ff) << 8) | \
(((x) & 0xff00) >> 8))

then the expression htons(++ping_seq_num) will expand to

(u16_t)((((++ping_seq_num) & 00ff) << 8) | \
(((++ping_seq_num) & 0xff00) >> 8))

As you can see, ping_seq_num is incremented twice.

This use of pre-increment or post-increment operators within the parameters to htons() is only valid if htons() is required to be a function. If it is ever implemented as a macro, the expression will have unexpected side effects.

The question then is whether htons() etc. are supposed to be functions. If this is a documented requirement, then this ping code is correct. If the port is allowed to implement htons() as a macro, then this ping code is wrong, and there may be other instances in the LWIP code where the same assumption is made.

David Empson <dempson>
Sun 24 May 2009 03:03:07 PM UTC, comment #2:

Re initial post:
this is already fixed in post-1.3.0 CVS

Re comment #1:
I'm afraida I don't understand what you want to tell us... :-(

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sun 24 May 2009 12:39:34 PM UTC, comment #1:

I have a advice,

need for speed, htons() htonl() ...

is macro like this:

#define LWIP_PLATFORM_HTONL(x) (u32_t)((((x) & 0x000000ff) << 24) | \
(((x) & 0x0000ff00) << 8) | \
(((x) & 0x00ff0000) >> 8) | \
(((x) & 0xff000000) >> 24))

if like ping tool use htons() like this :

iecho->seqno = htons(++ping_seq_num);

it will be error!!!

hanhui <hanhui03>
Sun 24 May 2009 12:24:03 PM UTC, original submission:

----------------CODE--------------------

function: ping_prepare_echo()
...
iecho->seqno = htons(++ping_seq_num);
iecho->chksum = inet_chksum(iecho, len);

/* fill the additional data buffer with some data */
for(i = 0; i < PING_DATA_SIZE; i++) {
((char*)iecho)[sizeof(struct icmp_echo_hdr) + i] = i;
}
...
----------------------------------------

I think first fill data and then calculate check sum

should be:

...
iecho->seqno = htons(++ping_seq_num);

/* fill the additional data buffer with some data */
for(i = 0; i < PING_DATA_SIZE; i++) {
((char*)iecho)[sizeof(struct icmp_echo_hdr) + i] = i;
}
iecho->chksum = inet_chksum(iecho, len);
...

hanhui <hanhui03>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by dempson (Posted a comment)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by hanhui03 (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 24 May 2009 03:03:07 PM UTCgoldsimonStatusNone=>Invalid
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1