Opened 2 years ago

Closed 22 months ago

#19168 closed defect (fixed)

Integer overflows in case conversion tables

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points: .2
Reviewer: Sponsor:

Description

The case conversion tables contain integer overflows because the type is a signed char. All values above CHAR_MAX (typically +127) overflow.

Patch is coming once i know the bug number.

Child Tickets

Attachments (1)

0001-Fix-integer-overflows-in-the-conversion-tables.patch (2.2 KB) - added by cypherpunks 2 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 2 years ago by cypherpunks

Status: newneeds_review

comment:2 Changed 2 years ago by nickm

Milestone: Tor: 0.2.9.x-final
Status: needs_reviewneeds_revision

From what I can tell from the C standard, "integer overflow" means integer operations that produce a result that can't be represented in the arithmetic type; it doesn't refer to the conversion that an ordinary cast or assignment. The results of conversion from an unsigned value to a signed type that can't hold that value is implementation-defined, not undefined. See 6.3.1.3 from http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf , and similar language in earlier versions of C.

(Also, minor note: 'char' is not the same as 'signed char' or 'unsigned char'; the signedness of char is also implementation-defined.)

Unless I'm super-wrong here, I'd plan to close this as wontfix?

comment:3 Changed 2 years ago by cypherpunks

Sorry for mixing up the terminology. It came from the message GCC was giving me

error: overflow in implicit constant conversion [-Werror=overflow]

Because the tables contain integers, i figured they are integer overflows but i stand corrected.

This error was thrown by compiling Tor with GCC in pedantic mode. As you mention, this behavior is implementation-defined. To quote the document for this specific case

Otherwise, the new type is signed and the value cannot be represented in it; either the
result is implementation-defined or an implementation-defined signal is raised.

In the first case, the resulting values differ from what is expected. In cases where it matters, future developers will have to waste time again to trace these difference back to these tables. In the second case, some implementations are free to raise signals which Tor would need handle. This would require new code with the chance of introducing new bugs. To avoid all this, I think applying the patch (after changing it to use proper terminology) would prevent future headaches.

comment:4 Changed 2 years ago by nickm

Points: .2

comment:5 Changed 22 months ago by nickm

Resolution: fixed
Status: needs_revisionclosed

Merged, then tweaked the changes file. Thanks!

Note: See TracTickets for help on using tickets.