On 11/01/2013 8:34 p.m., Amos Jeffries wrote:
As we are closing defects identified by Coverity and improving
constructors everywhere we are creating a minor anti-pattern in
MemPool'ed objects with calloc() in the pool initializing the memory
then constructors re-initializing it in a better way for that object.
MemPools contains a doZeroOnPush flag to optimize performance by
removing use of memset() as chunks are added back into the pool.
However, on closer inspection it is clear that the following pop()
process to re-use those chunks is never performing memset() anyway. As
such I believe that there is no special need to use calloc() on these
particular object types in the first place.
The attached patch updates MemPools to use malloc() instead of
calloc() on all types with doZeroOnPush set. This should increase
performance a little, and allows us to remove the anti-pattern by
setting doZeroOnPush for more objects as we can verify they are
correctly initialized by their constructors.
Amos
Updated patch which actually builds the MemPoolChunked class :-(
Amos
=== modified file 'lib/MemPoolChunked.cc'
--- lib/MemPoolChunked.cc 2012-09-01 14:38:36 +0000
+++ lib/MemPoolChunked.cc 2013-01-12 10:01:46 +0000
@@ -141,7 +141,11 @@
next = NULL;
pool = aPool;
- objCache = xcalloc(1, pool->chunk_size);
+ if (pool->doZeroOnPush)
+ objCache = xcalloc(1, pool->chunk_size);
+ else
+ objCache = xmalloc(pool->chunk_size);
+
freeList = objCache;
void **Free = (void **)freeList;
=== modified file 'lib/MemPoolMalloc.cc'
--- lib/MemPoolMalloc.cc 2012-09-01 14:38:36 +0000
+++ lib/MemPoolMalloc.cc 2013-01-11 07:16:57 +0000
@@ -56,7 +56,10 @@
memMeterDec(meter.idle);
++saved_calls;
} else {
- obj = xcalloc(1, obj_size);
+ if (doZeroOnPush)
+ obj = xcalloc(1, obj_size);
+ else
+ obj = xmalloc(obj_size);
memMeterInc(meter.alloc);
}
memMeterInc(meter.inuse);
=== modified file 'src/auth/Acl.cc'
--- src/auth/Acl.cc 2012-09-19 17:16:56 +0000
+++ src/auth/Acl.cc 2013-01-11 05:02:04 +0000
@@ -26,7 +26,7 @@
return ACCESS_DENIED;
} else if (request->flags.sslBumped) {
debugs(28, 5, "SslBumped request: It is an encapsulated request do not
authenticate");
- checklist->auth_user_request = checklist->conn() != NULL ?
checklist->conn()->auth_user_request : request->auth_user_request;
+ checklist->auth_user_request = checklist->conn() != NULL ?
checklist->conn()->getConnectionAuth() : request->auth_user_request;
if (checklist->auth_user_request != NULL)
return ACCESS_ALLOWED;
else
=== modified file 'src/auth/AclProxyAuth.cc'
--- src/auth/AclProxyAuth.cc 2012-08-14 11:53:07 +0000
+++ src/auth/AclProxyAuth.cc 2013-01-11 05:02:04 +0000
@@ -163,7 +163,7 @@
checklist->auth_user_request = NULL;
if (checklist->conn() != NULL) {
- checklist->conn()->auth_user_request = NULL;
+ checklist->conn()->setConnectionAuth(NULL, "proxy_auth ACL
failure");
}
}
=== modified file 'src/auth/UserRequest.cc'
--- src/auth/UserRequest.cc 2012-10-29 04:59:58 +0000
+++ src/auth/UserRequest.cc 2013-01-11 05:02:05 +0000
@@ -221,7 +221,7 @@
{}
void
-Auth::UserRequest::onConnectionClose(ConnStateData *)
+Auth::UserRequest::releaseAuthServer()
{}
const char *
@@ -253,7 +253,7 @@
else if (request != NULL && request->auth_user_request != NULL)
return request->auth_user_request;
else if (conn != NULL)
- return conn->auth_user_request;
+ return conn->getConnectionAuth();
else
return NULL;
}
@@ -303,7 +303,7 @@
/* connection auth we must reset on auth errors */
if (conn != NULL) {
- conn->auth_user_request = NULL;
+ conn->setConnectionAuth(NULL, "HTTP request missing credentials");
}
*auth_user_request = NULL;
@@ -315,13 +315,13 @@
* No check for function required in the if: its compulsory for conn based
* auth modules
*/
- if (proxy_auth && conn != NULL && conn->auth_user_request != NULL &&
- authenticateUserAuthenticated(conn->auth_user_request) &&
- conn->auth_user_request->connLastHeader() != NULL &&
- strcmp(proxy_auth, conn->auth_user_request->connLastHeader())) {
+ if (proxy_auth && conn != NULL && conn->getConnectionAuth() != NULL &&
+ authenticateUserAuthenticated(conn->getConnectionAuth()) &&
+ conn->getConnectionAuth()->connLastHeader() != NULL &&
+ strcmp(proxy_auth, conn->getConnectionAuth()->connLastHeader())) {
debugs(29, 2, "WARNING: DUPLICATE AUTH - authentication header on
already authenticated connection!. AU " <<
- conn->auth_user_request << ", Current user '" <<
- conn->auth_user_request->username() << "' proxy_auth " <<
+ conn->getConnectionAuth() << ", Current user '" <<
+ conn->getConnectionAuth()->username() << "' proxy_auth " <<
proxy_auth);
/* remove this request struct - the link is already authed and it
can't be to reauth. */
@@ -330,7 +330,7 @@
* authenticateAuthenticate
*/
assert(*auth_user_request == NULL);
- conn->auth_user_request = NULL;
+ conn->setConnectionAuth(NULL, "changed credentials token");
}
/* we have a proxy auth header and as far as we know this connection has
@@ -342,20 +342,20 @@
debugs(29, 9, HERE << "This is a new checklist test on:" <<
conn->clientConnection);
}
- if (proxy_auth && request->auth_user_request == NULL && conn != NULL
&& conn->auth_user_request != NULL) {
+ if (proxy_auth && request->auth_user_request == NULL && conn != NULL
&& conn->getConnectionAuth() != NULL) {
Auth::Config * scheme = Auth::Config::Find(proxy_auth);
- if (conn->auth_user_request->user() == NULL ||
conn->auth_user_request->user()->config != scheme) {
+ if (conn->getConnectionAuth()->user() == NULL ||
conn->getConnectionAuth()->user()->config != scheme) {
debugs(29, DBG_IMPORTANT, "WARNING: Unexpected change of
authentication scheme from '" <<
- conn->auth_user_request->user()->config->type() <<
+ conn->getConnectionAuth()->user()->config->type() <<
"' to '" << proxy_auth << "' (client " <<
src_addr << ")");
- conn->auth_user_request = NULL;
+ conn->setConnectionAuth(NULL, "changed auth scheme");
}
}
- if (request->auth_user_request == NULL && (conn == NULL ||
conn->auth_user_request == NULL)) {
+ if (request->auth_user_request == NULL && (conn == NULL ||
conn->getConnectionAuth() == NULL)) {
/* beginning of a new request check */
debugs(29, 4, HERE << "No connection authentication type");
@@ -378,14 +378,14 @@
*auth_user_request = request->auth_user_request;
} else {
assert (conn != NULL);
- if (conn->auth_user_request != NULL) {
- *auth_user_request = conn->auth_user_request;
+ if (conn->getConnectionAuth() != NULL) {
+ *auth_user_request = conn->getConnectionAuth();
} else {
/* failed connection based authentication */
debugs(29, 4, HERE << "Auth user request " <<
*auth_user_request << " conn-auth user request " <<
- conn->auth_user_request << " conn type " <<
- conn->auth_user_request->user()->auth_type << "
authentication failed.");
+ conn->getConnectionAuth() << " conn type " <<
+ conn->getConnectionAuth()->user()->auth_type << "
authentication failed.");
*auth_user_request = NULL;
return AUTH_ACL_CHALLENGE;
=== modified file 'src/auth/UserRequest.h'
--- src/auth/UserRequest.h 2012-11-04 12:27:49 +0000
+++ src/auth/UserRequest.h 2013-01-11 05:02:05 +0000
@@ -145,7 +145,7 @@
/* add the [Proxy-]Authentication-Info trailer */
virtual void addAuthenticationInfoTrailer(HttpReply * rep, int accel);
- virtual void onConnectionClose(ConnStateData *);
+ virtual void releaseAuthServer();
/**
* Called when squid is ready to put the request on hold and wait for a
callback from the auth module
=== modified file 'src/auth/digest/User.cc'
--- src/auth/digest/User.cc 2012-09-23 09:04:21 +0000
+++ src/auth/digest/User.cc 2013-01-11 05:35:50 +0000
@@ -9,7 +9,9 @@
Auth::Digest::User::User(Auth::Config *aConfig) :
Auth::User(aConfig),
HA1created(0)
-{}
+{
+ memset(HA1, 0, sizeof(HA1));
+}
Auth::Digest::User::~User()
{
=== modified file 'src/auth/negotiate/UserRequest.cc'
--- src/auth/negotiate/UserRequest.cc 2012-11-27 21:19:46 +0000
+++ src/auth/negotiate/UserRequest.cc 2013-01-11 05:02:08 +0000
@@ -129,27 +129,6 @@
debugs(29, 6, HERE << "No Negotiate auth server to release.");
}
-/* clear any connection related authentication details */
-void
-Auth::Negotiate::UserRequest::onConnectionClose(ConnStateData *conn)
-{
- assert(conn != NULL);
-
- debugs(29, 8, HERE << "closing connection '" << conn << "' (this is '" <<
this << "')");
-
- if (conn->auth_user_request == NULL) {
- debugs(29, 8, HERE << "no auth_user_request");
- return;
- }
-
- releaseAuthServer();
-
- /* unlock the connection based lock */
- debugs(29, 9, HERE << "Unlocking auth_user from the connection '" << conn
<< "'.");
-
- conn->auth_user_request = NULL;
-}
-
void
Auth::Negotiate::UserRequest::authenticate(HttpRequest * aRequest,
ConnStateData * conn, http_hdr_type type)
{
@@ -199,8 +178,8 @@
user()->credentials(Auth::Pending);
safe_free(client_blob);
client_blob=xstrdup(blob);
- assert(conn->auth_user_request == NULL);
- conn->auth_user_request = this;
+ assert(conn->getConnectionAuth() == NULL);
+ conn->setConnectionAuth(this, "new Negotiate handshake request");
request = aRequest;
HTTPMSGLOCK(request);
break;
=== modified file 'src/auth/negotiate/UserRequest.h'
--- src/auth/negotiate/UserRequest.h 2012-11-04 12:27:49 +0000
+++ src/auth/negotiate/UserRequest.h 2013-01-11 05:02:08 +0000
@@ -26,7 +26,6 @@
virtual int authenticated() const;
virtual void authenticate(HttpRequest * request, ConnStateData * conn,
http_hdr_type type);
virtual Direction module_direction();
- virtual void onConnectionClose(ConnStateData *);
virtual void module_start(AUTHCB *, void *);
virtual void addAuthenticationInfoHeader(HttpReply * rep, int accel);
=== modified file 'src/auth/ntlm/UserRequest.cc'
--- src/auth/ntlm/UserRequest.cc 2012-11-27 21:19:46 +0000
+++ src/auth/ntlm/UserRequest.cc 2013-01-11 05:02:08 +0000
@@ -123,26 +123,6 @@
}
void
-Auth::Ntlm::UserRequest::onConnectionClose(ConnStateData *conn)
-{
- assert(conn != NULL);
-
- debugs(29, 8, HERE << "closing connection '" << conn << "' (this is '" <<
this << "')");
-
- if (conn->auth_user_request == NULL) {
- debugs(29, 8, HERE << "no auth_user_request");
- return;
- }
-
- releaseAuthServer();
-
- /* unlock the connection based lock */
- debugs(29, 9, HERE << "Unlocking auth_user from the connection '" << conn
<< "'.");
-
- conn->auth_user_request = NULL;
-}
-
-void
Auth::Ntlm::UserRequest::authenticate(HttpRequest * aRequest, ConnStateData *
conn, http_hdr_type type)
{
assert(this);
@@ -192,8 +172,8 @@
user()->credentials(Auth::Pending);
safe_free(client_blob);
client_blob=xstrdup(blob);
- assert(conn->auth_user_request == NULL);
- conn->auth_user_request = this;
+ assert(conn->getConnectionAuth() == NULL);
+ conn->setConnectionAuth(this, "new NTLM handshake request");
request = aRequest;
HTTPMSGLOCK(request);
break;
=== modified file 'src/auth/ntlm/UserRequest.h'
--- src/auth/ntlm/UserRequest.h 2012-11-04 12:27:49 +0000
+++ src/auth/ntlm/UserRequest.h 2013-01-11 05:02:08 +0000
@@ -26,14 +26,13 @@
virtual int authenticated() const;
virtual void authenticate(HttpRequest * request, ConnStateData * conn,
http_hdr_type type);
virtual Auth::Direction module_direction();
- virtual void onConnectionClose(ConnStateData *);
virtual void module_start(AUTHCB *, void *);
virtual const char * connLastHeader();
/* we need to store the helper server between requests */
helper_stateful_server *authserver;
- void releaseAuthServer(void); ///< Release authserver NTLM helpers
properly when finished or abandoning.
+ virtual void releaseAuthServer(); ///< Release authserver NTLM helpers
properly when finished or abandoning.
/* our current blob to pass to the client */
char *server_blob;
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2013-01-11 10:02:21 +0000
+++ src/client_side.cc 2013-01-12 09:53:06 +0000
@@ -789,6 +789,82 @@
deleteThis("ConnStateData::connStateClosed");
}
+#if USE_AUTH
+void
+ConnStateData::setConnectionAuth(const Auth::UserRequest::Pointer &aur, const
char *by)
+{
+ if (credentialsState == NULL) {
+ debugs(33, 2, HERE << "Adding connection-auth to " << clientConnection
<< " from " << by);
+ credentialsState = aur;
+ return;
+ }
+
+ // clobered with self-pointer
+ // NP: something nasty is going on in Squid, but harmless.
+ if (aur == credentialsState) {
+ debugs(33, 2, HERE << "WARNING: Ignoring duplicate connection-auth for
" << clientConnection << " from " << by);
+ return;
+ }
+
+ /*
+ * Connection-auth relies on a single set of credentials being preserved
+ * for all requests on a connection once they have been setup.
+ * There are several things which need to happen to preserve security
+ * when connection-auth credentials change unexpectedly or are unset.
+ *
+ * 1) auth helper released from any active state
+ *
+ * They can only be reserved by a handshake process which this
+ * connection can now never complete.
+ * This prevents helpers hanging when their connections close.
+ *
+ * 2) pinning is expected to be removed and server conn closed
+ *
+ * The upstream link is authenticated with the same credentials.
+ * Expecting the same level of consistency we should have received.
+ * This prevents upstream being faced with multiple or missing
+ * credentials after authentication.
+ *
+ * 3) the connection needs to close.
+ *
+ * This prevents attackers injecting requests into a connection,
+ * or gateways wrongly multiplexing users into a single connection.
+ *
+ * When credentials are missing closure needs to follow an auth
+ * challenge for best recovery by the client.
+ *
+ * When credentials change there is nothing we can do but abort as
+ * fast as possible. If TCP RST can be sent instead of HTTP response
+ * that would be the best-case action.
+ */
+
+ // clobbered with nul-pointer
+ if (aur == NULL) {
+ debugs(33, 2, HERE << "WARNING: Closing " << clientConnection << " due
to connection-auth erase from " << by);
+ credentialsState->releaseAuthServer();
+ credentialsState = NULL;
+ unpinConnection();
+ // XXX: need to test whether the connection re-auth challenge is sent.
If not, how to trigger it from here.
+ // NP: the current situation seems to fix challenge loops in Safari
without visible issues in others.
+ deleteThis("connection-auth removed");
+ return;
+ }
+
+ // clobbered with alternative credentials
+ if (aur != credentialsState) {
+ debugs(33, 2, HERE << "ERROR: Closing " << clientConnection << " due
to change of connection-auth from " << by);
+ credentialsState->releaseAuthServer();
+ credentialsState = NULL;
+ unpinConnection();
+ clientConnection->close(); // XXX: need to send TCP RST packet if we
can.
+ deleteThis("connection-auth change attempted");
+ return;
+ }
+
+ /* NOT REACHABLE */
+}
+#endif
+
// cleans up before destructor is called
void
ConnStateData::swanSong()
@@ -798,10 +874,13 @@
clientdbEstablished(clientConnection->remote, -1); /* decrement */
assert(areAllContextsForThisConnection());
freeAllContexts();
+
#if USE_AUTH
- if (auth_user_request != NULL) {
- debugs(33, 4, "ConnStateData::swanSong: freeing auth_user_request '"
<< auth_user_request << "' (this is '" << this << "')");
- auth_user_request->onConnectionClose(this);
+ if (getConnectionAuth() != NULL) {
+ debugs(33, 4, "ConnStateData::swanSong: freeing connection-auth
credentials '" << getConnectionAuth() << "' (this is '" << this << "')");
+ // avoid setConnectionAuth() because of its side-effects, but ensure
the helpers are released properly
+ credentialsState->releaseAuthServer();
+ credentialsState = NULL;
}
#endif
@@ -2676,8 +2755,8 @@
!conn->port->allow_direct : 0;
#if USE_AUTH
if (request->flags.sslBumped) {
- if (conn->auth_user_request != NULL)
- request->auth_user_request = conn->auth_user_request;
+ if (conn->getConnectionAuth() != NULL)
+ request->auth_user_request = conn->getConnectionAuth();
}
#endif
=== modified file 'src/client_side.h'
--- src/client_side.h 2012-11-04 12:27:49 +0000
+++ src/client_side.h 2013-01-11 05:02:13 +0000
@@ -238,11 +238,28 @@
int64_t mayNeedToReadMoreBody() const;
#if USE_AUTH
+
+private:
+ /// state of some credentials that can be used to perform authentication
on this connection
+ Auth::UserRequest::Pointer credentialsState;
+public:
/**
* note this is ONLY connection based because NTLM and Negotiate is
against HTTP spec.
* the user details for connection based authentication
*/
- Auth::UserRequest::Pointer auth_user_request;
+ Auth::UserRequest::Pointer getConnectionAuth() const { return
credentialsState; };
+
+ /**
+ * Set the connection-based credentials to use from now until connection
closure.
+ *
+ * NP: once set the credentials are fixed until closure because for any
change to be needed
+ * since something invalid has happened:
+ * - NTLM/Negotiate auth was violated by the per-request headers missing a
revalidation token
+ * - NTLM/Negotiate auth was violated by the per-request headers being for
another user
+ * - SSL-Bump CONNECT tunnel with persistent credentials has ended
+ */
+ void setConnectionAuth(const Auth::UserRequest::Pointer &aur, const char
*cause);
+
#endif
/**
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2012-12-28 15:23:21 +0000
+++ src/client_side_request.cc 2013-01-11 05:02:13 +0000
@@ -613,8 +613,8 @@
http->request,
NULL,
#if USE_AUTH
- http->getConn() != NULL &&
http->getConn()->auth_user_request != NULL ?
- http->getConn()->auth_user_request :
http->request->auth_user_request);
+ http->getConn() != NULL &&
http->getConn()->getConnectionAuth() != NULL ?
+ http->getConn()->getConnectionAuth() :
http->request->auth_user_request);
#else
NULL);
#endif
@@ -782,8 +782,8 @@
#if USE_AUTH
char const *proxy_auth_msg = "<null>";
- if (http->getConn() != NULL && http->getConn()->auth_user_request != NULL)
- proxy_auth_msg =
http->getConn()->auth_user_request->denyMessage("<null>");
+ if (http->getConn() != NULL && http->getConn()->getConnectionAuth() !=
NULL)
+ proxy_auth_msg =
http->getConn()->getConnectionAuth()->denyMessage("<null>");
else if (http->request->auth_user_request != NULL)
proxy_auth_msg =
http->request->auth_user_request->denyMessage("<null>");
#endif
@@ -846,8 +846,8 @@
#if USE_AUTH
error->auth_user_request =
- http->getConn() != NULL && http->getConn()->auth_user_request !=
NULL ?
- http->getConn()->auth_user_request :
http->request->auth_user_request;
+ http->getConn() != NULL && http->getConn()->getConnectionAuth() !=
NULL ?
+ http->getConn()->getConnectionAuth() :
http->request->auth_user_request;
#endif
readNextRequest = true;
@@ -1497,7 +1497,7 @@
#if USE_AUTH
// Preserve authentication info for the ssl-bumped request
if (request->auth_user_request != NULL)
- getConn()->auth_user_request = request->auth_user_request;
+ getConn()->setConnectionAuth(request->auth_user_request, "SSL-bumped
CONNECT");
#endif
assert(sslBumpNeeded());
@@ -1964,7 +1964,7 @@
);
#if USE_AUTH
calloutContext->error->auth_user_request =
- c != NULL && c->auth_user_request != NULL ? c->auth_user_request :
request->auth_user_request;
+ c != NULL && c->getConnectionAuth() != NULL ?
c->getConnectionAuth() : request->auth_user_request;
#endif
calloutContext->error->detailError(errDetail);
calloutContext->readNextRequest = true;
=== modified file 'src/redirect.cc'
--- src/redirect.cc 2012-11-30 11:08:47 +0000
+++ src/redirect.cc 2013-01-11 05:02:45 +0000
@@ -279,8 +279,8 @@
http->request,
NULL,
#if USE_AUTH
- http->getConn() != NULL &&
http->getConn()->auth_user_request != NULL ?
- http->getConn()->auth_user_request :
http->request->auth_user_request);
+ http->getConn() != NULL &&
http->getConn()->getConnectionAuth() != NULL ?
+ http->getConn()->getConnectionAuth() :
http->request->auth_user_request);
#else
NULL);
#endif
=== modified file 'src/tests/Stub.list'
--- src/tests/Stub.list 2012-11-02 00:13:41 +0000
+++ src/tests/Stub.list 2013-01-11 04:59:49 +0000
@@ -17,6 +17,7 @@
tests/stub_cache_manager.cc \
tests/stub_cbdata.cc \
tests/stub_client_db.cc \
+ tests/stub_client_side.cc \
tests/stub_client_side_request.cc \
tests/stub_comm.cc \
tests/stub_debug.cc \
