Author: Alex Rousskov <rousskov@measurement-factory.com

Bug #1950: cachemgr requests for auth helper stats can crash Squid

Partially-configured authentication helpers may result in Squid
crashes when requesting stats via cachemgr.  The attached patch
moves the "is this helper valid?" check into the helper*Stats()
function so that new helpers do not introduce more bugs like this
one.

This patch also reports invalid/nonexistent helpers, which may or
may not be the right thing to do (but would be trivial to change
since the reporting code is in one place now).

diff --git a/src/auth/basic/auth_basic.cc b/src/auth/basic/auth_basic.cc
index 303bc3092e397496723d0258770931be829a67ba..790ede601dc45412e98d860d6153a9041b86fe36 100644 (file)
--- a/src/auth/basic/auth_basic.cc
+++ b/src/auth/basic/auth_basic.cc
@@ -1,5 +1,5 @@
 /*
- * $Id: auth_basic.cc,v 1.46 2007/04/28 22:26:48 hno Exp $
+ * $Id: auth_basic.cc,v 1.47 2007/05/07 18:38:40 wessels Exp $
  *
  * DEBUG: section 29    Authenticator
  * AUTHOR: Duane Wessels
@@ -346,8 +346,7 @@ AuthBasicConfig::parse(AuthConfig * scheme, int n_configured, char *param_str)
 static void
 authenticateBasicStats(StoreEntry * sentry)
 {
-    storeAppendPrintf(sentry, "Basic Authenticator Statistics:\n");
-    helperStats(sentry, basicauthenticators);
+    helperStats(sentry, basicauthenticators, "Basic Authenticator Statistics");
 }
 
 CBDATA_TYPE(AuthenticateStateData);

diff --git a/src/auth/digest/auth_digest.cc b/src/auth/digest/auth_digest.cc
index 28b58b46afaef2db6b6818d265398803e536810c..27ef3721cf8a1c5bb0c4cbccd1f79e25a8ed5907 100644 (file)
--- a/src/auth/digest/auth_digest.cc
+++ b/src/auth/digest/auth_digest.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: auth_digest.cc,v 1.52 2007/04/30 16:56:15 wessels Exp $
+ * $Id: auth_digest.cc,v 1.53 2007/05/07 18:38:40 wessels Exp $
  *
  * DEBUG: section 29    Authenticator
  * AUTHOR: Robert Collins
@@ -965,8 +965,7 @@ AuthDigestConfig::type() const
 static void
 authenticateDigestStats(StoreEntry * sentry)
 {
-    storeAppendPrintf(sentry, "Digest Authenticator Statistics:\n");
-    helperStats(sentry, digestauthenticators);
+    helperStats(sentry, digestauthenticators, "Digest Authenticator Statistics");
 }
 
 /* NonceUserUnlink: remove the reference to auth_user and unlink the node from the list */

diff --git a/src/auth/negotiate/auth_negotiate.cc b/src/auth/negotiate/auth_negotiate.cc
index 353b838494f159f7fe936d44678a9262cacbec5a..df59e6a59995371e8a86e301003c3e24ce76a343 100644 (file)
--- a/src/auth/negotiate/auth_negotiate.cc
+++ b/src/auth/negotiate/auth_negotiate.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: auth_negotiate.cc,v 1.14 2007/04/28 22:26:49 hno Exp $
+ * $Id: auth_negotiate.cc,v 1.15 2007/05/07 18:38:41 wessels Exp $
  *
  * DEBUG: section 29    Negotiate Authenticator
  * AUTHOR: Robert Collins, Henrik Nordstrom, Francesco Chemolli
@@ -503,8 +503,7 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply)
 static void
 authenticateNegotiateStats(StoreEntry * sentry)
 {
-    storeAppendPrintf(sentry, "NEGOTIATE Authenticator Statistics:\n");
-    helperStatefulStats(sentry, negotiateauthenticators);
+    helperStatefulStats(sentry, negotiateauthenticators, "NEGOTIATE Authenticator Statistics");
 }
 
 

diff --git a/src/auth/ntlm/auth_ntlm.cc b/src/auth/ntlm/auth_ntlm.cc
index 8685c86551174d4d762769887ab1f58bd413addf..818c17a24af4fa6a230b13f40f7607e59f614c59 100644 (file)
--- a/src/auth/ntlm/auth_ntlm.cc
+++ b/src/auth/ntlm/auth_ntlm.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: auth_ntlm.cc,v 1.63 2007/04/28 22:26:49 hno Exp $
+ * $Id: auth_ntlm.cc,v 1.64 2007/05/07 18:38:41 wessels Exp $
  *
  * DEBUG: section 29    NTLM Authenticator
  * AUTHOR: Robert Collins, Henrik Nordstrom, Francesco Chemolli
@@ -434,8 +434,7 @@ authenticateNTLMHandleReply(void *data, void *lastserver, char *reply)
 static void
 authenticateNTLMStats(StoreEntry * sentry)
 {
-    storeAppendPrintf(sentry, "NTLM Authenticator Statistics:\n");
-    helperStatefulStats(sentry, ntlmauthenticators);
+    helperStatefulStats(sentry, ntlmauthenticators, "NTLM Authenticator Statistics");
 }
 
 

