Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#2837 closed defect (fixed)

MySQL STRICT mode causes libdhcpsrv unit tests t fail when MySQL STRICT mode is enabled

Reported by: tmark Owned by: tmark
Priority: medium Milestone: Sprint-DHCP-20130411
Component: libdhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The following two unit tests in /src/lib/dhcpsrv/tests fail:

[ RUN ] MySqlLeaseMgrTest?.getLease4HwaddrSize
unknown file: Failure
C++ exception with description "unable to execute for <INSERT INTO
lease4(address, hwaddr, client_id, valid_lifetime, expire, subnet_id)
VALUES (?, ?, ?, ?, ?, ?)>, reason: Data too long for column 'hwaddr' at row 1
(error code 1406)" thrown in the test body.
[ FAILED ] MySqlLeaseMgrTest?.getLease4HwaddrSize (96 ms)
:
[ RUN ] MySqlLeaseMgrTest?.getLease4HwaddrSubnetIdSize
unknown file: Failure
C++ exception with description "unable to execute for <INSERT INTO
lease4(address, hwaddr, client_id, valid_lifetime, expire, subnet_id)
VALUES (?, ?, ?, ?, ?, ?)>, reason: Data too long for column 'hwaddr' at row 1
(error code 1406)" thrown in the test body.
[ FAILED ] MySqlLeaseMgrTest?.getLease4HwaddrSubnetIdSize (59 ms)
:

If MySQL STRICT mode is enabled (see STRICT_TRANSACTION_TABLES or STRICT_ALL_TABLES).
Certain versions/installations of MySQL have STRICT mode enabled by default. This
causes "out of range" values (i.e. too long) to generate an ERROR response rather than
truncate. These two tests only "pass" when STRICT mode is disabled.

Actions needed:

  1. Ensure that STRICT mode is always enabled. This may be done

using a call to mysql_options immediately after mysql_init:

mysql_options (&mysql_, "set SESSION sql_mode = 'STRICT_ALL_TABLES';");

(Should explore if there are any other modes we should set. Perhaps a configurable list?)

  1. Alter the tests such that they expect an exception.

Currently the tests add a value that is 1 too long and then expect NOT to find it. In other
words they search for HwAddress?-too-long, which should not match the value actually inserted.

  1. Lease4::HWADDR_MAX should be defined in "hwaddr" class, not in Lease4 class
  1. An attempt to create a hwaddr object should fail - probably with an InvalidParameter? exception -

if the passed-in length of the hardware address is too large.

Subtickets

Attachments (1)

trac2837-test-artifacts.tgz (18.4 KB) - added by tmark 7 years ago.
Unt test and live test artifacts

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by stephen

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130328

comment:2 Changed 7 years ago by tmark

  • Owner set to tmark
  • Status changed from new to accepted

Changed 7 years ago by tmark

Unt test and live test artifacts

comment:3 Changed 7 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from accepted to reviewing
  1. Ensure that STRICT mode is always enabled.

Modified mysql_lease_mgr to set STRICT_ALL_TABLES SQL mode:
Done. Other modes available, such as NO_ZERO_DATE do not seem to be applicable
at this time. As to making this configurable, ensuring that we have
predictable rules governing what constitutes valid data is paramount to accurate
lease behavior. I can't see nay real benefit from making the configurable. They
are not likely to be set differently from customer to customer. So for the time
being, only the STRICT_ALL_TABLES SQL mode option is set.

  1. Alter the tests such that they expect an exception.

Modified mysql_lease_mgr_unittest.cc to expect exception throws on db inserts
(addLease) with too large a value of hardware address.

  1. Lease4::HWADDR_MAX should be defined in "hwaddr" class, not in Lease4 class

Moved Lease4::HWAddr to HWAddr::MAX_HWADDR_LEN, (changed name in keeping with
apparent convention).

  1. An attempt to create a hwaddr object should fail - probably with an

InvalidParameter?? exception - if the passed-in length of the hardware address
is too large.

Modified HWAddr constructors to throw InvalidParameter? if hardware address is
too big.

Added tests to hwaddr_unittest.cc to verify new constructor logic.


Test artifacts are in attached tar ball.


ChangeLog? entry:

[5xx] [bug] tmark

