Opened 7 years ago

Closed 7 years ago

#2784 closed defect (fixed)

perfdhcp cores when given command line "-6" and v4 server address

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

Description

If you give perfdhcp mismatched protocol (-6) and server address v4 it core dumps as shown in the stdout capture below:


Running: perfdhcp -6 -l eth1 -n 5 -r 1 -x aeistT 172.16.1.1
IPv6
:
terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<std::bad_cast> >'

what(): std::bad_cast

Aborted (core dumped)
retval is 134


If you give it -4 with v6 address it gracefully handles the error:


Running: perfdhcp -4 -l eth1 -n 5 -r 1 -x aeistT fe80::20c:29ff:fea5:954a
IPv4
:
Error running perfdhcp: Can't convert fe80::20c:29ff:fea5:954a address to IPv4.
retval is 1


Subtickets

Attachments (2)

perfdhcp.out (1.3 KB) - added by tmark 7 years ago.
Output of perfdhcp showing two valid invocations and two invalid invocations
run_unittests.log (25.6 KB) - added by tmark 7 years ago.
unit test log output for perfdhcp

Download all attachments as: .zip

Change History (11)

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 assigned

Changed 7 years ago by tmark

Output of perfdhcp showing two valid invocations and two invalid invocations

Changed 7 years ago by tmark

unit test log output for perfdhcp

comment:3 Changed 7 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing

IOAddress will construct an object from a string address so long as the address is valid
whether it is v4 or a v6 address, and then it sets the IOAddress family member accordingly.
This issue here is that we have a valid address, and hence a valid IOAddress object, it just isn't the IPv family we are expecting, so when we try to treat it as v6 address bad things happen.

I added logic to perfdhcp/test_control.cc to check for a mismatch between the commnd line arguments for ip version and server:

If an IOAddress object is successfully created from the server value, then it's family is checked against the requested ip version. If they do not match it throws an InvalidParameter?
exception causing the process to exit gracefully.

I also added unit tests to verify mismatch detection.

Attached is output from perfdhcp showing proper behavior as well as perfdchp unit test logs.
The following ChangeLog? entry is suggested:

"
5xx. [bug] tmark

Perdhcp will now exit gracefully if the command line argument for Ip version
(-4 or -6) does not match the command line argument given for the server.
Prior to this perfdhcp would core when given an Ip version of -6 but a valid
IPv4 address for server.
(Trac #2784, git 2f3d1fe8c582c75c98b739e0f7b1486b0a9141ca)

"

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

Reviewed commit 2f3d1fe8c582c75c98b739e0f7b1486b0a9141ca.

tests/tools/perfdhcp/test_control.cc
openSocket: "family" is initialized in one line, then potentially reset a couple of lines later after the initialization of "sock", . Why not initialize "family" on one line:

uint8_t family = (options.getIpVersion() == 6) ? AF_INET6 : AF_INET;

"check for mismatch..." - Please start comments with a capital letter.

In the "if" test following the previously mentioned comment, the BIND 10 coding guidelines indicate that the opening brace should be on the same line as the "if".

In the "isc_throw" statement, the coding standards state that second and subsequent lines should be aligned with the opening parenthesis.

In the "isc_throw" statement, the unstated standard for BIND 10 is to use the new C++ construct: "static_cast<unsigned int>" to cast the result of options.getIpVersion(), not the old-style C++ casts.

As the message in isc_throw is printed to the user, the message should be perhaps less cryptic: perhaps something like: "The server address <address> is not an IPv<version> address as requested by the command-line switches".

The changes will require a ChangeLog entry as it is user-visible.

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

  • Owner changed from tmark to stephen

Replying to stephen:

Reviewed commit 2f3d1fe8c582c75c98b739e0f7b1486b0a9141ca.

tests/tools/perfdhcp/test_control.cc
openSocket: "family" is initialized in one line, then potentially reset a couple of lines later after the initialization of "sock", . Why not initialize "family" on one line:

uint8_t family = (options.getIpVersion() == 6) ? AF_INET6 : AF_INET;

Done.

"check for mismatch..." - Please start comments with a capital letter.

Done.

In the "if" test following the previously mentioned comment, the BIND 10 coding guidelines indicate that the opening brace

should be on the same line as the "if".

Old habits, Done.

In the "isc_throw" statement, the coding standards state that second and subsequent lines should be aligned with the opening parenthesis.

Done.

In the "isc_throw" statement, the unstated standard for BIND 10 is to use the new C++ construct: "static_cast<unsigned int>" to cast the result of options.getIpVersion(), not the old-style C++ casts.

Done. Though one can't really have a standard if it isn't stated.

As the message in isc_throw is printed to the user, the message should be perhaps less cryptic: perhaps something like: "The server address <address> is not an IPv<version> address as requested by the command-line switches".

The changes will require a ChangeLog entry as it is user-visible.

ChangeLog? entry proposed in earlier comments, repeated below:

6xx. [bug] tmark

Perdhcp will now exit gracefully if the command line argument for Ip version
(-4 or -6) does not match the command line argument given for the server.
Prior to this perfdhcp would core when given an Ip version of -6 but a valid
IPv4 address for server.
(Trac #2784, git TBD)

comment:7 Changed 7 years ago by tmark

Subsequent commit was required to fix a syntax error. I got ahead of myself. Ticket is ready for review.

comment:8 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit 30fc969527cf3ae250cc6d2ee0ec0248c4b943d0

I've made a small change to the error message and pushed. If you are OK with the change, the code is ready to merge.

The ChangeLog message is OK, but "IP" in the first and third lines should capitalised ("Internet Protocol").

comment:9 Changed 7 years ago by tmark

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

Changes merged. ChangeLog? entry fixed and added.

Note: See TracTickets for help on using tickets.