diff --git a/src/helper.cc b/src/helper.cc
index b24e698d2a9addd3760ff8570a870e9ca1d3f31d..fa4f450de7496386deeb39f1051147e26d0c82ad 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: helper.cc,v 1.82 2007/05/04 15:40:12 rousskov Exp $
+ * $Id: helper.cc,v 1.83 2007/05/07 18:38:40 wessels Exp $
  *
  * DEBUG: section 84    Helper process maintenance
  * AUTHOR: Harvest Derived?
@@ -62,6 +62,8 @@ static void StatefulEnqueue(statefulhelper * hlp, helper_stateful_request * r);
 static helper_stateful_request *StatefulServerDequeue(helper_stateful_server * srv);
 static void StatefulServerEnqueue(helper_stateful_server * srv, helper_stateful_request * r);
 static void helperStatefulServerKickQueue(helper_stateful_server * srv);
+static bool helperStartStats(StoreEntry *sentry, void *hlp, const char *label);
+
 
 CBDATA_TYPE(helper);
 CBDATA_TYPE(helper_server);
@@ -506,8 +508,11 @@ helperStatefulServerGetData(helper_stateful_server * srv)
 }
 
 void
-helperStats(StoreEntry * sentry, helper * hlp)
+helperStats(StoreEntry * sentry, helper * hlp, const char *label)
 {
+    if (!helperStartStats(sentry, hlp, label))
+        return;
+
     dlink_node *link;
     storeAppendPrintf(sentry, "program: %s\n",
                       hlp->cmdline->key);
@@ -557,8 +562,11 @@ helperStats(StoreEntry * sentry, helper * hlp)
 }
 
 void
-helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp)
+helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label)
 {
+    if (!helperStartStats(sentry, hlp, label))
+        return;
+
     helper_stateful_server *srv;
     dlink_node *link;
     double tt;
@@ -1592,3 +1600,19 @@ helperStatefulRequestFree(helper_stateful_request * r)
     xfree(r->buf);
     delete r;
 }
+
+// TODO: should helper_ and helper_stateful_ have a common parent?
+static bool
+helperStartStats(StoreEntry *sentry, void *hlp, const char *label)
+{
+    if (!hlp) {
+        if (label)
+            storeAppendPrintf(sentry, "%s: unavailable\n", label);
+        return false;
+    }
+
+    if (label)
+        storeAppendPrintf(sentry, "%s:\n", label);
+
+    return true;
+}

diff --git a/src/helper.h b/src/helper.h
index 0d6fe303cbf502300dcb1b52df0e2e79eacd22a3..67992019d0f81f5e98b49c0fa06d4cddbf23dfe8 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: helper.h,v 1.8 2006/09/03 19:12:52 serassio Exp $
+ * $Id: helper.h,v 1.9 2007/05/07 18:38:40 wessels Exp $
  *
  * DEBUG: section 84    Helper process maintenance
  * AUTHOR: Harvest Derived?
@@ -238,8 +238,8 @@ SQUIDCEXTERN void helperOpenServers(helper * hlp);
 SQUIDCEXTERN void helperStatefulOpenServers(statefulhelper * hlp);
 SQUIDCEXTERN void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data);
 SQUIDCEXTERN void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, void *data, helper_stateful_server * lastserver);
-SQUIDCEXTERN void helperStats(StoreEntry * sentry, helper * hlp);
-SQUIDCEXTERN void helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp);
+SQUIDCEXTERN void helperStats(StoreEntry * sentry, helper * hlp, const char *label = NULL);
+SQUIDCEXTERN void helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label = NULL);
 SQUIDCEXTERN void helperShutdown(helper * hlp);
 SQUIDCEXTERN void helperStatefulShutdown(statefulhelper * hlp);
 SQUIDCEXTERN helper *helperCreate(const char *);

diff --git a/src/redirect.cc b/src/redirect.cc
index eaa351b1adbf4ae66834fe9e4dad637b77ce34e8..7cbf66314b7ada02758ad98b87868376a8742f82 100644 (file)
--- a/src/redirect.cc
+++ b/src/redirect.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: redirect.cc,v 1.117 2007/04/28 22:26:37 hno Exp $
+ * $Id: redirect.cc,v 1.118 2007/05/07 18:38:40 wessels Exp $
  *
  * DEBUG: section 61    Redirector
  * AUTHOR: Duane Wessels
@@ -101,8 +101,7 @@ redirectStats(StoreEntry * sentry)
         return;
     }
 
-    storeAppendPrintf(sentry, "Redirector Statistics:\n");
-    helperStats(sentry, redirectors);
+    helperStats(sentry, redirectors, "Redirector Statistics");
 
     if (Config.onoff.redirector_bypass)
         storeAppendPrintf(sentry, "\nNumber of requests bypassed "