Changed mysql_lease_mgr to set the SQL mode option to STRICT. This
causes mysql it to treat invalid input data as an error. Rather than
"successfully" inserting a too large value by truncating it, the
insert will fail, and the lease manager will throw an exception.
Attempts to create HWAddrs with too large a value for hardware address
now throws an exception.
(Trac #2387, git 02fb3337a339f2043183ae66062a774ae6375e83)
(Trac #2387, git ab624f466e084edecb4a6299a56f8d33282a1ba0)

comment:4 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

src/lib/dhcp/hwaddr.cc
HWAddr constructors: even single-line "if" blocks should use "{" and "}" (see the coding guidelines).

I think you can leave the initializations of the member hwaddr_ as they were (i.e. created with the passed arguments) and just have the length check in the constructor. At worst there is additional overhead of initializing hwaddr_ with the data if the exception is thrown (as opposed to the overhead of constructing hwaddr_ and copying data to it in separate steps in all cases when an exception is not thrown).

src/lib/dhcp/tests/hwaddr_unittest.cc
HWAddrTest: Please include spaces around the "+" in the initialization of big_data.

HWAddrTest: It is probably better to initialize big_data explicitly (via std::fill or memset); I recall a problem on one system (can't remember which, probably Solaris) when initializing an array with fewer elements than the size of the array. In fact, an even better solution is to create big_data_vector using:

vector<uint8_t> big_data_vector(HWAddr::MAX_HWADDR_LEN + 1, 0);

(although the "0" is not really necessary - it's the default) and, when testing the constructor that requires a base address and size, pass &big_data_vector[0] and big_data_vector.size() through as arguments.

src/lib/dhcpsrv/mysql_lease_mgr.cc
openDatabase: Suggest that the "set" in the command setting the session variable be uppercase (matches the style of the prepared statements).

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
Line 884/906: not sure about the reformatting here. The BIND 10 standard for continuation lines is the BSD style.

ChangeLog entry
Would rewrite the last sentence as:

Also, attempts to create a HWAddr (hardware address) object with
too long an array of data now throw an exception.

Only needs one git commit number, that of the commit in which the change has been merged into master.

comment:6 in reply to: ↑ 5 Changed 7 years ago by tmark

  • Owner changed from tmark to stephen

Replying to stephen:

src/lib/dhcp/hwaddr.cc
HWAddr constructors: even single-line "if" blocks should use "{" and "}" (see the coding guidelines).

Fixed.

I think you can leave the initializations of the member hwaddr_ as they were (i.e. created with the passed arguments) and just have the length check in the constructor. At worst there is additional overhead of initializing hwaddr_ with the data if the exception is thrown (as opposed to the overhead of constructing hwaddr_ and copying data to it in separate steps in all cases when an exception is not thrown).

Agreed, Fixed.

src/lib/dhcp/tests/hwaddr_unittest.cc
HWAddrTest: Please include spaces around the "+" in the initialization of big_data.

HWAddrTest: It is probably better to initialize big_data explicitly (via std::fill or memset); I recall a problem on one system (can't remember which, probably Solaris) when initializing an array with fewer elements than the size of the array. In fact, an even better solution is to create big_data_vector using:

vector<uint8_t> big_data_vector(HWAddr::MAX_HWADDR_LEN + 1, 0);

(although the "0" is not really necessary - it's the default) and, when testing the constructor that requires a base address and size, pass &big_data_vector[0] and big_data_vector.size() through as arguments.

Good catch. Done.

src/lib/dhcpsrv/mysql_lease_mgr.cc
openDatabase: Suggest that the "set" in the command setting the session variable be uppercase (matches the style of the prepared statements).

Done.

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
Line 884/906: not sure about the reformatting here. The BIND 10 standard for continuation lines is the BSD style.

Adjusted per guidelines.

ChangeLog entry
Would rewrite the last sentence as:

Also, attempts to create a HWAddr (hardware address) object with
too long an array of data now throw an exception.

Only needs one git commit number, that of the commit in which the change has been merged into master.

Marcin, corrected me on this on an earlier ticket. Will use only the merge commit.

ChangeLog? text will be:

Changed mysql_lease_mgr to set the SQL mode option to STRICT. This
causes mysql it to treat invalid input data as an error. Rather than
"successfully" inserting a too large value by truncating it, the
insert will fail, and the lease manager will throw an exception.
Also, attempts to create a HWAddr (hardware address) object with
too long an array of data now throw an exception.
(Trac #2387, git TBD)

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit 2b7e75de166cb416a60080ad34d8acbed4fd8119

I've fixed some small omissions (add braces to a single-line "if" statement, corrected some comments and removed some trailing space) and pushed.

If you are happy with these changes, please merge - all is OK now (including the ChangeLog entry).

comment:8 Changed 7 years ago by tmark

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

Changes merged. ChangeLog? entry made.

comment:9 Changed 6 years ago by stephen

  • Sub-Project changed from DNS to DHCP
Note: See TracTickets for help on using tickets.