Opened 6 years ago

Closed 6 years ago

#3180 closed defect (fixed)

Kea6 is not able to handle relayed traffic from DOCSIS3.0

Reported by: tomek Owned by: marcin
Priority: very high Milestone: Sprint-DHCP-20131016
Component: dhcp6 Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Kea6 has to be able to handle relayed traffic from cable modem connected to CMTS.
Currently it does not. Ticket #3177 introduced Dhcpv6SrvTest.docsisTraffic, which fails.

See src/bin/dhcp6/tests/dhcp6_srv_unittest.cc. It seems that the server fails on parsing incoming buffer. This a traffic capture from a real cable network. See attached traffic capture for details.

Set priority to very high as it blocks upcoming demo.

Subtickets

Attachments (1)

dhcpv6_relay_forw-vm.pcap (581 bytes) - added by tomek 6 years ago.

Download all attachments as: .zip

Change History (9)

Changed 6 years ago by tomek

comment:1 Changed 6 years ago by tomek

  • Milestone changed from Sprint-DHCP-20130918 to Sprint-DHCP-20131016

comment:2 Changed 6 years ago by marcin

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

Changelog:

6XX.	[func]*		marcin
	b10-dhcp4 and b10-dhcp6 install callback functions which parse options
	in the received DHCP packets.
	(Trac #3180, git ABCD)

This ticket resolves the problem whereby the generic libdhcp++ functions which parse packet options have no access to the option definitions defined on the server side. The server now provides a custom implementation of the options' parsing algorithm by installing a callback. This callback is called from within a Pkt4, Pkt6 and Option class. If callback is not installed the libdhcp++ functions are used instead (as previously).

Please review.

comment:3 Changed 6 years ago by marcin

  • Status changed from assigned to reviewing

comment:4 Changed 6 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:5 follow-up: Changed 6 years ago by tomek

  • Owner changed from tomek to marcin

Reviewing changes on ff3ddd8332fbaab5ff76f140ae9f185a2d2d6e34.

option.h
OptionCollection - why is it for DHCPv6 options only ? It is perfectly
capable to store v4 options. If it's intended for v6 only, there should
be a comment that explains that.

If OptionCollection is really v6 only, perhaps it should be
renamed for Option6Collection?

UpackOptionsCallback - why the parameters are not named? Those should
be named and documented (or if naming the parameters create warnings,
at least they should be documented by number, e.g. parameter 1
specifes option buffer to be parsed, parameter 2 specifies ...)

option6_iaaddr.cc

option_int.h
It is fine to keep the option constructors as they are for now,
but I think the constructors should be extended to cover
other option spaces one day, e.g. vendor option. I think that
adding a @todo about it now would be sufficient, but I won't
object if you have other plans for handling interger suboptions
within vendor-option.

pkt4.cc
The comment in line 214 is wrong and useless. This is not the
first use of readVector(). It would be better to explain why
the readVector has to be used. For the callback_ call in line
219 it would be useful to explain why the the last 2 arguments
passed are zeros. And shouldn't they be NULLs, not zeros? Those
arguments are pointers.

pkt6.cc
The same comment as for pkt4. The zeros passed to first use
of callback_() should be NULLs and explained what they mean.

option_unittest.cc
OptionTest.unpackCallback(): Comments for opt_data are wrong.
length is 2 for the first option, length for the second
option is not specified. The content of second option should
be different than the first one.

Parameters passed to boost::bind should be commented on. It is
unclear what _1 to _5 mean.

pkt4_unittest.cc
Generic comment: why is CustomCallback defined in option_unittest.cc
and in pkt4_unittest.cc? I would expect it to be either in
pkt4_ and pkt6_ or both defined in option_unittest.cc which is
common for v4 and v6. There's no need to change anything, I'm just
curious.

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?

dhcp4_srv.cc
Dhcpv4Srv::unpackOptions() why the check is for dhcp6? This is v4
code. If that is correct, an explanation comment is required. This
looks like a bug.

On a related note, this is not picked up by unit-tests. We don't want
to spend any more time on this, so please just add a todo that more
thorough tests for unpackOptions() that also cover dhcp4 option space
are needed.

Dhcv4Srv::unpackOptions() - parameters are not indented correctly
(one space too far)


The code does not compile:

make[5]: Wejście do katalogu `/home/thomson/devel/bind10-clean/tests/tools/perfdhcp'
  CXX    perfdhcp-perf_pkt6.o
  CXX    perfdhcp-perf_pkt4.o
  CXX    perfdhcp-pkt_transform.o
  CXX    perfdhcp-test_control.o
In file included from pkt_transform.cc:22:0:
pkt_transform.h:69:28: error: ‘OptionCollection’ in ‘class isc::dhcp::Option’ does not name a type
pkt_transform.h:69:60: error: ISO C++ forbids declaration of ‘options’ with no type [-fpermissive]
pkt_transform.h:91:30: error: ‘OptionCollection’ in ‘class isc::dhcp::Option’ does not name a type
pkt_transform.h:91:62: error: ISO C++ forbids declaration of ‘options’
with no type [-fpermissive]

Many more errors follow.

Changelog proposal looks good.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Replying to tomek:

Reviewing changes on ff3ddd8332fbaab5ff76f140ae9f185a2d2d6e34.

option.h
OptionCollection - why is it for DHCPv6 options only ? It is perfectly
capable to store v4 options. If it's intended for v6 only, there should
be a comment that explains that.

If OptionCollection is really v6 only, perhaps it should be
renamed for Option6Collection?

Actually I copied over the typedef along with its description from the interiors of the Option class to outside of the class. So, the comment had been there for a while. I now fixed it.

UpackOptionsCallback - why the parameters are not named? Those should
be named and documented (or if naming the parameters create warnings,
at least they should be documented by number, e.g. parameter 1
specifes option buffer to be parsed, parameter 2 specifies ...)

Good point. Fixed.

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? :)

option_int.h
It is fine to keep the option constructors as they are for now,
but I think the constructors should be extended to cover
other option spaces one day, e.g. vendor option. I think that
adding a @todo about it now would be sufficient, but I won't
object if you have other plans for handling interger suboptions
within vendor-option.

True. I didn't want to mess up in constructors and just provided the modifier function: ''SetEncapsulatedSpace?'' if someone wants to set the space name after creation of the object. I added a @todo for this if we decide that we want to add this parameter to the constructor.

pkt4.cc
The comment in line 214 is wrong and useless. This is not the
first use of readVector(). It would be better to explain why
the readVector has to be used. For the callback_ call in line
219 it would be useful to explain why the the last 2 arguments
passed are zeros. And shouldn't they be NULLs, not zeros? Those
arguments are pointers.

This comment had been there for a while so it is not really related to this ticket. I changed the comment.

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? I changed to NULLs.

option_unittest.cc
OptionTest.unpackCallback(): Comments for opt_data are wrong.
length is 2 for the first option, length for the second
option is not specified. The content of second option should
be different than the first one.

Fixed.

Parameters passed to boost::bind should be commented on. It is
unclear what _1 to _5 mean.

Added a comment.

pkt4_unittest.cc
Generic comment: why is CustomCallback defined in option_unittest.cc
and in pkt4_unittest.cc? I would expect it to be either in
pkt4_ and pkt6_ or both defined in option_unittest.cc which is
common for v4 and v6. There's no need to change anything, I'm just
curious.

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.

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.

dhcp4_srv.cc
Dhcpv4Srv::unpackOptions() why the check is for dhcp6? This is v4
code. If that is correct, an explanation comment is required. This
looks like a bug.

Yes it is.

On a related note, this is not picked up by unit-tests. We don't want
to spend any more time on this, so please just add a todo that more
thorough tests for unpackOptions() that also cover dhcp4 option space
are needed.

Right. The test covers option spaces other than standard option space and it should have been implemented. I added a todo comment.

Dhcv4Srv::unpackOptions() - parameters are not indented correctly
(one space too far)

Fixed.


The code does not compile:

make[5]: Wejście do katalogu `/home/thomson/devel/bind10-clean/tests/tools/perfdhcp'
  CXX    perfdhcp-perf_pkt6.o
  CXX    perfdhcp-perf_pkt4.o
  CXX    perfdhcp-pkt_transform.o
  CXX    perfdhcp-test_control.o
In file included from pkt_transform.cc:22:0:
pkt_transform.h:69:28: error: ‘OptionCollection’ in ‘class isc::dhcp::Option’ does not name a type
pkt_transform.h:69:60: error: ISO C++ forbids declaration of ‘options’ with no type [-fpermissive]
pkt_transform.h:91:30: error: ‘OptionCollection’ in ‘class isc::dhcp::Option’ does not name a type
pkt_transform.h:91:62: error: ISO C++ forbids declaration of ‘options’
with no type [-fpermissive]

Many more errors follow.

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.

Changelog proposal looks good.

Ok. Thanks for the review! Please review again.

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

  • Owner changed from tomek to marcin

Replying to 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.

comment:8 Changed 6 years ago by marcin

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

Merged with commit b080f05c590ce7e24c38db62194c85889a321ae1

Note: See TracTickets for help on using tickets.