Opened 7 years ago

Closed 7 years ago

#2272 closed defect (fixed)

Improve the perfdhcp command_options_helper code.

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20121004
Component: Unclassified 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

After fixing the memory leak reported by Valgrind (commit 8ad1accced4284ff727b03c43cab755566e486ab) there is a repeating unit tests failure on macmini.lab with GCC compiler:
http://git.bind10.isc.org/~tester/builder/BIND10/20120918045429-MacOSX10.6-x86_64-GCC/logs/unittests.out

It has been investigated that changing malloc()/free() to new/delete makes unit test pass however it was not fully rootcaused. BIND10 dev team indicated during a quick review of proposed change that command_options_helper code should be improved (exception safety, strcpy safety etc). Also it should be better explained why the change from malloc to new makes a difference.

Subtickets

Change History (10)

comment:1 Changed 7 years ago by marcin

  • Milestone changed from DHCP 2012 to Sprint-DHCP-20121004

comment:2 Changed 7 years ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from new to assigned

The command_options_unit tests prepare certain number of perfdhcp command lines in the string format like "perfdhcp -l eth0 -4 all". Each new command line is later tokenized and stored in argc, argv variables. The argc, argv is allocated for each new command line being tested in command_options_helper.h by CommandOptionsHelper::tokenizeString() and then it is passed to CommandOptions::parse() for parsing.

The getopt() function used in parse() function keeps its internal state including optind, opterr etc. during parsing subsequent arguments of the command line. It also appears that getopt() remembers the pointer of the previously used argv buffer and if getopt() state is not properly reset before parsing new command line it will refer to freed memory and that will eventually lead to undefined behavior and output from getopt().

There are at least two ways to reset the state of the getopt() to prevent it from trying to access freed buffers. On Linux: the optind must be set to 0. This however does not work on BSD and MacOS where there is an additional variable declared: optreset. On those systems both optreset and optind must be set to 1.

The key of the change being done here is to check if optreset is available on the OS we are at and if so, define the HAVE_OPTRESET so as the code can take different reset path based on this. The code also now uses new/delete instead of malloc/free to allocate the argv table which is more proper when programming in C++. Some additional minor changes have been made.

Please review.

comment:3 Changed 7 years ago by marcin

  • Status changed from assigned to reviewing

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 marcin

Reviewed commit 3148cf2b018b222e263894b80cd7e92b20c86ceb

configure.ac
Missing semicolon at the end of the "optreset=1" statement in the AC_TRY_LINK code for checking optreset. (Although according to my config.log, configure added one anyway.)

tests/tools/perfdhcp/tests/command_options_helper.h
The "results" array allocated is too long: the number of elements in the array should not be multiplied by "sizeof(char*)".

The CommandOptionsHelper class is only used in unit tests using Gtest. With this in mind, when allocating memory with "new", rather than using "new (std::nothrow)" then an assert() to check that the returned result is non-null, why not write a standard "new" within an EXPECT_NO_THROW?

tests/tools/perfdhcp/tests/command_options_helper.cc

There are at least two ways to reset the state of the getopt() to prevent it from trying to access freed buffers. On Linux: the optind must be set to 0. This however does not work on BSD and MacOS where there is an additional variable declared: optreset. On those systems both optreset and optind must be set to 1.

This seems to imply that both optind and optreset must be set to 1 on BSD systems only (i.e. with HAVE_OPTRESET set to 1). However the code only sets optreset according to this preprocessor symbol; the setting of optind is determined by the macro __GLIBC__.

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

  • Owner changed from marcin to stephen

Replying to stephen:

Reviewed commit 3148cf2b018b222e263894b80cd7e92b20c86ceb

configure.ac
Missing semicolon at the end of the "optreset=1" statement in the AC_TRY_LINK code for checking optreset. (Although according to my config.log, configure added one anyway.)

Corrected.

tests/tools/perfdhcp/tests/command_options_helper.h
The "results" array allocated is too long: the number of elements in the array should not be multiplied by "sizeof(char*)".

ooops! That looks like typical mistake I make when I replace malloc with new.

The CommandOptionsHelper class is only used in unit tests using Gtest. With this in mind, when allocating memory with "new", rather than using "new (std::nothrow)" then an assert() to check that the returned result is non-null, why not write a standard "new" within an EXPECT_NO_THROW?

That would work as well but my intention was to separate errors that come purely from invocation of CommandOptions::parse() from errors that come from helper methods, which are only to prepare the input to parse(). If I follow your suggestion I would get the error similar to this when new fails:

test_control_unittest.cc:633: Failure
Expected: processCmdLine("perfdhcp -l 127.0.0.1 all") doesn't throw an exception.
  Actual: it throws.

