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

Submitter:  Andrea Sacco <socket>
Submitted:  Fri 28 Oct 2011 09:18:23 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Invalid
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.4.0

Jump to the original submission

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

Hehe...

Closing as invalid.

Simon Goldschmidt <goldsimon>
Group 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>
Group 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>
Group 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>
Group Member
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>

 

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

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

    Date Changed by Updated Field Previous Value => Replaced by
    2011-10-28 goldsimon StatusWorks For Me Invalid
        Open/ClosedOpen Closed
    2011-10-28 socket Attached File- Added mss.option.test.zip, #24231
    2011-10-28 goldsimon StatusNeed Info Works For Me
    2011-10-28 goldsimon StatusNone Need Info
    2011-10-28 socket Attached File- Added google.com.request.png, #24226

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code