BIND 10 #3180: Kea6 is not able to handle relayed traffic from DOCSIS3.0

[BIND 10 Development]

#3180: Kea6 is not able to handle relayed traffic from DOCSIS3.0
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  defect        |  marcin
            Priority:  very high     |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20131016
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:
                                     |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:6 marcin]:
 > > '''option6_iaaddr.cc'''
 > Should I be happy that you haven't found an issue here or you forgot to
 put your comments with respect to this file? :)
 The code was so horrible that it was difficult to describe the problem. Or
 the code was so good that I had a difficulty to find the right
 superlatives to express my admiration. I'm not sure which one of those...
 ;)

 > > '''pkt6.cc'''
 > > The same comment as for pkt4. The zeros passed to first use
 > > of callback_() should be NULLs and explained what they mean.
 > Right. They can be NULL but that maps to zeros anyway?
 Yes, but typically we should use NULLs, not 0s when dealing with pointers.
 I think that Mateusz mentioned that NULLs are specific types in C++11.

 > I changed to NULLs.
 Are you sure about your change? I see pkt6.cc:336 still has two zeros.

 > > '''option_unittest.cc'''
 > > Parameters passed to boost::bind should be commented on. It is
 > > unclear what _1 to _5 mean.
 > Added a comment.
 It is helpful, but does not address the issue. Please add something like:
 {{{
 // See UnpackOptionsCallback in option.h for description of the parameter
 values.
 }}}

 > It is possible to set custom callback for either a PktX or Option. Note
 that each of these objects has to unpackOptions (they just unpack options
 on different encapsulation level). Since each of these classes come with
 different unit tests, I have to replicate the test class
 !''CustomCallback!''. I could actually share it by storing it in the
 header file which would be included by all files. But, I thought this
 class is small enough to be copied into each unit test file so as whole
 unit tests code is in a single file.
 Ok. That makes sense. Thanks.

 > > '''pkt6_unittest.cc'''
 > > packAndClone(): why do you create one packet and then create
 > > second one based on the first one? Wouldn't it be easier to just
 > > create a packet and return it?
 >
 > I have to admit I didn't think much about it when I created the new
 function. I just copied common parts of the existing tests to the new
 function. On reflection I can see that it had been like this because it
 simulates the reception of the packet when the constructor taking a
 pointer to data and data length is used. This constructor copies the
 provided buffer to data_ field (See pkt6.cc, line 48). In such case, when
 I later call unpack the, the data are taken from this buffer and parsed.
 If I use a non-cloned packet it is slightly different. I create a packet
 and add options to it. Options are held as a collection of objects. I call
 pack() and options are stored in output buffer (not data_ field), ready to
 be sent. When I call unpack() the data_ is still blank (because actual
 data is in output buffer. This results in failure to unpack() because of
 the !''truncation!''. I am not sure that the behaviour of the Pkt6 class
 is correct here but it was not my aim to fix this here.
 Can you add a comment that explains that? It is definitely not obvious
 from a casual look at the code.

 > What a shame! It seems that I didn't do full compilation, just bits that
 I modified. SOrry about it. Now it should compile just fine.
 Thanks. It complies fine now.

 > Ok. Thanks for the review! Please review again.
 Your changes look good. The outstanding things are minor. Once you do
 them, go ahead and merge. I don't have to see this ticket again.

-- 
Ticket URL: <https://bind10.isc.org/ticket/3180#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development