From: Francis Dupont <fdupont@isc.org>
Date: Wed, 20 Nov 2019 01:50:53 +0000 (+0100)
Subject: [880-libdhcp-vs-thread-sanitizer] Checkpoint: first point addressed
X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=8055792c5298ce494b4aa2d3193141ac85d6f6c6;p=thirdparty%2Fkea.git

[880-libdhcp-vs-thread-sanitizer] Checkpoint: first point addressed
---

diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc
index afcdb03863..c1ca2cf55f 100644
--- a/src/lib/dhcp/iface_mgr.cc
+++ b/src/lib/dhcp/iface_mgr.cc
@@ -1032,7 +1032,7 @@ Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec /
     // DHCP packets are waiting so we don't starve external
     // sockets under heavy DHCP load.
     struct timeval select_timeout;
-    if (getPacketQueue4()->empty()) {
+    if (!getPacketQueue4()->canDequeue()) {
         select_timeout.tv_sec = timeout_sec;
         select_timeout.tv_usec = timeout_usec;
     } else {
@@ -1045,7 +1045,7 @@ Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec /
 
     int result = select(maxfd + 1, &sockets, NULL, NULL, &select_timeout);
 
-    if ((result == 0) && getPacketQueue4()->empty()) {
+    if ((result == 0) && !getPacketQueue4()->canDequeue()) {
         // nothing received and timeout has been reached
         return (Pkt4Ptr());
     } else if (result < 0) {
@@ -1373,7 +1373,7 @@ IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */
     // DHCP packets are waiting so we don't starve external
     // sockets under heavy DHCP load.
     struct timeval select_timeout;
-    if (getPacketQueue6()->empty()) {
+    if (!getPacketQueue6()->canDequeue()) {
         select_timeout.tv_sec = timeout_sec;
         select_timeout.tv_usec = timeout_usec;
     } else {
@@ -1386,7 +1386,7 @@ IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */
 
     int result = select(maxfd + 1, &sockets, NULL, NULL, &select_timeout);
 
-    if ((result == 0) && getPacketQueue6()->empty()) {
+    if ((result == 0) && !getPacketQueue6()->canDequeue()) {
         // nothing received and timeout has been reached
         return (Pkt6Ptr());
     } else if (result < 0) {
diff --git a/src/lib/dhcp/packet_queue.h b/src/lib/dhcp/packet_queue.h
index aea1c95e2a..375d97d157 100644
--- a/src/lib/dhcp/packet_queue.h
+++ b/src/lib/dhcp/packet_queue.h
@@ -82,6 +82,11 @@ public:
     /// @brief return True if the queue is empty.
     virtual bool empty() const = 0;
 
+    /// @brief return True if dequeue will not return null.
+    ///
+    /// In fact it is !empty within a lock guard.
+    virtual bool canDequeue() = 0;
+
     /// @brief Returns the current number of packets in the buffer.
     virtual size_t getSize() const = 0;
 
diff --git a/src/lib/dhcp/packet_queue_ring.h b/src/lib/dhcp/packet_queue_ring.h
index 24b8890a26..3db20088cc 100644
--- a/src/lib/dhcp/packet_queue_ring.h
+++ b/src/lib/dhcp/packet_queue_ring.h
@@ -123,12 +123,13 @@ public:
     /// @return A pointer to dequeued packet, or an empty pointer
     /// if the queue is empty.
     virtual PacketTypePtr popPacket(const QueueEnd& from = QueueEnd::FRONT) {
+        std::lock_guard<std::mutex> lock(mutex_);
+
         PacketTypePtr packet;
         if (queue_.empty()) {
             return (packet);
         }
 
-        std::lock_guard<std::mutex> lock(mutex_);
         if (from == QueueEnd::FRONT) {
             packet = queue_.front();
             queue_.pop_front();
@@ -161,7 +162,14 @@ public:
 
     /// @brief Returns True if the queue is empty.
     virtual bool empty() const {
-        return(queue_.empty());
+        return (queue_.empty());
+    }
+
+    /// @brief return True if dequeue will not return null.
+    /// Use the mutex so race free with a concurrent enqueue.
+    virtual bool canDequeue() {
+        std::lock_guard<std::mutex> lock(mutex_);
+        return (!queue_.empty());
     }
 
     /// @brief Returns the maximum number of packets allowed in the buffer.