buglwIP - A Lightweight TCP/IP stack - Bugs: bug #33672, checksum calculate error!!!

 
 

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

bug #33672: checksum calculate error!!!

Submitted by:  hanhui <hanhui03>
Submitted on:  Wed 29 Jun 2011 05:39:00 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

(Jump to the original submission Jump to the original submission)

Mon 04 Jul 2011 07:13:00 PM UTC, comment #8:

Could you maybe just provide an example of what you mean? Keep in mind that folding once does not always zero the upper half, as the lower half could be 0xFFFF.

Also, I think we cannot remove folding from inside the loop, as we need to fold before calling SWAP_BYTES_IN_WORD (if a pbuf has an uneven length).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 30 Jun 2011 07:08:00 PM UTC, comment #7:

What I mean really is why not just FOLD_U32T before the for loop in inet_cksum_pseudo_partial_base and call it a day? That zeroes the upper word and you don't need the newly added FOLD_U32T's every else they were added.

But it's been a long week already, so chances are it's me who doesn't get it. :-)

Bill Auerbach <billauerbach>
Thu 30 Jun 2011 05:59:20 PM UTC, comment #6:

Sorry, I don't get it. I'm not saying you're wrong, I just don't get it. Currently, all 4 pseudo-functions (v4/v6, pseudo/partial) call FOLD_U32T 5 times. The 2 folds at the end are always needed because if the lower 16 bits of 'acc' are 0xFFFF at that time, one FOLD call is not enough.

What do you mean with '8 times' and how would 2 times be enough here?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 30 Jun 2011 12:30:19 PM UTC, comment #5:

Correction. The patch is a bit inefficient. FOLD_U32T can be outside the loop and still be used only 2 times, not 8 times. Maybe I'm seeing it wrong?

Bill Auerbach <billauerbach>
Thu 30 Jun 2011 05:27:50 AM UTC, comment #4:

> Hanhui's solution is more efficient


Is it really? I deliberately kept the 2nd fold out of the loop. I could have left the argument as u32_t though, regarding performance (leaving away the cast, which on some platforms might result in an AND op).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 29 Jun 2011 09:39:33 PM UTC, comment #3:

Hanhui's solution is more efficient - keep the passed checksum to inet_cksum_pseudo_partial_base as a u32_t and only do the FOLD_U32T once in inet_cksum_pseudo_partial_base. You added 8 FOLD_U32T's which is 6 more than his solution.

Bill Auerbach <billauerbach>
Wed 29 Jun 2011 07:57:34 PM UTC, comment #2:

FIXED, THANKS FOR REPORTING!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 29 Jun 2011 07:53:16 PM UTC, comment #1:

OK, that's a bug introduced while merging the checksum_pseudo_*_base functions. Thanks for reporting this! However, in most places, the assumption of only having to fold once is correct, since 'acc' is (almost everywhere) limited to 16 bits, so when adding 0xFFFF and 0xFFFF, folding once is enough.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 29 Jun 2011 05:39:00 PM UTC, original submission:

in some cases, checksum calculate error!

for example:

inet_cksum_pseudo_partial_base(...)
{
...
for(q = p; (q != NULL) && (chksum_len > 0); q = q->next) {
...
acc = FOLD_U32T(acc);
if (q->len % 2 != 0) {
swapped = 1 - swapped;
acc = SWAP_BYTES_IN_WORD(acc);
}
...
}
}

line : acc = FOLD_U32T(acc); Can not guarantee that upper 16bit is 0

so it should be:

acc = FOLD_U32T(acc);
acc = FOLD_U32T(acc);
if (q->len % 2 != 0) {
swapped = 1 - swapped;
acc = SWAP_BYTES_IN_WORD(acc);
}

A similar error also exists in many places!!!

Debugging is very difficult to find this errors, CODING WITH CAUTION PLEASE! THANK YOU!

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 billauerbach (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 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 29 Jun 2011 07:57:33 PM UTCgoldsimonStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Wed 29 Jun 2011 07:53:16 PM UTCgoldsimonStatusNone=>In Progress
      Assigned toNone=>goldsimon

    Back to the top


    Powered by Savane 3.1-cleanup1