Fixed a bug and some confusion with ACLChecklist::fastCheck()

- Added cbdataReferenceDone(accessList) call to fastCheck() in the
  "if finished()" block.

- peerAllowedToUse() was setting accessList = NULL after calling
  fastCheck().  Since neither fastCheck() nor peerAllowedToUse()
  were calling cbdataReferenceDone(), the cbdata lock_count would
  never decrease and eventually hit an assertion.

- In general, the fastCheck() callers do not need to call
  cbdataReferenceDone, nor set accessList to NULL.  The
  cbdataReferenceDone() call should always happen in fastCheck().
  If not, it will happen in the ACLCheckList destructor.

- It might be even better if fastCheck() called cbdataReference()
  as well, to avoid this situation where locking happens in one
  place and unlocking in another.  This is future work, however.

diff --git a/src/ACLChecklist.cc b/src/ACLChecklist.cc
index 339b58236f0162ca4eed011b0607ac23a6270df6..c177f17445a2ae010c36a3e8a485706972c73dad 100644 (file)
--- a/src/ACLChecklist.cc
+++ b/src/ACLChecklist.cc
@@ -1,5 +1,5 @@
 /*
- * $Id: ACLChecklist.cc,v 1.28 2005/10/23 11:55:31 hno Exp $
+ * $Id: ACLChecklist.cc,v 1.29 2005/12/08 20:08:46 wessels Exp $
  *
  * DEBUG: section 28    Access Control
  * AUTHOR: Duane Wessels
@@ -409,6 +409,7 @@ ACLChecklist::fastCheck()
 
         if (finished()) {
             PROF_stop(aclCheckFast);
+            cbdataReferenceDone(accessList);
             return currentAnswer() == ACCESS_ALLOWED;
         }
 

diff --git a/src/DelayId.cc b/src/DelayId.cc
index d965d6ce4b24d0e5f4608dce64ec57c8b9d03ae8..2ed9457abe5d6bf482d888192e8baa8c49366f35 100644 (file)
--- a/src/DelayId.cc
+++ b/src/DelayId.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: DelayId.cc,v 1.17 2005/09/09 17:31:33 wessels Exp $
+ * $Id: DelayId.cc,v 1.18 2005/12/08 20:08:46 wessels Exp $
  *
  * DEBUG: section 77    Delay Pools
  * AUTHOR: Robert Collins <robertc@squid-cache.org>
@@ -116,9 +116,10 @@ DelayId::DelayClient(ClientHttpRequest * http)
 
         ch.accessList = cbdataReference(DelayPools::delay_data[pool].access);
 
+        /* cbdataReferenceDone() happens in either fastCheck() or ~ACLCheckList */
+
         if (DelayPools::delay_data[pool].theComposite().getRaw() &&
                 ch.fastCheck()) {
-            ch.accessList = NULL;
             DelayId result (pool + 1);
             CompositePoolNode::CompositeSelectionDetails details;
             details.src_addr = ch.src_addr;
@@ -127,8 +128,6 @@ DelayId::DelayClient(ClientHttpRequest * http)
             result.compositePosition(DelayPools::delay_data[pool].theComposite()->id(details));
             return result;
         }
-
-        ch.accessList = NULL;
     }
 
 

diff --git a/src/cbdata.cc b/src/cbdata.cc
index 4c7fc8023af616369a12f2c3968f318f76a32e64..6a0ddabff030d1b1467714b9127ea0de9d8b1640 100644 (file)
--- a/src/cbdata.cc
+++ b/src/cbdata.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: cbdata.cc,v 1.65 2005/11/05 00:08:32 wessels Exp $
+ * $Id: cbdata.cc,v 1.66 2005/12/08 20:08:46 wessels Exp $
  *
  * DEBUG: section 45    Callback Data Registry
  * ORIGINAL AUTHOR: Duane Wessels
@@ -364,7 +364,7 @@ cbdataInternalLock(const void *p)
 
     c->check(__LINE__);
 
-    assert(c->locks < 65535);
+    assert(c->locks < 655);
 
     c->locks++;
 }

diff --git a/src/client_side.cc b/src/client_side.cc
index 2599fb746dc79020d49a096018f9595461489fba..40ca91652cd24ec54fe52002aafebe1f19760dc4 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side.cc,v 1.705 2005/12/06 23:03:34 wessels Exp $
+ * $Id: client_side.cc,v 1.706 2005/12/08 20:08:46 wessels Exp $
  *
  * DEBUG: section 33    Client-side Routines
  * AUTHOR: Duane Wessels
