Opened 6 years ago

Closed 6 years ago

#3010 closed defect (fixed)

Fix signed integer overflow

Reported by: shane Owned by: shane
Priority: low Milestone: bind10-1.2-release-freeze
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Low
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 1 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

I ran the unit tests with the g++ -ftrapv compile-time option, which is documented as:

-ftrapv

This option generates traps for signed overflow on addition, subtraction, multiplication operations.

Only one test failed, in rrttl_unittests.cc:

    // Second part out of range, and will become negative with the unit,
    EXPECT_THROW(RRTTL("256S307445734561825856M"), InvalidRRTTL);

This is because in parseTTLString() we assume that signed integers overflow in a well-defined way, which is not true.

I've attached a diff which fixes this.

Subtickets

Attachments (2)

overflow.diff (2.9 KB) - added by shane 6 years ago.
0001-3010-Fix-signed-integer-overflow-in-TTL-parsing.patch (4.9 KB) - added by muks 6 years ago.
Extended the patch to do better overflow testing

Download all attachments as: .zip

Change History (9)

Changed 6 years ago by shane

comment:1 Changed 6 years ago by muks

  • Estimated Difficulty changed from 0 to 1

comment:2 Changed 6 years ago by muks

  • Milestone changed from New Tasks to Sprint-20130723

comment:3 Changed 6 years ago by muks

  • Status changed from new to reviewing

This seems to be a review ticket.

comment:4 Changed 6 years ago by muks

  • Owner changed from UnAssigned to muks

Picking for review.

comment:5 Changed 6 years ago by muks

Hi Shane

Here is my review:

  • The use of uint64_t looks good to me.
  • Why was the seconds variable introduced? It doesn't seem possible that a value would fail the newly introduced checks and pass the old checks below on value and val. In case there is, can you add a unittest for it?
  • There is a trailing whitespace in the patch.

Changed 6 years ago by muks

Extended the patch to do better overflow testing

comment:6 Changed 6 years ago by muks

  • Owner changed from muks to shane

comment:7 Changed 6 years ago by shane

  • Resolution set to fixed
  • Status changed from reviewing to closed

I've confirmed that this patch works and pushed it to master.

Note that I did not include the change to configure.ac to set the -ftrapv flag, since there is a runtime cost. Probably the correct thing is to set up one of our build farm boxes to include the -ftrapv flag on build so that when tests are run this behavior is verified. (Ticket #3318 documents this.)

I have not added a ChangeLog entry as this is entirely an internal change.

Note: See TracTickets for help on using tickets.