From: Marcin Siodelski Date: Tue, 21 May 2013 08:45:22 +0000 (+0200) Subject: [2902] Changes as a result of the review. X-Git-Tag: bind10-1.2.0beta1-release~440^2~6 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=5162bd7739ccfab83a0e81e44ba5e46314eae0cf;p=thirdparty%2Fkea.git [2902] Changes as a result of the review. --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 426b26b004..3eb187bfa7 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 0a19e6ee34..8591eefb62 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 srv(new NakedDhcpv4Srv(0)); + + // Initialize the source HW address. vector mac(6); for (int i = 0; i < 6; i++) { - mac[i] = i*10; + mac[i] = i * 10; } - + // Initialized the destination HW address. vector 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 req(new Pkt4(msg_type, 1234)); + // Create a response message. It will hold a reponse packet. + // Initially, set it to NULL. boost::shared_ptr 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 93e6205641..1e97205287 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& 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 338f8ebc57..35b4dc0248 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 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 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 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& 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 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 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 92b99cdc62..2293877eb5 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 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 71a8a865ba..f31c35334b 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 pkt_filter(new PktFilterLPF()); - setPacketFilter(pkt_filter); + setPacketFilter(PktFilterPtr(new PktFilterLPF())); } else { - boost::shared_ptr 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 e046791719..46c4a97467 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 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 4f0d55fcec..919235bd09 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 c89c2c118f..204b25e6b6 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 a80f6b7a26..62695e5949 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 23340b5478..2522ad3da1 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(&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 704bc0e755..a57311da3e 100644 --- a/src/lib/dhcp/protocol_util.cc +++ b/src/lib/dhcp/protocol_util.cc @@ -16,6 +16,7 @@ #include #include #include +#include // 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 e40f1032f8..582938e207 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 +/// RFC 791 +/// /// @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 89a2110404..09806c5d22 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 sendPkt(new Pkt4(DHCPDISCOVER, 1234) ); diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc index f033e64b2a..df29beeed9 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);