Hello,
Squid calls cbdataFree() for objects that have non-POD data members
such as refcounted pointers. Since cbdataFree() does not call the object
destructor when freeing object memory, those data members are not
properly destroyed, leading to leaks (at best). Some code tries to reset
those pointers before calling cbdataFree(), but, naturally, there are
quite a few places were we fail to cleanup. The other places are just
ticking bombs until somebody adds a sensitive non-POD data member. See
bug #3133 for an example.
The attached untested patch tries to fix the bugs I could identify, but
I wonder whether it would be better to remove cbdataAlloc/Free and call
new/delete instead (to properly execute constructors and destructors)?
Such a rewrite would require adding CBDATA_CLASS2 macros to some
structs, but it will be much safer than trying to find all such bugs and
to monitor that new ones do not crop up (especially where a non-POD
member is added indirectly -- to a type used by a previously POD class
member; for example String added to Ip::Address used by many
cbdataFree()d types).
I think this transition can be done more-or-less safely by first
adjusting cbdataAlloc/Free() macros so that they fail to compile if the
corresponding class does not have a special member added by
CBDATA_CLASS2 macros. This way, we will not forget to add CBDATA_CLASS2
to any class/struct that uses cbdataFree(). cbdataFree() also sets the
pointer to NULL so we would still have to manually check that it is not
needed if we want to remove that macro.
Any better ideas?
Thank you,
Alex.
P.S. I am also concerned what happens when cbdataReferenceDone() reaches
zero lock counter for a non-POD object. Will need to get some sleep
before researching that :-).
Squid calls cbdataFree() for objects that have non-POD data members such as
refcounted pointers. Since cbdataFree() does not call the object destructor
when freeing object memory, those data members are not properly destroyed,
leading to leaks (at best). Some code tries to reset those pointers before
calling cbdataFree(), but, naturally, there are quite a few places were we
fail to cleanup. The other places are just ticking bombs until somebody adds
a sensitive non-POD data member.
TODO: replacee cbdataAlloc/Free() with new/delete?
=== modified file 'src/acl/Gadgets.cc'
--- src/acl/Gadgets.cc 2012-07-02 12:28:10 +0000
+++ src/acl/Gadgets.cc 2012-07-07 05:13:12 +0000
@@ -260,41 +260,41 @@
ACLList *l;
debugs(28, 8, "aclDestroyAclList: invoked");
for (l = *head; l; l = *head) {
*head = l->next;
delete l;
}
}
void
aclDestroyAccessList(acl_access ** list)
{
acl_access *l = NULL;
acl_access *next = NULL;
for (l = *list; l; l = next) {
debugs(28, 3, "aclDestroyAccessList: '" << l->cfgline << "'");
next = l->next;
aclDestroyAclList(&l->aclList);
safe_free(l->cfgline);
- cbdataFree(l);
+ delete l;
}
*list = NULL;
}
/* [email protected] (06.09.1996)
* destroy an acl_deny_info_list */
void
aclDestroyDenyInfoList(acl_deny_info_list ** list)
{
acl_deny_info_list *a = NULL;
acl_deny_info_list *a_next = NULL;
acl_name_list *l = NULL;
acl_name_list *l_next = NULL;
debugs(28, 8, "aclDestroyDenyInfoList: invoked");
for (a = *list; a; a = a_next) {
for (l = a->acl_list; l; l = l_next) {
=== modified file 'src/dns_internal.cc'
--- src/dns_internal.cc 2012-03-05 11:36:38 +0000
+++ src/dns_internal.cc 2012-07-07 05:16:40 +0000
@@ -814,41 +814,41 @@
comm_read(conn, (char *)&vc->msglen, 2, call);
vc->busy = 0;
idnsDoSendQueryVC(vc);
}
static void
idnsVCClosed(const CommCloseCbParams ¶ms)
{
nsvc * vc = (nsvc *)params.data;
delete vc->queue;
delete vc->msg;
vc->conn = NULL;
if (vc->ns < nns) // XXX: dnsShutdown may have freed nameservers[]
nameservers[vc->ns].vc = NULL;
cbdataFree(vc);
}
static void
idnsInitVC(int ns)
{
- nsvc *vc = cbdataAlloc(nsvc);
+ nsvc *vc = cbdataAlloc(nsvc); // XXX: but has Comm::ConnectionPointer
assert(ns < nns);
assert(vc->conn == NULL); // MUST be NULL from the construction process!
nameservers[ns].vc = vc;
vc->ns = ns;
vc->queue = new MemBuf;
vc->msg = new MemBuf;
vc->busy = 1;
Comm::ConnectionPointer conn = new Comm::Connection();
if (!Config.Addrs.udp_outgoing.IsNoAddr())
conn->local = Config.Addrs.udp_outgoing;
else
conn->local = Config.Addrs.udp_incoming;
conn->remote = nameservers[ns].S;
if (conn->remote.IsIPv4()) {
conn->local.SetIPv4();
}
=== modified file 'src/gopher.cc'
--- src/gopher.cc 2012-01-20 18:55:04 +0000
+++ src/gopher.cc 2012-07-07 05:32:05 +0000
@@ -159,40 +159,41 @@
/// \ingroup ServerProtocolGopherInternal
static char def_gopher_text[] = "text/plain";
/// \ingroup ServerProtocolGopherInternal
static void
gopherStateFree(const CommCloseCbParams ¶ms)
{
GopherStateData *gopherState = (GopherStateData *)params.data;
if (gopherState == NULL)
return;
if (gopherState->entry) {
gopherState->entry->unlock();
}
HTTPMSGUNLOCK(gopherState->req);
gopherState->fwd = NULL; // refcounted
+ gopherState->serverConn = NULL; // refcounted
memFree(gopherState->buf, MEM_4K_BUF);
gopherState->buf = NULL;
cbdataFree(gopherState);
}
/**
\ingroup ServerProtocolGopherInternal
* Create MIME Header for Gopher Data
*/
static void
gopherMimeCreate(GopherStateData * gopherState)
{
StoreEntry *entry = gopherState->entry;
const char *mime_type = NULL;
const char *mime_enc = NULL;
switch (gopherState->type_id) {
case GOPHER_DIRECTORY:
@@ -939,41 +940,41 @@
debugs(10, 5, HERE << gopherState->serverConn);
AsyncCall::Pointer call = commCbCall(5,5, "gopherSendComplete",
CommIoCbPtrFun(gopherSendComplete, gopherState));
Comm::Write(gopherState->serverConn, buf, strlen(buf), call, NULL);
if (EBIT_TEST(gopherState->entry->flags, ENTRY_CACHABLE))
gopherState->entry->setPublicKey(); /* Make it public */
}
/// \ingroup ServerProtocolGopherInternal
CBDATA_TYPE(GopherStateData);
/// \ingroup ServerProtocolGopherAPI
void
gopherStart(FwdState * fwd)
{
StoreEntry *entry = fwd->entry;
GopherStateData *gopherState;
CBDATA_INIT_TYPE(GopherStateData);
- gopherState = cbdataAlloc(GopherStateData);
+ gopherState = cbdataAlloc(GopherStateData); // XXX: but has Pointers
gopherState->buf = (char *)memAllocate(MEM_4K_BUF);
entry->lock();
gopherState->entry = entry;
gopherState->fwd = fwd;
debugs(10, 3, "gopherStart: " << entry->url() );
statCounter.server.all.requests++;
statCounter.server.other.requests++;
/* Parse url. */
gopher_request_parse(fwd->request,
&gopherState->type_id, gopherState->request);
comm_add_close_handler(fwd->serverConnection()->fd, gopherStateFree, gopherState);
if (((gopherState->type_id == GOPHER_INDEX) || (gopherState->type_id == GOPHER_CSO))
=== modified file 'src/helper.h'
--- src/helper.h 2012-01-20 18:55:04 +0000
+++ src/helper.h 2012-07-07 04:52:46 +0000
@@ -53,41 +53,41 @@
public:
wordlist *cmdline;
dlink_list servers;
dlink_list queue;
const char *id_name;
HelperChildConfig childs; ///< Configuration settings for number running.
int ipc_type;
Ip::Address addr;
time_t last_queue_warn;
time_t last_restart;
char eom; ///< The char which marks the end of (response) message, normally '\n'
struct _stats {
int requests;
int replies;
int queue_size;
int avg_svc_time;
} stats;
private:
- CBDATA_CLASS2(helper);
+ CBDATA_CLASS2(helper); // XXX: but uses cbdataAlloc/cbdataFree
};
class statefulhelper : public helper
{
public:
inline statefulhelper(const char *name) : helper(name) {};
inline ~statefulhelper() {};
public:
MemAllocator *datapool;
HLPSAVAIL *IsAvailable;
HLPSONEQ *OnEmptyQueue;
private:
CBDATA_CLASS2(statefulhelper);
};
/*
* Fields shared between stateless and stateful helper servers.
*/
=== modified file 'src/ident/Ident.cc'
--- src/ident/Ident.cc 2012-01-20 18:55:04 +0000
+++ src/ident/Ident.cc 2012-07-07 05:23:42 +0000
@@ -89,40 +89,41 @@
if (result && *result == '\0')
result = NULL;
while ((client = state->clients)) {
void *cbdata;
state->clients = client->next;
if (cbdataReferenceValidDone(client->callback_data, &cbdata))
client->callback(result, cbdata);
xfree(client);
}
}
void
Ident::Close(const CommCloseCbParams ¶ms)
{
IdentStateData *state = (IdentStateData *)params.data;
identCallback(state, NULL);
state->conn->close();
+ state->conn = NULL;
hash_remove_link(ident_hash, (hash_link *) state);
xfree(state->hash.key);
cbdataFree(state);
}
void
Ident::Timeout(const CommTimeoutCbParams &io)
{
debugs(30, 3, HERE << io.conn);
io.conn->close();
}
void
Ident::ConnectDone(const Comm::ConnectionPointer &conn, comm_err_t status, int xerrno, void *data)
{
IdentStateData *state = (IdentStateData *)data;
if (status != COMM_OK) {
if (status == COMM_TIMEOUT) {
debugs(30, 3, "IDENT connection timeout to " << state->conn->remote);
@@ -227,41 +228,41 @@
Ident::Start(const Comm::ConnectionPointer &conn, IDCB * callback, void *data)
{
IdentStateData *state;
char key1[IDENT_KEY_SZ];
char key2[IDENT_KEY_SZ];
char key[IDENT_KEY_SZ];
conn->local.ToURL(key1, IDENT_KEY_SZ);
conn->remote.ToURL(key2, IDENT_KEY_SZ);
snprintf(key, IDENT_KEY_SZ, "%s,%s", key1, key2);
if (!ident_hash) {
Init();
}
if ((state = (IdentStateData *)hash_lookup(ident_hash, key)) != NULL) {
ClientAdd(state, callback, data);
return;
}
CBDATA_INIT_TYPE(IdentStateData);
- state = cbdataAlloc(IdentStateData);
+ state = cbdataAlloc(IdentStateData); // XXX: uses Comm::ConnectionPointer
state->hash.key = xstrdup(key);
// copy the conn details. We dont want the original FD to be re-used by IDENT.
state->conn = conn->copyDetails();
// NP: use random port for secure outbound to IDENT_PORT
state->conn->local.SetPort(0);
ClientAdd(state, callback, data);
hash_join(ident_hash, &state->hash);
AsyncCall::Pointer call = commCbCall(30,3, "Ident::ConnectDone", CommConnectCbPtrFun(Ident::ConnectDone, state));
AsyncJob::Start(new Comm::ConnOpener(state->conn, call, Ident::TheConfig.timeout));
}
void
Ident::Init(void)
{
if (ident_hash) {
debugs(30, DBG_CRITICAL, "WARNING: Ident already initialized.");
return;
=== modified file 'src/neighbors.cc'
--- src/neighbors.cc 2012-04-25 05:29:20 +0000
+++ src/neighbors.cc 2012-07-07 05:02:34 +0000
@@ -1407,41 +1407,41 @@
{
ps_state *psstate = (ps_state *)data;
StoreEntry *fake = psstate->entry;
if (cbdataReferenceValid(psstate->callback_data)) {
peer *p = (peer *)psstate->callback_data;
p->mcast.flags.counting = 0;
p->mcast.avg_n_members = Math::doubleAverage(p->mcast.avg_n_members, (double) psstate->ping.n_recv, ++p->mcast.n_times_counted, 10);
debugs(15, 1, "Group " << p->host << ": " << psstate->ping.n_recv <<
" replies, "<< std::setw(4)<< std::setprecision(2) <<
p->mcast.avg_n_members <<" average, RTT " << p->stats.rtt);
p->mcast.n_replies_expected = (int) p->mcast.avg_n_members;
}
cbdataReferenceDone(psstate->callback_data);
fake->abort(); // sets ENTRY_ABORTED and initiates releated cleanup
HTTPMSGUNLOCK(fake->mem_obj->request);
fake->unlock();
HTTPMSGUNLOCK(psstate->request);
- cbdataFree(psstate);
+ delete psstate; // XXX: but there is also peerSelectStateFree()
}
static void
peerCountHandleIcpReply(peer * p, peer_t type, AnyP::ProtocolType proto, void *hdrnotused, void *data)
{
int rtt_av_factor;
ps_state *psstate = (ps_state *)data;
StoreEntry *fake = psstate->entry;
MemObject *mem = fake->mem_obj;
int rtt = tvSubMsec(mem->start_ping, current_time);
assert(proto == AnyP::PROTO_ICP);
assert(fake);
assert(mem);
psstate->ping.n_recv++;
rtt_av_factor = RTT_AV_FACTOR;
if (p->options.weighted_roundrobin)
rtt_av_factor = RTT_BACKGROUND_AV_FACTOR;
=== modified file 'src/peer_select.cc'
--- src/peer_select.cc 2012-05-08 01:21:10 +0000
+++ src/peer_select.cc 2012-07-07 04:58:18 +0000
@@ -90,41 +90,41 @@
eventDelete(peerPingTimeout, psstate);
psstate->entry->ping_status = PING_DONE;
}
if (psstate->acl_checklist) {
debugs(44, 1, "calling aclChecklistFree() from peerSelectStateFree");
delete (psstate->acl_checklist);
}
HTTPMSGUNLOCK(psstate->request);
if (psstate->entry) {
assert(psstate->entry->ping_status != PING_WAITING);
psstate->entry->unlock();
psstate->entry = NULL;
}
delete psstate->lastError;
- cbdataFree(psstate);
+ delete psstate;
}
static int
peerSelectIcpPing(HttpRequest * request, int direct, StoreEntry * entry)
{
int n;
assert(entry);
assert(entry->ping_status == PING_NONE);
assert(direct != DIRECT_YES);
debugs(44, 3, "peerSelectIcpPing: " << entry->url() );
if (!request->flags.hierarchical && direct != DIRECT_NO)
return 0;
if (EBIT_TEST(entry->flags, KEY_PRIVATE) && !neighbors_do_private_keys)
if (direct != DIRECT_NO)
return 0;
n = neighborsCount(request);
=== modified file 'src/stat.cc'
--- src/stat.cc 2012-06-22 03:49:38 +0000
+++ src/stat.cc 2012-07-07 05:05:44 +0000
@@ -371,45 +371,45 @@
e->swap_dirn, e->swap_filen);
if (mem != NULL)
mem->stat (mb);
mb->Printf("\n");
}
/* process objects list */
static void
statObjects(void *data)
{
StatObjectsState *state = static_cast<StatObjectsState *>(data);
StoreEntry *e;
if (state->theSearch->isDone()) {
if (UsingSmp())
storeAppendPrintf(state->sentry, "} by kid%d\n\n", KidIdentifier);
state->sentry->complete();
state->sentry->unlock();
- cbdataFree(state);
+ delete state;
return;
} else if (EBIT_TEST(state->sentry->flags, ENTRY_ABORTED)) {
state->sentry->unlock();
- cbdataFree(state);
+ delete state;
return;
} else if (state->sentry->checkDeferRead(-1)) {
state->sentry->flush();
eventAdd("statObjects", statObjects, state, 0.1, 1);
return;
}
state->sentry->buffer();
size_t statCount = 0;
MemBuf mb;
mb.init();
while (statCount++ < static_cast<size_t>(Config.Store.objectsPerBucket) && state->
theSearch->next()) {
e = state->theSearch->currentItem();
if (state->filter && 0 == state->filter(e))
continue;
statStoreEntry(&mb, e);
=== modified file 'src/store_swapout.cc'
--- src/store_swapout.cc 2012-02-10 00:01:17 +0000
+++ src/store_swapout.cc 2012-07-07 05:09:04 +0000
@@ -289,41 +289,42 @@
StoreEntry::swapOutFileClose(int how)
{
assert(mem_obj != NULL);
debugs(20, 3, "storeSwapOutFileClose: " << getMD5Text() << " how=" << how);
debugs(20, 3, "storeSwapOutFileClose: sio = " << mem_obj->swapout.sio.getRaw());
if (mem_obj->swapout.sio == NULL)
return;
storeClose(mem_obj->swapout.sio, how);
}
static void
storeSwapOutFileClosed(void *data, int errflag, StoreIOState::Pointer self)
{
generic_cbdata *c = (generic_cbdata *)data;
StoreEntry *e = (StoreEntry *)c->data;
MemObject *mem = e->mem_obj;
assert(mem->swapout.sio == self);
assert(e->swap_status == SWAPOUT_WRITING);
- cbdataFree(c);
+ delete c;
+ c = NULL;
// if object_size is still unknown, the entry was probably aborted
if (errflag || e->objectLen() < 0) {
debugs(20, 2, "storeSwapOutFileClosed: dirno " << e->swap_dirn << ", swapfile " <<
std::hex << std::setw(8) << std::setfill('0') << std::uppercase <<
e->swap_filen << ", errflag=" << errflag);
if (errflag == DISK_NO_SPACE_LEFT) {
/* FIXME: this should be handle by the link from store IO to
* Store, rather than being a top level API call.
*/
e->store()->diskFull();
storeConfigure();
}
if (e->swap_filen >= 0)
e->unlink();
assert(e->swap_status == SWAPOUT_NONE);
=== modified file 'src/whois.cc'
--- src/whois.cc 2012-01-20 18:55:04 +0000
+++ src/whois.cc 2012-07-07 05:25:44 +0000
@@ -69,41 +69,41 @@
CBDATA_TYPE(WhoisState);
WhoisState::~WhoisState()
{
fwd = NULL; // refcounted
}
static void
whoisWriteComplete(const Comm::ConnectionPointer &, char *buf, size_t size, comm_err_t flag, int xerrno, void *data)
{
xfree(buf);
}
void
whoisStart(FwdState * fwd)
{
WhoisState *p;
char *buf;
size_t l;
CBDATA_INIT_TYPE(WhoisState);
- p = cbdataAlloc(WhoisState);
+ p = cbdataAlloc(WhoisState); // XXX: but uses FwdState::Pointer
p->request = fwd->request;
p->entry = fwd->entry;
p->fwd = fwd;
p->dataWritten = false;
p->entry->lock();
comm_add_close_handler(fwd->serverConnection()->fd, whoisClose, p);
l = p->request->urlpath.size() + 3;
buf = (char *)xmalloc(l);
String str_print=p->request->urlpath.substr(1,p->request->urlpath.size());
snprintf(buf, l, SQUIDSTRINGPH"\r\n", SQUIDSTRINGPRINT(str_print));
AsyncCall::Pointer writeCall = commCbCall(5,5, "whoisWriteComplete",
CommIoCbPtrFun(whoisWriteComplete, p));
Comm::Write(fwd->serverConnection(), buf, strlen(buf), writeCall, NULL);
AsyncCall::Pointer readCall = commCbCall(5,4, "whoisReadReply",
CommIoCbPtrFun(whoisReadReply, p));
@@ -185,22 +185,23 @@
}
/* no bytes read. stop reading */
entry->timestampsSet();
entry->flush();
if (!EBIT_TEST(entry->flags, RELEASE_REQUEST))
entry->setPublicKey();
fwd->complete();
debugs(75, 3, "whoisReadReply: Done: " << entry->url());
conn->close();
}
static void
whoisClose(const CommCloseCbParams ¶ms)
{
WhoisState *p = (WhoisState *)params.data;
debugs(75, 3, "whoisClose: FD " << params.fd);
p->entry->unlock();
+ p->fwd = NULL;
cbdataFree(p);
}
