Partial solution to a race condition between arp_timer() and

arp_update_entry(). While arp_timer() was sending the last request
arp_update_entry() could be called (caused by an incoming reply) and
free the request buffer, which arp_timer() would free a moment later
again. The problem is not completely solved, since a duplicate reply can
still cause a double free. Checked in mainly for Axel's reading
pleasure. :-)


git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@25252 a95241bf-73f2-0310-859d-f6bbb57e9c96

src/add-ons/kernel/network/datalink_protocols/arp/arp.cpp

@@ -68,6 +68,7 @@ struct arp_entry {
 	sockaddr_dl	hardware_address;
 	uint32		flags;
 	net_buffer	*request_buffer;
+	int32		request_buffer_ref_count;
 	net_timer	timer;
 	uint32		timer_state;
 	bigtime_t	timestamp;
@@ -118,6 +119,28 @@ static benaphore sCacheLock;
 static bool sIgnoreReplies;
 
 
+static net_buffer*
+get_request_buffer_reference(arp_entry* entry)
+{
+	if (atomic_test_and_set(&entry->request_buffer_ref_count, 2, 1) == 1)
+		return entry->request_buffer;
+
+	return NULL;
+}
+
+
+static void
+put_request_buffer_reference(arp_entry* entry)
+{
+	if (atomic_add(&entry->request_buffer_ref_count, -1) == 0) {
+		if (entry->request_buffer != NULL) {
+			gBufferModule->free(entry->request_buffer);
+			entry->request_buffer = NULL;
+		}
+	}
+}
+
+
 /*static*/ int
 arp_entry::Compare(void *_entry, const void *_key)
 {
@@ -174,6 +197,7 @@ arp_entry::Add(in_addr_t protocolAddress, sockaddr_dl *hardwareAddress,
 	entry->timestamp = system_time();
 	entry->protocol = NULL;
 	entry->request_buffer = NULL;
+	entry->request_buffer_ref_count = 0;
 	entry->timer_state = ARP_NO_STATE;
 	sStackModule->init_timer(&entry->timer, arp_timer, entry);
 
@@ -320,10 +344,7 @@ arp_update_entry(in_addr_t protocolAddress, sockaddr_dl *hardwareAddress,
 			return B_NO_MEMORY;
 	}
 
-	if (entry->request_buffer != NULL) {
+	put_request_buffer_reference(entry);
-		gBufferModule->free(entry->request_buffer);
-		entry->request_buffer = NULL;
-	}
 
 	if ((entry->flags & ARP_FLAG_PERMANENT) == 0) {
 		// (re)start the stale timer
@@ -537,7 +558,15 @@ arp_timer(struct net_timer *timer, void *data)
 				break;
 
 			TRACE(("  send request for ARP entry %p!\n", entry));
-			net_buffer *request = entry->request_buffer;
+
+			net_buffer *request = get_request_buffer_reference(entry);
+			if (request == NULL)
+				break;
+			// TODO: The reference counting does still not solve request_buffer
+			// access problem completely: A duplicate reply between this point
+			// and the "entry->request_buffer = NULL" will release two
+			// references in arp_update_entry() and thus delete the buffer.
+
 			if (entry->timer_state < ARP_STATE_LAST_REQUEST) {
 				// we'll still need our buffer, so in order to prevent it being
 				// freed by a successful send, we need to clone it
@@ -547,7 +576,13 @@ arp_timer(struct net_timer *timer, void *data)
 					// many requests as originally planned
 					request = entry->request_buffer;
 					entry->timer_state = ARP_STATE_LAST_REQUEST;
-				}
+				} else
+					put_request_buffer_reference(entry);
+			}
+
+			if (entry->timer_state == ARP_STATE_LAST_REQUEST) {
+				// the request buffer will be deleted, detach it
+				entry->request_buffer = NULL;
 			}
 
 			// we're trying to resolve the address, so keep sending requests
@@ -556,11 +591,6 @@ arp_timer(struct net_timer *timer, void *data)
 			if (status < B_OK)
 				gBufferModule->free(request);
 
-			if (entry->timer_state == ARP_STATE_LAST_REQUEST) {
-				// buffer has been freed on send
-				entry->request_buffer = NULL;
-			}
-
 			entry->timer_state++;
 			sStackModule->set_timer(&entry->timer, ARP_REQUEST_TIMEOUT);
 			break;
@@ -591,6 +621,7 @@ arp_start_resolve(net_datalink_protocol *protocol, in_addr_t address,
 		// TODO: do something with the entry
 		return B_NO_MEMORY;
 	}
+	entry->request_buffer_ref_count = 1;
 
 	NetBufferPrepend<arp_header> bufferHeader(entry->request_buffer);
 	status_t status = bufferHeader.Status();