which tells me nothing where the error comes from and since failure to allocate memory with new would be very rare, my intuition would tell me that it is exception produced by CommandOptions::parse (which would not be true). Apart from this all tests will be run and each will end be interrupted by ASSERT_NO_THROW because it is unlikely that if allocation issue occurs for the first time it will not occur later.

If I leave as it is now, I get:

lt-run_unittests: command_options_helper.h:119: static char** isc::perfdhcp::CommandOptionsHelper::tokenizeString(const string&, int&): Assertion `results == __null' failed.

and I immediately know that it is the error in helper function, not in the tested class. In addition unit testing breaks (and I belive it should) as this is a fatal error.

tests/tools/perfdhcp/tests/command_options_helper.cc

There are at least two ways to reset the state of the getopt() to prevent it from trying to access freed buffers. On Linux: the optind must be set to 0. This however does not work on BSD and MacOS where there is an additional variable declared: optreset. On those systems both optreset and optind must be set to 1.

This seems to imply that both optind and optreset must be set to 1 on BSD systems only (i.e. with HAVE_OPTRESET set to 1). However the code only sets optreset according to this preprocessor symbol; the setting of optind is determined by the macro __GLIBC__.

I am not sure if I understand the comment.
If HAVE_OPTRESET is defined it means that optreset variable exists and I can set it. If it is undefined I can't set it. If it exists the only value I should set it to is 1 (there is no other value I should be setting it to). I don't set optind in HAVE_OPTRESET clause because I am setting it if GLIBC is undefined. If I am working on BSD the GLIBC will be undefined which means that optind will be set to 1, then optreset set to 1 which is what I want. If GLIBC is there the optind must be set to 0 and then there is no HAVE_OPTRESET because GLIBC does not need optreset (because it resets through optind=0). I think, this code is going to work in each case.

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

  • Owner changed from stephen to marcin

Reviewed commit 3148cf2b018b222e263894b80cd7e92b20c86ceb

Corrections to the code are OK.

That would work as well but my intention was to separate errors that come purely from invocation of CommandOptions::parse() from errors that come from helper methods, which are only to prepare the input to parse().

I wouldn't worry about that - "new" is used all over the place, both in tests and in the code, so the extra information as to what exception was thrown is far outweighed (IMHO) by the non-standard use of "new". If memory is really so short, it would probably show up in other tests anyway.

I am not sure if I understand the comment

Your original comment seemed to state:

  • on BSD/MacOS, optreset is available. optreset must be set to 1. In addition, optind must be set to 1.
  • on Linux, there is no optreset. optind must be set to 0.

The final case, which is not mentioned is neither BSD, MacOS or Linux (e.g. Solaris). Assuming that optind is set to 0 in these cases, the logic is:

#ifdef HAVE_OPTRESET
optind = 1;
optreset = 1;

#else
optind = 0;

#endif

i.e. there does not need to be a dependency on __GLIBC__.

comment:8 in reply to: ↑ 7 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Replying to stephen:

Reviewed commit 3148cf2b018b222e263894b80cd7e92b20c86ceb

Corrections to the code are OK.

That would work as well but my intention was to separate errors that come purely from invocation of CommandOptions::parse() from errors that come from helper methods, which are only to prepare the input to parse().

I wouldn't worry about that - "new" is used all over the place, both in tests and in the code, so the extra information as to what exception was thrown is far outweighed (IMHO) by the non-standard use of "new". If memory is really so short, it would probably show up in other tests anyway.

Ok. The code will now throw exception if new fails to allocate memory. The missing part is ASSERT_NO_THROW checks on call to process() in command_options_unittest.cc. I haven't added it here because it has been added with #1960 so once we merge both trac2272 and trac1960 to master it will be there.

I am not sure if I understand the comment

Your original comment seemed to state:

  • on BSD/MacOS, optreset is available. optreset must be set to 1. In addition, optind must be set to 1.
  • on Linux, there is no optreset. optind must be set to 0.

The final case, which is not mentioned is neither BSD, MacOS or Linux (e.g. Solaris). Assuming that optind is set to 0 in these cases, the logic is:

#ifdef HAVE_OPTRESET
optind = 1;
optreset = 1;

#else
optind = 0;

#endif

i.e. there does not need to be a dependency on __GLIBC__.

I checked that this will not work on Solaris 10. Solaris does not support optreset. Also setting optind=0 results in seg fault. The optind=0 is not a rule but it is the exception that exists for GLIBC only and is not portable.

comment:9 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 3d13f1625ffb635f629944e8a1c0e793804d0112

All OK, please merge.

comment:10 Changed 7 years ago by marcin

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

Merged.

Note: See TracTickets for help on using tickets.