> I've had a strange assert (at HttpHeader.cc:1551, in
> ~HttpHeaderEntry), but I could not reproduce that nor find anything in
> p2 or p3 of the refactor patch which may trigger it.
> I'm running more tests at full debugging.
Hit it again. Looking at HttpHeader, that code is really crappy.
The root cause is a double-free.
I suspect that the problem is caused by bad interactions in the
methods manipulating the entry array; it may be that Vector used to be
more forgiving than std::vector, especially with dealing with
out-of-bounds access and bad iterator maths (suspect: getEntry).
The attached patch worksforme(tm) for a few tens of thousands of hits
of real browsing. If you're OK with it I can clean it up and commit.
I suspect that HttpHeader.cc needs some love. Has anyone already
thought about this topic or should I prepare a proposal? I'd like to
share design ideas before going for an implementation attempt.
Thanks.
--
Kinkie
=== modified file 'src/HttpHeader.cc'
--- src/HttpHeader.cc 2014-02-10 15:07:49 +0000
+++ src/HttpHeader.cc 2014-02-11 16:13:35 +0000
@@ -410,99 +410,115 @@ HttpHeader::HttpHeader(const http_hdr_ow
httpHeaderMaskInit(&mask, 0);
}
HttpHeader::HttpHeader(const HttpHeader &other): owner(other.owner), len(other.len)
{
httpHeaderMaskInit(&mask, 0);
update(&other, NULL); // will update the mask as well
}
HttpHeader::~HttpHeader()
{
clean();
}
HttpHeader &
HttpHeader::operator =(const HttpHeader &other)
{
if (this != &other) {
// we do not really care, but the caller probably does
assert(owner == other.owner);
clean();
update(&other, NULL); // will update the mask as well
len = other.len;
}
return *this;
}
void
HttpHeader::clean()
{
- HttpHeaderPos pos = HttpHeaderInitPos;
- HttpHeaderEntry *e;
assert(owner > hoNone && owner < hoEnd);
debugs(55, 7, "cleaning hdr: " << this << " owner: " << owner);
PROF_start(HttpHeaderClean);
if (owner <= hoReply) {
/*
* An unfortunate bug. The entries array is initialized
* such that count is set to zero. httpHeaderClean() seems to
* be called both when 'hdr' is created, and destroyed. Thus,
* we accumulate a large number of zero counts for 'hdr' before
* it is ever used. Can't think of a good way to fix it, except
* adding a state variable that indicates whether or not 'hdr'
* has been used. As a hack, just never count zero-sized header
* arrays.
*/
if (!entries.empty())
HttpHeaderStats[owner].hdrUCountDistr.count(entries.size());
++ HttpHeaderStats[owner].destroyedCount;
HttpHeaderStats[owner].busyDestroyedCount += entries.size() > 0;
} // if (owner <= hoReply)
+#if 0
+ HttpHeaderEntry *e;
+ HttpHeaderPos pos = HttpHeaderInitPos;
+
while ((e = getEntry(&pos))) {
/* tmp hack to try to avoid coredumps */
if (e->id < 0 || e->id >= HDR_ENUM_END) {
debugs(55, DBG_CRITICAL, "HttpHeader::clean BUG: entry[" << pos << "] is invalid (" << e->id << "). Ignored.");
} else {
if (owner <= hoReply)
HttpHeaderStats[owner].fieldTypeDistr.count(e->id);
/* yes, this deletion leaves us in an inconsistent state */
delete e;
}
}
+#else
+ for (std::vector<HttpHeaderEntry *>::iterator i = entries.begin(); i != entries.end(); ++i) {
+ HttpHeaderEntry *e = *i;
+ if (e == NULL)
+ continue;
+ if (e->id < 0 || e->id >= HDR_ENUM_END) {
+ debugs(55, DBG_CRITICAL, "BUG: invalid entry (" << e->id << "). Ignored.");
+ } else {
+ if (owner <= hoReply)
+ HttpHeaderStats[owner].fieldTypeDistr.count(e->id);
+ delete e;
+ }
+ }
+#endif
entries.clear();
httpHeaderMaskInit(&mask, 0);
len = 0;
PROF_stop(HttpHeaderClean);
}
/* append entries (also see httpHeaderUpdate) */
void
HttpHeader::append(const HttpHeader * src)
{
const HttpHeaderEntry *e;
HttpHeaderPos pos = HttpHeaderInitPos;
assert(src);
assert(src != this);
debugs(55, 7, "appending hdr: " << this << " += " << src);
while ((e = src->getEntry(&pos))) {
addEntry(e->clone());
}
}
/* use fresh entries to replace old ones */
void
httpHeaderUpdate(HttpHeader * old, const HttpHeader * fresh, const HttpHeaderMask * denied_mask)
{
assert (old);
old->update (fresh, denied_mask);
}
void
