buglwIP - A Lightweight TCP/IP stack - Bugs: bug #34669, Inverted byte order in...

 
 

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

bug #34669: Inverted byte order in TCP_BUILD_MSS_OPTION

Submitted by:  Andrea Sacco <socket>
Submitted on:  Fri 28 Oct 2011 09:18:23 AM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Invalid
Privacy: PublicAssigned to: None
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.4.0

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

Fri 28 Oct 2011 04:46:31 PM UTC, comment #6:

Hehe...

Closing as invalid.

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 28 Oct 2011 03:51:53 PM UTC, comment #5:

> If you're running it on the PC (which is little endian)

..Doh!

I was running the code on a PC, being sure it was BIG endian..

I really apologize for the incovenience and I thank you all for the quick and authoritative responses..

Now in my office is echoing this funeral march http://www.youtube.com/watch?v=kfSrzGcCbGk

Regards,
Andrea

Andrea Sacco <socket>
Fri 28 Oct 2011 03:03:04 PM UTC, comment #4:

And the code you attached, does it run on the target board or on a PC? I don't have big-endian hardware to test, so I might not be able to reproduce the issue...

If you're running it on the PC (which is little endian), that won't work as the code uses htonl() (which is little-endian because defined to nothing and you are running it on a little-endian machine) to build the mss option but uses array access (which expects big-endian).

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 28 Oct 2011 02:03:15 PM UTC, comment #3:

> When running the win32 simulation...


We're not running in simulation mode. We have noticed this behaviour during a unit-test campaign we're working on.
In our configuration, the htons, ntohs, etc. functions are redefined by the preprocessor switch BYTE_ORDER=BIG_ENDIAN as:

#if BYTE_ORDER == BIG_ENDIAN
#define htons(x) (x)
#define ntohs(x) (x)
#define htonl(x) (x)
#define ntohl(x) (x)
#else /* BYTE_ORDER != BIG_ENDIAN */

We're testing your code for an industrial application running on ColdFire micro.

> Andrea, could you please provide a wireshark-screenshot of a faulty connection
> from lwIP that shows us what you think is wrong?


No I can't, i will need the board.. But I can provide you the code to reprocuce the error. I attach it to the comment.

Into the test we build a fake tcp header only with the option field and try to decode it with the parseopt function from the tcp_in module.
If you look at the gcov file you will notice that the parsing process will fail.
Now, uncommenting the FIXED_TCP_BUILD_MSS_OPTION_() call into the main, will produce a successful parding operation.

Regards,
Andrea

(file #24231)

Andrea Sacco <socket>
Fri 28 Oct 2011 11:08:52 AM UTC, comment #2:

> Can anyone verify that this is wrong in the current code?
> I find it hard to believe that this wouldn't have been noticed before now.


When running the win32 simulation, wireshark decodes "MSS=1024" for TCP_MSS=1024, which looks right to me (the bytes do match the order in the png attached to the OP). And since the TCP_BUILD_MSS_OPTION define uses htonl(), I would have expected this to work both on little- and big-endian platforms.

Andrea, could you please provide a wireshark-screenshot of a faulty connection from lwIP that shows us what you think is wrong?

> the MSS value is never changed, the packet option will result
> always with the default value of 536 defined into TCP_MSS define


This has been fixed in git exactly 10 days ago (reported as bug #34587). The macro now uses pcb->mss instead of TCP_MSS.

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 28 Oct 2011 09:39:45 AM UTC, comment #1:

Can anyone verify that this is wrong in the current code? I find it hard to believe that this wouldn't have been noticed before now.

Kieran Mansley <kieranm>
Project Administrator
Fri 28 Oct 2011 09:18:23 AM UTC, original submission:

The TCP_BUILD_MSS_OPTION macro writes the MSS value and the related option-type and option-length fields with an inverted byte order.

According to [1] the Maximum Size Segment option should be written into the TCP header with the following bytes order:

+--------+--------+---------+--------+
|00000010|00000100| max seg size |
+--------+--------+---------+--------+
Kind=2 Length=4

Also a Wireshark data acquisition of a generic TCP communication trace shows the same pattern [2].

The TCP_BUILD_MSS_OPTION macro has to problems:

- the MSS value is never changed, the packet option will result always with the default value of 536 defined into TCP_MSS define

- the byte order of the option field is written inverted, the kind field will results in the 4th bytes and the LSB of the data field will results in the 1st one (e.g. MSS=536 => 0x02040218)

The solution will be to invert the TCP_BUILD_MSS_OPTION bytes order like that:

#define TCP_BUILD_MSS_OPTION(x) (x) = htonl(((u32_t)2) | \
((u32_t)4 << 8) | \
(((u32_t)TCP_MSS / 256) << 16) | \
(TCP_MSS & 255) << 24)

with MSS=536 this will result in opts=0x18020402, allowing the tcp_parseopt function to properly parse the option.

Kindest regards,
Andrea Sacco

References

[1] RFC 793 - Transmission Control Protocol - 3.1 pp 18-19
[2] Wireshark screenshot, google.com request/response

Andrea Sacco <socket>

 

Attached Files
file #24231:  mss.option.test.zip added by socket (108KiB - application/zip)
file #24226:  google.com.request.png added by socket (140KiB - image/png - screenshots)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by socket (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 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 28 Oct 2011 04:46:31 PM UTCgoldsimonStatusWorks For Me=>Invalid
      Open/ClosedOpen=>Closed
    Fri 28 Oct 2011 02:03:15 PM UTCsocketAttached File-=>Added mss.option.test.zip, #24231
    Fri 28 Oct 2011 11:09:00 AM UTCgoldsimonStatusNeed Info=>Works For Me
    Fri 28 Oct 2011 11:08:52 AM UTCgoldsimonStatusNone=>Need Info
    Fri 28 Oct 2011 09:18:23 AM UTCsocketAttached File-=>Added google.com.request.png, #24226

    Back to the top


    Powered by Savane 3.1-cleanup1