Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1313 closed enhancement (fixed)

writeUint32 and readUint32 are needed

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20111026
Component: dhcp 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

Similar to writeUint16 and readUint32, in lib/util/io_utilities.h

DHCP code could use them.

Subtickets

Change History (8)

comment:1 Changed 8 years ago by tomek

Lack of this code causes SIGBUS errors on Solaris 10, when compiled with gcc. In particular, 3 tests had to be disabled:

Option6IATest.suboptions_pack
Option6IAAddrTest.basic
Pkt6Test.unpack_solicit1

They must be reenabled once underlying problem (misaligned reads) is solved.

comment:2 Changed 8 years ago by tomek

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Implemented readUint32 and writeUint32.
Modified writeUint16 to return pointer to the next byte after stored value.
Implemented 2 new tests for those functions.
Enabled 3 DHCP tests that caused SIGBUS on Solaris 10 (Sparc).

Checked that tests pass on Ubuntu 11.04 (x86) ans Solaris 10 (Sparc).

Code ready for review.

comment:3 Changed 8 years ago by jinmei

Not fully looked at it (or probably I'm not supposed to look at it)
but just happened to notice this, so I'll make some comments anyway...

IMO the writeXXX interface is very dangerous to use in that it
accepts a bare pointer without any information of the valid size
of the range. While it might look relatively safer due to the
fact that the modified size is fixed, and while it's true even if
we pass the valid range a buggy/evil caller could still cause a
disaster, IMO this type of dangerous interfaces shouldn't be used in
higher level code such as protocol handling.

We have much safer abstraction of buffers:
isc::util::Input/OutputBuffer?. I strongly suggest using the safer
interfaces. (And I actually plan to open a ticket to deprecate the
dangerous ones).

comment:4 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to tomek

Responding to jinmei:

We have much safer abstraction of buffers: isc::util::Input/OutputBuffer. I strongly suggest using the safer interfaces.

The original read/writeUintXX were for tests which, on reflection, could probably be implemented without it.

Input/OutputBuffer is being used for new code and it is likely that the existing code will be refactored in this way.

And I actually plan to open a ticket to deprecate the dangerous ones

Dangerous is probably too strong a word - an unexploded bomb is dangerous, this interface is perhaps just unsafe? :-)

As to the review:

src/lib/util/io_utilities.h
Header comments are wrong - the buffer is assumed to be four bytes long, not two bytes.

src/lib/util/tests/io_utilities_unittest.cc
Should be spaces around binary operators (e.g. "offset < 4" instead of "offset<4").

Should be no spaces (except in exceptional circumstances) after a "(" and before a ")".

readUint32 test: the index of the inner loop (i) ranges between 0 and sizeof(test32). However, i is then used as an index into test32. As sizeof(test32) is in bytes, and test32 is a uint32_t array, this means that for values of i > 4, the loop is accessing elements beyond the end of the array. The end value of the inner loop should be sizeof(test32) / sizeof(uint32_t).

The same comment applies for the writeUint32 test.

writeUint32 test: suggest the final assertion be written as:

EXPECT_EQ(0, memcmp(...))

Although I know that 0 tests as false in C/C++, the impression of the line containing the EXPECT_FALSE is that the memory comparison is expected to fail.

comment:5 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

Thanks for review. Changes commited to trac1313.

comment:6 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

All seems OK, please merge.

comment:7 Changed 8 years ago by tomek

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

Merged to master.
Resolving.

comment:8 Changed 8 years ago by stephen

  • Milestone changed from DHCP 2011 to Sprint-DHCP-20111026
Note: See TracTickets for help on using tickets.