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.

Submitter:  hanhui <hanhui03>
Submitted:  Sun 24 May 2009 12:24:03 PM UTC
   
 
Category:  Contrib Severity:  3 - Normal
Item Group:  Change Request Status:  Invalid
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned 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>
Group Member
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>
Group administrator
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>
Group administrator
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>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by dempson (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by hanhui03 (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-05-24 goldsimon StatusNone Invalid
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code