From: wessels <>
Date: Thu, 15 Sep 2005 00:23:21 +0000 (+0000)
Subject: Bug #1332: mem_node leak
X-Git-Tag: SQUID_3_0_PRE4~625
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=12d74f9612eb869a2c2de1045379e83a042c562f;p=thirdparty%2Fsquid.git

Bug #1332: mem_node leak

There was a fairly significant mem_node leak because (1) nodes were
being removed from the 'nodes' list without also being deleted, and
(2) there was no storeIOWrite callback to decrement the use count.

This patch changes the mem_node use count to a boolean write_pending
flag, adds a memNodeWriteComplete() callback function, and removes
and deletes nodes at the same time.  mem_hdr::unlink() was changed
to return boolean to prevent loops in freeDataUpto().
---

diff --git a/src/mem_node.cc b/src/mem_node.cc
index 788309a8c4..47baf4fe5f 100644
--- a/src/mem_node.cc
+++ b/src/mem_node.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: mem_node.cc,v 1.6 2004/08/30 05:12:31 robertc Exp $
+ * $Id: mem_node.cc,v 1.7 2005/09/14 18:23:21 wessels Exp $
  *
  * DEBUG: section 19    Store Memory Primitives
  * AUTHOR: Robert Collins
@@ -36,7 +36,35 @@
 #include "squid.h"
 #include "mem_node.h"
 
+static int makeMemNodeDataOffset();
+
 unsigned long mem_node::store_mem_size;
+static int _mem_node_data_offset = makeMemNodeDataOffset();
+
+/*
+ * Calculate the offset between the start of a mem_node and
+ * its 'data' member
+ */
+static int
+makeMemNodeDataOffset()
+{
+    mem_node *p = 0L;
+    return int(&p->data);
+}
+
+/*
+ * This is the callback when storeIOWrite() is done.  We need to
+ * clear the write_pending flag for the mem_node.  First we have
+ * to calculate the start of the mem_node based on the character
+ * buffer that we wrote.  ick.
+ */
+void
+memNodeWriteComplete(void* d)
+{
+    mem_node* n = (mem_node*)((char*)d - _mem_node_data_offset);
+    assert(n->write_pending);
+    n->write_pending = 0;
+}
 
 mem_node::mem_node(off_t offset):nodeBuffer(0,offset,data)
 {}
diff --git a/src/mem_node.h b/src/mem_node.h
index 86f8f31d5e..3ef21ab63f 100644
--- a/src/mem_node.h
+++ b/src/mem_node.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: mem_node.h,v 1.8 2005/04/01 21:11:28 serassio Exp $
+ * $Id: mem_node.h,v 1.9 2005/09/14 18:23:21 wessels Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -58,7 +58,9 @@ public:
     StoreIOBuffer nodeBuffer;
     /* Private */
     char data[SM_PAGE_SIZE];
-    int uses;
+
+unsigned int write_pending:
+    1;
 };
 
 MEMPROXY_CLASS_INLINE(mem_node)
@@ -70,4 +72,6 @@ operator << (std::ostream &os, mem_node &aNode)
     return os;
 }
 
+void memNodeWriteComplete(void *);
+
 #endif /* SQUID_MEM_NODE_H */
diff --git a/src/stmem.cc b/src/stmem.cc
index dc5c8ed237..be352e4bf3 100644
--- a/src/stmem.cc
+++ b/src/stmem.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: stmem.cc,v 1.88 2005/04/01 21:11:28 serassio Exp $
+ * $Id: stmem.cc,v 1.89 2005/09/14 18:23:21 wessels Exp $
  *
  * DEBUG: section 19    Store Memory Primitives
  * AUTHOR: Harvest Derived
@@ -40,10 +40,18 @@
 #include "MemObject.h"
 #include "Generic.h"
 
+/*
+ * NodeGet() is called to get the data buffer to pass to storeIOWrite().
+ * By setting the write_pending flag here we are assuming that there
+ * will be no other users of NodeGet().  The storeIOWrite() callback
+ * is memNodeWriteComplete(), which, for whatever reason, lives in
+ * mem_node.cc.
+ */
 char *
 mem_hdr::NodeGet(mem_node * aNode)
 {
-    aNode->uses++;
+    assert(!aNode->write_pending);
+    aNode->write_pending = 1;
     return aNode->data;
 }
 
@@ -79,16 +87,17 @@ mem_hdr::freeContent()
     inmem_hi = 0;
 }
 
-void
+bool
 mem_hdr::unlink(mem_node *aNode)
 {
-    nodes.remove (aNode, NodeCompare);
-
-    if (!aNode->uses)
-        delete aNode;
-    else
-        aNode->uses--;
+    if (aNode->write_pending) {
+        debug(0,0)("cannot unlink mem_node %p while write_pending\n", aNode);
+        return false;
+    }
 
+    nodes.remove (aNode, NodeCompare);
+    delete aNode;
+    return true;
 }
 
 int
@@ -98,10 +107,15 @@ mem_hdr::freeDataUpto(int target_offset)
 
     SplayNode<mem_node*> const * theStart = nodes.start();
 
-    while (theStart && theStart != nodes.finish() &&
-            theStart->data->end() <= (size_t) target_offset ) {
-        unlink(theStart->data);
-        theStart = nodes.start();
+    while ((theStart = nodes.start())) {
+        if (theStart == nodes.finish())
+            break;
+
+        if (theStart->data->end() > (size_t) target_offset )
+            break;
+
+        if (!unlink(theStart->data))
+            break;
     }
 
     assert (lowestOffset () <= target_offset);
diff --git a/src/stmem.h b/src/stmem.h
index 04a8875d0e..3624ae283e 100644
--- a/src/stmem.h
+++ b/src/stmem.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: stmem.h,v 1.8 2005/04/01 21:11:28 serassio Exp $
+ * $Id: stmem.h,v 1.9 2005/09/14 18:23:21 wessels Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -74,7 +74,7 @@ public:
 
 private:
     void debugDump() const;
-    void unlink(mem_node *aNode);
+    bool unlink(mem_node *aNode);
     void makeAppendSpace();
     int appendToNode(mem_node *aNode, const char *data, int maxLength);
     void appendNode (mem_node *aNode);
diff --git a/src/store_swapout.cc b/src/store_swapout.cc
index 1137e2317e..d52cc66bfb 100644
--- a/src/store_swapout.cc
+++ b/src/store_swapout.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: store_swapout.cc,v 1.100 2005/04/01 21:11:28 serassio Exp $
+ * $Id: store_swapout.cc,v 1.101 2005/09/14 18:23:21 wessels Exp $
  *
  * DEBUG: section 20    Storage Manager Swapout Functions
  * AUTHOR: Duane Wessels
@@ -148,7 +148,7 @@ doPages(StoreEntry *anEntry)
 
         mem->swapout.queue_offset += swap_buf_len;
 
-        storeIOWrite(mem->swapout.sio, mem->data_hdr.NodeGet(mem->swapout.memnode), swap_buf_len, -1, NULL);
+        storeIOWrite(mem->swapout.sio, mem->data_hdr.NodeGet(mem->swapout.memnode), swap_buf_len, -1, memNodeWriteComplete);
 
         /* the storeWrite() call might generate an error */
         if (anEntry->swap_status != SWAPOUT_WRITING)