@@ -2869,12 +2869,11 @@ httpAccept(int sock, int newfd, ConnectionDetail *details,
 
     identChecklist.accessList = cbdataReference(Config.accessList.identLookup);
 
+    /* cbdataReferenceDone() happens in either fastCheck() or ~ACLCheckList */
+
     if (identChecklist.fastCheck())
         identStart(&details->me, &details->peer, clientIdentDone, connState);
 
-    cbdataReferenceDone(identChecklist.accessList);
-
-    identChecklist.accessList = NULL;
 
 #endif
 
@@ -3052,11 +3051,11 @@ httpsAccept(int sock, int newfd, ConnectionDetail *details,
 
     identChecklist.accessList = cbdataReference(Config.accessList.identLookup);
 
+    /* cbdataReferenceDone() happens in either fastCheck() or ~ACLCheckList */
+
     if (identChecklist.fastCheck())
         identStart(&details->me, &details->peer, clientIdentDone, connState);
 
-    identChecklist.accessList = NULL;
-
 #endif
 
     commSetSelect(newfd, COMM_SELECT_READ, clientNegotiateSSL, connState, 0);

diff --git a/src/forward.cc b/src/forward.cc
index 4549eaf10dc058791d124cab183101f1050a58d0..28e22f2c7d5384ae9a0d95f8cd0ef6014a13e4e1 100644 (file)
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: forward.cc,v 1.131 2005/12/06 23:03:34 wessels Exp $
+ * $Id: forward.cc,v 1.132 2005/12/08 20:08:46 wessels Exp $
  *
  * DEBUG: section 17    Request Forwarding
  * AUTHOR: Duane Wessels
@@ -876,8 +876,8 @@ fwdStart(int fd, StoreEntry * e, HttpRequest * r)
         ch.my_port = r->my_port;
         ch.request = requestLink(r);
         ch.accessList = cbdataReference(Config.accessList.miss);
+        /* cbdataReferenceDone() happens in either fastCheck() or ~ACLCheckList */
         answer = ch.fastCheck();
-        ch.accessList = NULL;
 
         if (answer == 0) {
             err_type page_id;

diff --git a/src/http.cc b/src/http.cc
index 86173e440db1cc7dfa13adeffc96466272d8e2b2..85ed132df1aa3796b63ab57fb0f8a1f0262b9180 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: http.cc,v 1.470 2005/12/06 23:03:34 wessels Exp $
+ * $Id: http.cc,v 1.471 2005/12/08 20:08:46 wessels Exp $
  *
  * DEBUG: section 11    Hypertext Transfer Protocol (HTTP)
  * AUTHOR: Harvest Derived
@@ -1907,6 +1907,8 @@ httpSendRequestEntityDone(int fd, void *data)
     if (Config.accessList.brokenPosts)
         ch.accessList = cbdataReference(Config.accessList.brokenPosts);
 
+    /* cbdataReferenceDone() happens in either fastCheck() or ~ACLCheckList */
+
     if (!Config.accessList.brokenPosts) {
         debug(11, 5) ("httpSendRequestEntityDone: No brokenPosts list\n");
         HttpStateData::SendComplete(fd, NULL, 0, COMM_OK, data);
@@ -1917,8 +1919,6 @@ httpSendRequestEntityDone(int fd, void *data)
         debug(11, 2) ("httpSendRequestEntityDone: matched brokenPosts\n");
         comm_old_write(fd, "\r\n", 2, HttpStateData::SendComplete, data, NULL);
     }
-
-    ch.accessList = NULL;
 }
 
 static void

diff --git a/src/icp_v2.cc b/src/icp_v2.cc
index f6076224a21214878534434a23df153f3c4a723b..29407dfe21c4734f6cc7ebcce65fb4425d270a6f 100644 (file)
--- a/src/icp_v2.cc
+++ b/src/icp_v2.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: icp_v2.cc,v 1.87 2005/06/09 16:04:30 serassio Exp $
+ * $Id: icp_v2.cc,v 1.88 2005/12/08 20:08:47 wessels Exp $
  *
  * DEBUG: section 12    Internet Cache Protocol
  * AUTHOR: Duane Wessels
@@ -399,8 +399,8 @@ icpAccessAllowed(struct sockaddr_in *from, HttpRequest * icp_request)
     checklist.my_addr = no_addr;
     checklist.request = requestLink(icp_request);
     checklist.accessList = cbdataReference(Config.accessList.icp);
+    /* cbdataReferenceDone() happens in either fastCheck() or ~ACLCheckList */
     int result = checklist.fastCheck();
-    checklist.accessList = NULL;
     return result;
 }
 

diff --git a/src/neighbors.cc b/src/neighbors.cc
index 1bc96f104facd5000153216fcd0e68bd21ee3bde..c40bd44063811afb542d667c60d699513f1d915f 100644 (file)
--- a/src/neighbors.cc
+++ b/src/neighbors.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: neighbors.cc,v 1.331 2005/06/09 16:04:30 serassio Exp $
+ * $Id: neighbors.cc,v 1.332 2005/12/08 20:08:47 wessels Exp $
  *
  * DEBUG: section 15    Neighbor Routines
  * AUTHOR: Harvest Derived
@@ -187,6 +187,8 @@ peerAllowedToUse(const peer * p, HttpRequest * request)
 
     checklist.accessList = cbdataReference(p->access);
 
+    /* cbdataReferenceDone() happens in either fastCheck() or ~ACLCheckList */
+
 #if 0 && USE_IDENT
     /*
      * this is currently broken because 'request->user_ident' has been
@@ -198,11 +200,7 @@ peerAllowedToUse(const peer * p, HttpRequest * request)
 
 #endif
 
-    int result = checklist.fastCheck();
-
-    checklist.accessList = NULL;
-
-    return result;
+    return checklist.fastCheck();
 }
 
 /* Return TRUE if it is okay to send an ICP request to this peer.   */

diff --git a/src/snmp_core.cc b/src/snmp_core.cc
index a5f6c33decf6128348706ac5efb42e69de651998..c744ffd068e2f551c5fd9ef87b22f859aaaf8174 100644 (file)
--- a/src/snmp_core.cc
+++ b/src/snmp_core.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: snmp_core.cc,v 1.70 2005/12/08 18:33:55 wessels Exp $
+ * $Id: snmp_core.cc,v 1.71 2005/12/08 20:08:47 wessels Exp $
  *
  * DEBUG: section 49    SNMP support
  * AUTHOR: Glenn Chisholm
@@ -536,14 +536,14 @@ snmpDecodePacket(snmp_request_t * rq)
     PDU = snmp_pdu_create(0);
     rq->session.Version = SNMP_VERSION_1;
     Community = snmp_parse(&rq->session, PDU, buf, len);
-    ACLChecklist checklist;
     checklist.src_addr = rq->from.sin_addr;
     checklist.snmp_community = (char *) Community;
 
     if (Community) {
+        ACLChecklist checklist;
         checklist.accessList = cbdataReference(Config.accessList.snmp);
+        /* cbdataReferenceDone() happens in either fastCheck() or ~ACLCheckList */
         allow = checklist.fastCheck();
-        checklist.accessList = NULL;
     }
 
     if ((snmp_coexist_V2toV1(PDU)) && (Community) && (allow)) {

diff --git a/src/tunnel.cc b/src/tunnel.cc
index 3a1414f351553ac17db632523b29cd19996f1b98..ad29cd09e0ccff8ce235b5980bcd5cbdf7d96bdf 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: tunnel.cc,v 1.155 2005/12/01 21:35:40 serassio Exp $
+ * $Id: tunnel.cc,v 1.156 2005/12/08 20:08:47 wessels Exp $
  *
  * DEBUG: section 26    Secure Sockets Layer Proxy
  * AUTHOR: Duane Wessels
@@ -604,8 +604,8 @@ sslStart(ClientHttpRequest * http, size_t * size_ptr, int *status_ptr)
         ch.my_port = request->my_port;
         ch.request = requestLink(request);
         ch.accessList = cbdataReference(Config.accessList.miss);
+        /* cbdataReferenceDone() happens in either fastCheck() or ~ACLCheckList */
         answer = ch.fastCheck();
-        ch.accessList = NULL;
 
         if (answer == 0) {
             err = errorCon(ERR_FORWARDING_DENIED, HTTP_FORBIDDEN);