Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#6789 closed bug (fixed)

Wrong comparison in tcp add-on

Reported by: kaliber Owned by: stippi
Priority: normal Milestone: R1
Component: Network & Internet/TCP Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Here is clang diagnose:

src/add-ons/kernel/network/protocols/tcp/tcp.cpp:201:43: error: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
                                if (option->length == 4 && (size - 4) >= 0)
                                                           ~~~~~~~~~~ ^  ~
src/add-ons/kernel/network/protocols/tcp/tcp.cpp:205:43: error: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
                                if (option->length == 3 && (size - 3) >= 0) {
                                                           ~~~~~~~~~~ ^  ~
src/add-ons/kernel/network/protocols/tcp/tcp.cpp:211:45: error: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
                                if (option->length == 10 && (size - 10) >= 0) {
                                                            ~~~~~~~~~~~ ^  ~
src/add-ons/kernel/network/protocols/tcp/tcp.cpp:219:43: error: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
                                if (option->length == 2 && (size - 2) >= 0)
                                                           ~~~~~~~~~~ ^  ~

Attachments (1)

tcp-wrong-comparison.patch (1.1 KB) - added by kaliber 9 years ago.
proposed patch

Download all attachments as: .zip

Change History (6)

Changed 9 years ago by kaliber

Attachment: tcp-wrong-comparison.patch added

proposed patch

comment:1 Changed 9 years ago by kaliber

Has a Patch: set

comment:2 Changed 9 years ago by stippi

Owner: changed from axeld to stippi
Status: newin-progress

I've looked into the problem and your patch is not a good solution. The length from the option is used regardless of those comparisions. If size happens not to be a multiple of the option length (which may be checked elsewhere in the code, I don't know), then the loop will overflow (wrap) the size variable. I will commit a different fix.

comment:3 Changed 9 years ago by stippi

Resolution: fixed
Status: in-progressclosed

Should be fixed in hrev39309.

comment:4 Changed 9 years ago by axeld

Actually, I think kaliber's patch was just fine. If you look at the caller of process_options(), the size cannot be smaller than 0.

His patch also makes the code more readable; I'm going to apply it later.

comment:5 Changed 9 years ago by axeld

Patch applied, and stippi's issue fixed differently in hrev39311. Thanks everyone!

Note: See TracTickets for help on using tickets.