BIND 10 trac2902, updated. 5162bd7739ccfab83a0e81e44ba5e46314eae0cf [2902] Changes as a result of the review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue May 21 08:45:45 UTC 2013
The branch, trac2902 has been updated
via 5162bd7739ccfab83a0e81e44ba5e46314eae0cf (commit)
from 9c2f41d0ce4250623b36966eb04698d55eac7509 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 5162bd7739ccfab83a0e81e44ba5e46314eae0cf
Author: Marcin Siodelski <marcin at isc.org>
Date: Tue May 21 10:45:22 2013 +0200
[2902] Changes as a result of the review.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/dhcp4_srv.cc | 9 ++-
src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 51 ++++++++++++----
src/lib/dhcp/iface_mgr.cc | 5 +-
src/lib/dhcp/iface_mgr.h | 76 ++++++++++++-----------
src/lib/dhcp/iface_mgr_bsd.cc | 3 +-
src/lib/dhcp/iface_mgr_linux.cc | 6 +-
src/lib/dhcp/iface_mgr_sun.cc | 3 +-
src/lib/dhcp/pkt4.cc | 9 ++-
src/lib/dhcp/pkt_filter.h | 1 +
src/lib/dhcp/pkt_filter_inet.cc | 2 +-
src/lib/dhcp/pkt_filter_lpf.cc | 94 ++++++++++++++++++++---------
src/lib/dhcp/protocol_util.cc | 18 ++++--
src/lib/dhcp/protocol_util.h | 25 ++++++--
src/lib/dhcp/tests/iface_mgr_unittest.cc | 1 -
src/lib/dhcp/tests/pkt4_unittest.cc | 4 +-
15 files changed, 203 insertions(+), 104 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 426b26b..3eb187b 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -63,7 +63,7 @@ Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const char* dbconfig, const bool use_bcast,
try {
// First call to instance() will create IfaceMgr (it's a singleton)
// it may throw something if things go wrong.
- // The 'true' value of in the call to setMatchingPacketFilter imposes
+ // The 'true' value of the call to setMatchingPacketFilter imposes
// that IfaceMgr will try to use the mechanism to respond directly
// to the client which doesn't have address assigned. This capability
// may be lacking on some OSes, so there is no guarantee that server
@@ -373,7 +373,12 @@ Dhcpv4Srv::copyDefaultFields(const Pkt4Ptr& question, Pkt4Ptr& answer) {
}
// If src/dest HW addresses are used by the packet filtering class
- // we need to copy them as well.
+ // we need to copy them as well. There is a need to check that the
+ // address being set is not-NULL because an attempt to set the NULL
+ // HW would result in exception. If these values are not set, the
+ // the default HW addresses (zeroed) should be generated by the
+ // packet filtering class when creating Ethernet header for
+ // outgoing packet.
HWAddrPtr src_hw_addr = question->getLocalHWAddr();
if (src_hw_addr) {
answer->setLocalHWAddr(src_hw_addr);
diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
index 0a19e6e..8591eef 100644
--- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
@@ -48,12 +48,26 @@ public:
/// @brief Constructor.
///
- /// It disables configuration of broadcast options on
- /// sockets that are opened by the Dhcpv4Srv constructor.
- /// Also, disables the Direct V4 traffic as it requires
- /// use of raw sockets. Use of broadcast as well as raw
- /// sockets require root privileges, thus can't be used
- /// in unit testing.
+ /// This constructor disables default modes of operation used by the
+ /// Dhcpv4Srv class:
+ /// - Send/receive broadcast messages through sockets on interfaces
+ /// which support broadcast traffic.
+ /// - Direct DHCPv4 traffic - communication with clients which do not
+ /// have IP address assigned yet.
+ ///
+ /// Enabling these modes requires root privilges so they must be
+ /// disabled for unit testing.
+ ///
+ /// Note, that disabling broadcast options on sockets does not impact
+ /// the operation of these tests because they use local loopback
+ /// interface which doesn't have broadcast capability anyway. It rather
+ /// prevents setting broadcast options on other (broadcast capable)
+ /// sockets which are opened on other interfaces in Dhcpv4Srv constructor.
+ ///
+ /// The Direct DHCPv4 Traffic capability can be disabled here because
+ /// it is tested with PktFilterLPFTest unittest. The tests which belong
+ /// to PktFilterLPFTest can be enabled on demand when root privileges can
+ /// be guaranteed.
NakedDhcpv4Srv(uint16_t port = 0)
: Dhcpv4Srv(port, "type=memfile", false, false) {
}
@@ -373,26 +387,41 @@ public:
const IOAddress& client_addr,
const IOAddress& relay_addr) {
+ // Create an instance of the tested class.
boost::scoped_ptr<NakedDhcpv4Srv> srv(new NakedDhcpv4Srv(0));
+
+ // Initialize the source HW address.
vector<uint8_t> mac(6);
for (int i = 0; i < 6; i++) {
- mac[i] = i*10;
+ mac[i] = i * 10;
}
-
+ // Initialized the destination HW address.
vector<uint8_t> dst_mac(6);
- for (int i = 0; i < 6; i++) {
+ for (int i = 0; i < 6; ++i) {
dst_mac[i] = i * 20;
}
-
+ // Create a DHCP message. It will be used to simulate the
+ // incoming message.
boost::shared_ptr<Pkt4> req(new Pkt4(msg_type, 1234));
+ // Create a response message. It will hold a reponse packet.
+ // Initially, set it to NULL.
boost::shared_ptr<Pkt4> rsp;
-
+ // Set the name of the interface on which packet is received.
req->setIface("eth0");
+ // Set the interface index. It is just a dummy value and will
+ // not be interprented.
req->setIndex(17);
+ // Set the target HW address. This value is normally used to
+ // construct the data link layer header.
req->setRemoteHWAddr(1, 6, dst_mac);
+ // Set the HW address. This value is set on DHCP level (in chaddr).
req->setHWAddr(1, 6, mac);
+ // Set local HW address. It is used to construct the data link layer
+ // header.
req->setLocalHWAddr(1, 6, mac);
+ // Set target IP address.
req->setRemoteAddr(IOAddress(client_addr));
+ // Set relay address.
req->setGiaddr(relay_addr);
// We are going to test that certain options are returned
diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc
index 93e6205..1e97205 100644
--- a/src/lib/dhcp/iface_mgr.cc
+++ b/src/lib/dhcp/iface_mgr.cc
@@ -220,9 +220,10 @@ IfaceMgr::setPacketFilter(const boost::shared_ptr<PktFilter>& packet_filter) {
const Iface::SocketCollection& sockets = iface->getSockets();
for (Iface::SocketCollection::const_iterator sock = sockets.begin();
sock != sockets.end(); ++sock) {
- // There is at least one socket open, so we have to fail.
if (sock->family_ == AF_INET) {
- isc_throw(PacketFilterChangeDenied, "it is not allowed to set new packet"
+ // There is at least one socket open, so we have to fail.
+ isc_throw(PacketFilterChangeDenied,
+ "it is not allowed to set new packet"
<< " filter when there are open IPv4 sockets - need"
<< " to close them first");
}
diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h
index 338f8eb..35b4dc0 100644
--- a/src/lib/dhcp/iface_mgr.h
+++ b/src/lib/dhcp/iface_mgr.h
@@ -88,7 +88,7 @@ struct SocketInfo {
};
-/// @brief represents a single network interface
+/// @brief Represents a single network interface
///
/// Iface structure represents network interface with all useful
/// information, like name, interface index, MAC address and
@@ -96,13 +96,20 @@ struct SocketInfo {
class Iface {
public:
- /// maximum MAC address length (Infiniband uses 20 bytes)
+ /// Maximum MAC address length (Infiniband uses 20 bytes)
static const unsigned int MAX_MAC_LEN = 20;
- /// type that defines list of addresses
+ /// Type that defines list of addresses
typedef std::vector<isc::asiolink::IOAddress> AddressCollection;
- /// type that holds a list of socket informations
+ /// @brief Type that holds a list of socket information.
+ ///
+ /// @warning The type of the container used here must guarantee
+ /// that the iterators do not invalidate when erase() is called.
+ /// This is because, the \ref closeSockets function removes
+ /// elements selectively by calling erase on the element to be
+ /// removed and further iterates through remaining elements.
+ ///
/// @todo: Add SocketCollectionConstIter type
typedef std::list<SocketInfo> SocketCollection;
@@ -167,7 +174,7 @@ public:
/// @brief Sets flag_*_ fields based on bitmask value returned by OS
///
- /// Note: Implementation of this method is OS-dependent as bits have
+ /// @note Implementation of this method is OS-dependent as bits have
/// different meaning on each OS.
///
/// @param flags bitmask value returned by OS in interface detection
@@ -258,53 +265,53 @@ public:
const SocketCollection& getSockets() const { return sockets_; }
protected:
- /// socket used to sending data
+ /// Socket used to send data.
SocketCollection sockets_;
- /// network interface name
+ /// Network interface name.
std::string name_;
- /// interface index (a value that uniquely indentifies an interface)
+ /// Interface index (a value that uniquely indentifies an interface).
int ifindex_;
- /// list of assigned addresses
+ /// List of assigned addresses.
AddressCollection addrs_;
- /// link-layer address
+ /// Link-layer address.
uint8_t mac_[MAX_MAC_LEN];
- /// length of link-layer address (usually 6)
+ /// Length of link-layer address (usually 6).
size_t mac_len_;
- /// hardware type
+ /// Hardware type.
uint16_t hardware_type_;
public:
/// @todo: Make those fields protected once we start supporting more
/// than just Linux
- /// specifies if selected interface is loopback
+ /// Specifies if selected interface is loopback.
bool flag_loopback_;
- /// specifies if selected interface is up
+ /// Specifies if selected interface is up.
bool flag_up_;
- /// flag specifies if selected interface is running
- /// (e.g. cable plugged in, wifi associated)
+ /// Flag specifies if selected interface is running
+ /// (e.g. cable plugged in, wifi associated).
bool flag_running_;
- /// flag specifies if selected interface is multicast capable
+ /// Flag specifies if selected interface is multicast capable.
bool flag_multicast_;
- /// flag specifies if selected interface is broadcast capable
+ /// Flag specifies if selected interface is broadcast capable.
bool flag_broadcast_;
- /// interface flags (this value is as is returned by OS,
- /// it may mean different things on different OSes)
+ /// Interface flags (this value is as is returned by OS,
+ /// it may mean different things on different OSes).
uint32_t flags_;
};
-/// @brief handles network interfaces, transmission and reception
+/// @brief Handles network interfaces, transmission and reception.
///
/// IfaceMgr is an interface manager class that detects available network
/// interfaces, configured addresses, link-local addresses, and provides
@@ -312,7 +319,7 @@ public:
///
class IfaceMgr : public boost::noncopyable {
public:
- /// defines callback used when commands are received over control session
+ /// Defines callback used when commands are received over control session.
typedef void (*SessionCallback) (void);
/// @brief Packet reception buffer size
@@ -328,7 +335,7 @@ public:
// 2 maps (ifindex-indexed and name-indexed) and
// also hide it (make it public make tests easier for now)
- /// type that holds a list of interfaces
+ /// Type that holds a list of interfaces.
typedef std::list<Iface> IfaceCollection;
/// IfaceMgr is a singleton class. This method returns reference
@@ -401,11 +408,10 @@ public:
/// @return a socket descriptor
uint16_t getSocket(const isc::dhcp::Pkt4& pkt);
- /// debugging method that prints out all available interfaces
+ /// Debugging method that prints out all available interfaces.
///
/// @param out specifies stream to print list of interfaces to
- void
- printIfaces(std::ostream& out = std::cout);
+ void printIfaces(std::ostream& out = std::cout);
/// @brief Sends an IPv6 packet.
///
@@ -563,7 +569,7 @@ public:
const bool use_bcast = true);
/// @brief Closes all open sockets.
- /// Is used in destructor, but also from Dhcpv4_srv and Dhcpv6_srv classes.
+ /// Is used in destructor, but also from Dhcpv4Srv and Dhcpv6Srv classes.
void closeSockets();
/// @brief Closes all IPv4 or IPv6 sockets.
@@ -587,7 +593,7 @@ public:
/// @throw BadValue if family value is different than AF_INET or AF_INET6.
void closeSockets(const uint16_t family);
- /// @brief returns number of detected interfaces
+ /// @brief Returns number of detected interfaces.
///
/// @return number of detected interfaces
uint16_t countIfaces() { return ifaces_.size(); }
@@ -618,7 +624,7 @@ public:
///
/// @throw InvalidPacketFilter if provided packet filter object is NULL.
/// @throw PacketFilterChangeDenied if there are open IPv4 sockets
- void setPacketFilter(const boost::shared_ptr<PktFilter>& packet_filter);
+ void setPacketFilter(const PktFilterPtr& packet_filter);
/// @brief Set Packet Filter object to handle send/receive packets.
///
@@ -718,14 +724,14 @@ protected:
//int recvsock_; // TODO: should be fd_set eventually, but we have only
//int sendsock_; // 2 sockets for now. Will do for until next release
- // we can't use the same socket, as receiving socket
+ // We can't use the same socket, as receiving socket
// is bound to multicast address. And we all know what happens
// to people who try to use multicast as source address.
- /// length of the control_buf_ array
+ /// Length of the control_buf_ array
size_t control_buf_len_;
- /// control-buffer, used in transmission and reception
+ /// Control-buffer, used in transmission and reception.
boost::scoped_array<char> control_buf_;
/// @brief A wrapper for OS-specific operations before sending IPv4 packet
@@ -745,10 +751,10 @@ protected:
/// @return true if successful, false otherwise
bool os_receive4(struct msghdr& m, Pkt4Ptr& pkt);
- /// socket descriptor of the session socket
+ /// Socket descriptor of the session socket.
int session_socket_;
- /// a callback that will be called when data arrives over session_socket_
+ /// A callback that will be called when data arrives over session_socket_.
SessionCallback session_callback_;
private:
@@ -795,7 +801,7 @@ private:
/// families, e.g. AF_INET, AF_PACKET etc. Another possible type of
/// Packet Filter is the one used for unit testing, which doesn't
/// open sockets but rather mimics their behavior (mock object).
- boost::shared_ptr<PktFilter> packet_filter_;
+ PktFilterPtr packet_filter_;
};
}; // namespace isc::dhcp
diff --git a/src/lib/dhcp/iface_mgr_bsd.cc b/src/lib/dhcp/iface_mgr_bsd.cc
index 92b99cd..2293877 100644
--- a/src/lib/dhcp/iface_mgr_bsd.cc
+++ b/src/lib/dhcp/iface_mgr_bsd.cc
@@ -54,8 +54,7 @@ void
IfaceMgr::setMatchingPacketFilter(const bool /* direct_response_desired */) {
// @todo Currently we ignore the preference to use direct traffic
// because it hasn't been implemented for BSD systems.
- boost::shared_ptr<PktFilter> pkt_filter(new PktFilterInet());
- setPacketFilter(pkt_filter);
+ setPacketFilter(PktFilterPtr(new PktFilterInet()));
}
diff --git a/src/lib/dhcp/iface_mgr_linux.cc b/src/lib/dhcp/iface_mgr_linux.cc
index 71a8a86..f31c353 100644
--- a/src/lib/dhcp/iface_mgr_linux.cc
+++ b/src/lib/dhcp/iface_mgr_linux.cc
@@ -515,12 +515,10 @@ void Iface::setFlags(uint32_t flags) {
void
IfaceMgr::setMatchingPacketFilter(const bool direct_response_desired) {
if (direct_response_desired) {
- boost::shared_ptr<PktFilter> pkt_filter(new PktFilterLPF());
- setPacketFilter(pkt_filter);
+ setPacketFilter(PktFilterPtr(new PktFilterLPF()));
} else {
- boost::shared_ptr<PktFilter> pkt_filter(new PktFilterInet());
- setPacketFilter(pkt_filter);
+ setPacketFilter(PktFilterPtr(new PktFilterInet()));
}
}
diff --git a/src/lib/dhcp/iface_mgr_sun.cc b/src/lib/dhcp/iface_mgr_sun.cc
index e046791..46c4a97 100644
--- a/src/lib/dhcp/iface_mgr_sun.cc
+++ b/src/lib/dhcp/iface_mgr_sun.cc
@@ -54,8 +54,7 @@ void
IfaceMgr::setMatchingPacketFilter(const bool /* direct_response_desired */) {
// @todo Currently we ignore the preference to use direct traffic
// because it hasn't been implemented for Solaris.
- boost::shared_ptr<PktFilter> pkt_filter(new PktFilterInet());
- setPacketFilter(pkt_filter);
+ setPacketFilter(PktFilterPtr(new PktFilterInet()));
}
} // end of isc::dhcp namespace
diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc
index 4f0d55f..919235b 100644
--- a/src/lib/dhcp/pkt4.cc
+++ b/src/lib/dhcp/pkt4.cc
@@ -284,7 +284,8 @@ Pkt4::setHWAddr(uint8_t htype, uint8_t hlen,
void
Pkt4::setHWAddr(const HWAddrPtr& addr) {
if (!addr) {
- isc_throw(BadValue, "Setting hw address to NULL is forbidden");
+ isc_throw(BadValue, "Setting DHCPv4 chaddr field to NULL"
+ << " is forbidden");
}
hwaddr_ = addr;
}
@@ -315,7 +316,8 @@ Pkt4::setLocalHWAddr(const uint8_t htype, const uint8_t hlen,
void
Pkt4::setLocalHWAddr(const HWAddrPtr& addr) {
if (!addr) {
- isc_throw(BadValue, "Setting HW address to NULL is forbidden");
+ isc_throw(BadValue, "Setting local HW address to NULL is"
+ << " forbidden.");
}
local_hwaddr_ = addr;
}
@@ -329,7 +331,8 @@ Pkt4::setRemoteHWAddr(const uint8_t htype, const uint8_t hlen,
void
Pkt4::setRemoteHWAddr(const HWAddrPtr& addr) {
if (!addr) {
- isc_throw(BadValue, "Setting HW address to NULL is forbidden");
+ isc_throw(BadValue, "Setting remote HW address to NULL is"
+ << " forbidden.");
}
remote_hwaddr_ = addr;
}
diff --git a/src/lib/dhcp/pkt_filter.h b/src/lib/dhcp/pkt_filter.h
index c89c2c1..204b25e 100644
--- a/src/lib/dhcp/pkt_filter.h
+++ b/src/lib/dhcp/pkt_filter.h
@@ -29,6 +29,7 @@ public:
isc::Exception(file, line, what) { };
};
+/// Forward declaration to the structure describing a socket.
struct SocketInfo;
/// Forward declaration to the class representing interface
diff --git a/src/lib/dhcp/pkt_filter_inet.cc b/src/lib/dhcp/pkt_filter_inet.cc
index a80f6b7..62695e5 100644
--- a/src/lib/dhcp/pkt_filter_inet.cc
+++ b/src/lib/dhcp/pkt_filter_inet.cc
@@ -53,7 +53,7 @@ int PktFilterInet::openSocket(const Iface& iface,
}
#ifdef SO_BINDTODEVICE
- if (receive_bcast) {
+ if (receive_bcast && iface.flag_broadcast_) {
// Bind to device so as we receive traffic on a specific interface.
if (setsockopt(sock, SOL_SOCKET, SO_BINDTODEVICE, iface.getName().c_str(),
iface.getName().length() + 1) < 0) {
diff --git a/src/lib/dhcp/pkt_filter_lpf.cc b/src/lib/dhcp/pkt_filter_lpf.cc
index 23340b5..2522ad3 100644
--- a/src/lib/dhcp/pkt_filter_lpf.cc
+++ b/src/lib/dhcp/pkt_filter_lpf.cc
@@ -26,45 +26,66 @@
namespace {
-/// Socket filter program, used to filter out all traffic other
-/// then DHCP. In particular, it allows receipt of UDP packets
+using namespace isc::dhcp;
+
+/// The socket filter program, used to filter out all traffic other
+/// than DHCP. In particular, it allows receipt of UDP packets
/// on a specific (customizable) port. It does not allow fragmented
/// packets.
///
/// Socket filter program is platform independent code which is
/// executed on the kernel level when new packet arrives. This concept
-/// origins from the Berkeley Packet Filtering supported on BSD systems.
+/// originates from the Berkeley Packet Filtering supported on BSD systems.
///
/// @todo We may want to extend the filter to receive packets sent
/// to the particular IP address assigned to the interface or
/// broadcast address.
struct sock_filter dhcp_sock_filter [] = {
- // Make sure this is an IP packet...
- BPF_STMT (BPF_LD + BPF_H + BPF_ABS, 12),
- BPF_JUMP (BPF_JMP + BPF_JEQ + BPF_K, ETHERTYPE_IP, 0, 8),
-
- // Make sure it's a UDP packet...
- BPF_STMT (BPF_LD + BPF_B + BPF_ABS, 23),
- BPF_JUMP (BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_UDP, 0, 6),
-
- // Make sure this isn't a fragment...
- BPF_STMT(BPF_LD + BPF_H + BPF_ABS, 20),
+ // Make sure this is an IP packet: check the half-word (two bytes)
+ // at offset 12 in the packet (the Ethernet packet type). If it
+ // is, advance to the next instruction. If not, advance 8
+ // instructions (which takes execution to the last instruction in
+ // the sequence: "drop it").
+ BPF_STMT(BPF_LD + BPF_H + BPF_ABS, ETHERNET_PACKET_TYPE_OFFSET),
+ BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ETHERTYPE_IP, 0, 8),
+
+ // Make sure it's a UDP packet. The IP protocol is at offset
+ // 9 in the IP header so, adding the Ethernet packet header size
+ // of 14 bytes gives an absolute byte offset in the packet of 23.
+ BPF_STMT(BPF_LD + BPF_B + BPF_ABS, ETHERNET_HEADER_LEN + IP_PROTO_TYPE_OFFSET),
+ BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_UDP, 0, 6),
+
+ // Make sure this isn't a fragment by checking that the fragment
+ // offset field in the IP header is zero. This field is the
+ // least-significant 13 bits in the bytes at offsets 6 and 7 in
+ // the IP header, so the half-word at offset 20 (6 + size of
+ // Ethernet header) is loaded and an appropriate mask applied.
+ BPF_STMT(BPF_LD + BPF_H + BPF_ABS, ETHERNET_HEADER_LEN + IP_FLAGS_OFFSET),
BPF_JUMP(BPF_JMP + BPF_JSET + BPF_K, 0x1fff, 4, 0),
- // Get the IP header length...
- BPF_STMT (BPF_LDX + BPF_B + BPF_MSH, 14),
-
- // Make sure it's to the right port...
- BPF_STMT (BPF_LD + BPF_H + BPF_IND, 16),
- // Use default DHCP server port, but it can be
- // replaced if neccessary.
- BPF_JUMP (BPF_JMP + BPF_JEQ + BPF_K, isc::dhcp::DHCP4_SERVER_PORT, 0, 1),
+ // Get the IP header length. This is achieved by the following
+ // (special) instruction that, given the offset of the start
+ // of the IP header (offset 14) loads the IP header length.
+ BPF_STMT(BPF_LDX + BPF_B + BPF_MSH, ETHERNET_HEADER_LEN),
+
+ // Make sure it's to the right port. The following instruction
+ // adds the previously extracted IP header length to the given
+ // offset to locate the correct byte. The given offset of 16
+ // comprises the length of the Ethernet header (14) plus the offset
+ // of the UDP destination port (2) within the UDP header.
+ BPF_STMT(BPF_LD + BPF_H + BPF_IND, ETHERNET_HEADER_LEN + UDP_DEST_PORT),
+ // The following instruction tests against the default DHCP server port,
+ // but the action port is actually set in PktFilterLPF::openSocket().
+ // N.B. The code in that method assumes that this instruction is at
+ // offset 8 in the program. If this is changed, openSocket() must be
+ // updated.
+ BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, DHCP4_SERVER_PORT, 0, 1),
// If we passed all the tests, ask for the whole packet.
- BPF_STMT(BPF_RET+BPF_K, (u_int)-1),
+ BPF_STMT(BPF_RET + BPF_K, (u_int)-1),
// Otherwise, drop it.
- BPF_STMT(BPF_RET+BPF_K, 0),
+ BPF_STMT(BPF_RET + BPF_K, 0),
};
}
@@ -107,6 +128,10 @@ PktFilterLPF::openSocket(const Iface& iface, const isc::asiolink::IOAddress&,
sa.sll_family = AF_PACKET;
sa.sll_ifindex = iface.getIndex();
+ // For raw sockets we construct IP headers on our own, so we don't bind
+ // socket to IP address but to the interface. We will later use the
+ // Linux Packet Filtering to filter out these packets that we are
+ // interested in.
if (bind(sock, reinterpret_cast<const struct sockaddr*>(&sa),
sizeof(sa)) < 0) {
close(sock);
@@ -120,9 +145,13 @@ PktFilterLPF::openSocket(const Iface& iface, const isc::asiolink::IOAddress&,
Pkt4Ptr
PktFilterLPF::receive(const Iface& iface, const SocketInfo& socket_info) {
- // @todo: implement this function
uint8_t raw_buf[IfaceMgr::RCVBUFSIZE];
int data_len = read(socket_info.sockfd_, raw_buf, sizeof(raw_buf));
+ // If negative value is returned by read(), it indicates that an
+ // error occured. If returned value is 0, no data was read from the
+ // socket. In both cases something has gone wrong, because we expect
+ // that a chunk of data is there. We signal the lack of data by
+ // returing an empty packet.
if (data_len <= 0) {
return Pkt4Ptr();
}
@@ -131,9 +160,12 @@ PktFilterLPF::receive(const Iface& iface, const SocketInfo& socket_info) {
// @todo: This is awkward way to solve the chicken and egg problem
// whereby we don't know the offset where DHCP data start in the
- // received buffer when we create the packet object. The dummy
- // object is created so as we can pass it to the functions which
- // decode IP stack and find actual offset of the DHCP packet.
+ // received buffer when we create the packet object. In general case,
+ // the IP header has variable length. The information about its length
+ // is stored in one of its fields. Therefore, we have to decode the
+ // packet to get the offset of the DHCP data. The dummy object is
+ // created so as we can pass it to the functions which decode IP stack
+ // and find actual offset of the DHCP data.
// Once we find the offset we can create another Pkt4 object from
// the reminder of the input buffer and set the IP addresses and
// ports from the dummy packet. We should consider doing it
@@ -180,11 +212,17 @@ PktFilterLPF::send(const Iface& iface, uint16_t sockfd, const Pkt4Ptr& pkt) {
// are valid because they are checked by the function called.
writeEthernetHeader(pkt, buf);
+ // This object represents broadcast address. We will compare the
+ // local packet address with it a few lines below. Having static
+ // variable guarantees that this object is created only once, not
+ // every time this function is called.
+ static const isc::asiolink::IOAddress bcast_addr("255.255.255.255");
+
// It is likely that the local address in pkt object is set to
// broadcast address. This is the case if server received the
// client's packet on broadcast address. Therefore, we need to
// correct it here and assign the actual source address.
- if (pkt->getLocalAddr().toText() == "255.255.255.255") {
+ if (pkt->getLocalAddr() == bcast_addr) {
const Iface::SocketCollection& sockets = iface.getSockets();
for (Iface::SocketCollection::const_iterator it = sockets.begin();
it != sockets.end(); ++it) {
diff --git a/src/lib/dhcp/protocol_util.cc b/src/lib/dhcp/protocol_util.cc
index 704bc0e..a57311d 100644
--- a/src/lib/dhcp/protocol_util.cc
+++ b/src/lib/dhcp/protocol_util.cc
@@ -16,6 +16,7 @@
#include <dhcp/dhcp6.h>
#include <dhcp/protocol_util.h>
#include <boost/static_assert.hpp>
+#include <net/ethernet.h>
// in_systm.h is required on some some BSD systems
// complaining that n_time is undefined but used
// in ip.h.
@@ -34,7 +35,8 @@ decodeEthernetHeader(InputBuffer& buf, Pkt4Ptr& pkt) {
// then the size of the Ethernet frame header.
if (buf.getLength() - buf.getPosition() < ETHERNET_HEADER_LEN) {
isc_throw(InvalidPacketHeader, "size of ethernet header in received "
- << "packet is invalid, expected at least 14 bytes, received "
+ << "packet is invalid, expected at least "
+ << ETHERNET_HEADER_LEN << " bytes, received "
<< buf.getLength() - buf.getPosition() << " bytes");
}
// Packet object must not be NULL. We want to output some values
@@ -92,7 +94,9 @@ decodeIpUdpHeader(InputBuffer& buf, Pkt4Ptr& pkt) {
// IP length is the number of 4 byte chunks that construct IPv4 header.
// It must not be lower than 5 because first 20 bytes are fixed.
if (ip_len < 5) {
- ip_len = 5;
+ isc_throw(InvalidPacketHeader, "Value of the length of the IP header must not be"
+ << " lower than 5 words. The length of the received header is "
+ << ip_len << ".");
}
// Seek to the position of source IP address.
@@ -102,7 +106,8 @@ decodeIpUdpHeader(InputBuffer& buf, Pkt4Ptr& pkt) {
// Read destination address.
pkt->setLocalAddr(IOAddress(buf.readUint32()));
- // Skip IP header options (if any).
+ // Skip IP header options (if any) to start of the
+ // UDP header.
buf.setPosition(start_pos + ip_len * 4);
// Read source port from UDP header.
@@ -110,7 +115,8 @@ decodeIpUdpHeader(InputBuffer& buf, Pkt4Ptr& pkt) {
// Read destination port from UDP header.
pkt->setLocalPort(buf.readUint16());
- // Set the pointer position to the tail of UDP header.
+ // Set the pointer position to the first byte o the
+ // UDP payload (DHCP packet).
buf.setPosition(start_pos + ip_len * 4 + UDP_HEADER_LEN);
}
@@ -156,7 +162,7 @@ writeEthernetHeader(const Pkt4Ptr& pkt, OutputBuffer& out_buf) {
}
// Type IP.
- out_buf.writeUint16(0x0800);
+ out_buf.writeUint16(ETHERTYPE_IP);
}
void
@@ -216,7 +222,7 @@ uint16_t
calcChecksum(const uint8_t* buf, const uint32_t buf_size, uint32_t sum) {
uint32_t i;
for (i = 0; i < (buf_size & ~1U); i += 2) {
- uint16_t chunk = buf[i] << 8 | buf[i+1];
+ uint16_t chunk = buf[i] << 8 | buf[i + 1];
sum += chunk;
if (sum > 0xFFFF) {
sum -= 0xFFFF;
diff --git a/src/lib/dhcp/protocol_util.h b/src/lib/dhcp/protocol_util.h
index e40f103..582938e 100644
--- a/src/lib/dhcp/protocol_util.h
+++ b/src/lib/dhcp/protocol_util.h
@@ -35,19 +35,31 @@ public:
/// Size of the Ethernet frame header.
static const size_t ETHERNET_HEADER_LEN = 14;
+/// Offset of the 2-byte word in the Ethernet packet which
+/// holds the type of the protocol it encapsulates.
+static const size_t ETHERNET_PACKET_TYPE_OFFSET = 12;
+
/// Minimal IPv4 header length.
static const size_t MIN_IP_HEADER_LEN = 20;
-/// UDP header length.
-static const size_t UDP_HEADER_LEN = 8;
+/// Offset in the IP header where the flags field starts.
+static const size_t IP_FLAGS_OFFSET = 6;
+/// Offset of the byte in IP header which holds the type
+/// of the protocol it encapsulates.
+static const size_t IP_PROTO_TYPE_OFFSET = 9;
/// Offset of source address in the IPv4 header.
static const size_t IP_SRC_ADDR_OFFSET = 12;
+/// UDP header length.
+static const size_t UDP_HEADER_LEN = 8;
+/// Offset within UDP header where destination port is held.
+static const size_t UDP_DEST_PORT = 2;
+
/// @brief Decode the Ethernet header.
///
/// This function reads Ethernet frame header from the provided
/// buffer at the current read position. The source HW address
/// is read from the header and assigned as client address in
-/// a pkt object. The buffer read pointer is set to the end
+/// the pkt object. The buffer read pointer is set to the end
/// of the Ethernet frame header if read was successful.
///
/// @warning This function does not check that the provided 'pkt'
@@ -66,7 +78,7 @@ void decodeEthernetHeader(util::InputBuffer& buf, Pkt4Ptr& pkt);
/// This function reads IP and UDP headers from the provided buffer
/// at the current read position. The source and destination IP
/// addresses and ports and read from these headers and stored in
-/// the appropriate members of pkt object.
+/// the appropriate members of the pkt object.
///
/// @warning This function does not check that the provided 'pkt'
/// pointer is valid. Caller must make sure that pointer is
@@ -95,7 +107,7 @@ void writeEthernetHeader(const Pkt4Ptr& pkt,
///
/// This utility function assembles IP and UDP packet headers for the
/// provided DHCPv4 message. The source and destination addreses and
-/// ports stored in the Pkt4 object are copied as source and destination
+/// ports stored in the pkt object are copied as source and destination
/// addresses and ports into IP/UDP headers.
///
/// @warning This function does not check that the provided 'pkt'
@@ -117,6 +129,9 @@ void writeIpUdpHeader(const Pkt4Ptr& pkt, util::OutputBuffer& out_buf);
/// of the summed values. It must be calculated outside of this function
/// before writing the value to the packet buffer.
///
+/// The IP header checksum calculation algorithm has been defined in
+/// <a href="https://tools.ietf.org/html/rfc791#page-14">RFC 791</a>
+///
/// @param buf buffer for which the checksum is calculated.
/// @param buf_size size of the buffer for which checksum is calculated.
/// @param sum initial checksum value, other values will be added to it.
diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc
index 89a2110..09806c5 100644
--- a/src/lib/dhcp/tests/iface_mgr_unittest.cc
+++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc
@@ -818,7 +818,6 @@ TEST_F(IfaceMgrTest, sendReceive4) {
);
EXPECT_GE(socket1, 0);
- // EXPECT_GE(socket2, 0);
boost::shared_ptr<Pkt4> sendPkt(new Pkt4(DHCPDISCOVER, 1234) );
diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc
index f033e64..df29bee 100644
--- a/src/lib/dhcp/tests/pkt4_unittest.cc
+++ b/src/lib/dhcp/tests/pkt4_unittest.cc
@@ -686,8 +686,8 @@ TEST(Pkt4Test, hwaddrSrcRemote) {
EXPECT_TRUE(dst_hwaddr == pkt->getLocalHWAddr());
// Check that we can set the remote address.
- EXPECT_NO_THROW(pkt->setRemoteHWAddr(dst_hwaddr));
- EXPECT_TRUE(dst_hwaddr == pkt->getRemoteHWAddr());
+ EXPECT_NO_THROW(pkt->setRemoteHWAddr(src_hwaddr));
+ EXPECT_TRUE(src_hwaddr == pkt->getRemoteHWAddr());
// Can't set the NULL addres.
EXPECT_THROW(pkt->setRemoteHWAddr(HWAddrPtr()), BadValue);
More information about the bind10-changes
mailing list