From: Eduard Bagdasaryan Date: Fri, 15 Sep 2017 14:13:01 +0000 (+0300) Subject: Bug 4616: store_client.cc:92: "mem" assertion (#50) X-Git-Tag: M-staged-PR71~50 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=76d61119ed913c9edd68aeb59ede23c10f119b34;p=thirdparty%2Fsquid.git Bug 4616: store_client.cc:92: "mem" assertion (#50) This bug was probably caused by Bug 2833 feature/fix (1a210de). The primary fix here is limited to clientReplyContext::processExpired(): Collapsed forwarding code must ensure StoreEntry::mem_obj existence. It was missing for cache hits purged from (or never admitted into) the memory cache. Most storeClientListAdd() callers either have similar code or call storeCreateEntry() which also creates StoreEntry::mem_obj. Also avoided clobbering known StoreEntry URIs/method in some cases. The known effect of this change is fixed store.log URI and method fields when a hit transaction did not match the stored entry exactly (e.g., a HEAD hit for a GET cached entry), but this improvement may have even more important consequences: The original method is used by possibly still-running entry filling code (e.g., determining the end of the incoming response, validating the entry length, finding vary markers, etc.). Changing the method affects those actions, essentially corrupting the entry state. The same argument may apply to store ID and log URI. We even tried to make URIs/method constant, but that is impractical w/o addressing an XXX in MemStore::get(), which is outside this issue scope. To facilitate that future fix, the code now distinguishes these cases: * createMemObject(void): Buggy callers that create a new memory object but do not know what URIs/method the hosting StoreEntry was based on. Once these callers are fixed, we can make the URIs/method constant. * createMemObject(trio): Callers that create a new memory object with URIs/method that match the hosting StoreEntry. * ensureMemObject(trio): Callers that are not sure whether StoreEntry has a memory object but have URIs/method to create one if needed. --- diff --git a/src/MemObject.cc b/src/MemObject.cc index d608a21c53..53cf1c53b9 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -76,7 +76,11 @@ MemObject::hasUris() const void MemObject::setUris(char const *aStoreId, char const *aLogUri, const HttpRequestMethod &aMethod) { + if (hasUris()) + return; + storeId_ = aStoreId; + debugs(88, 3, this << " storeId: " << storeId_); // fast pointer comparison for a common storeCreateEntry(url,url,...) case if (!aLogUri || aLogUri == aStoreId) diff --git a/src/MemObject.h b/src/MemObject.h index 23d716ef22..35b1376157 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -39,7 +39,12 @@ public: MemObject(); ~MemObject(); - /// sets store ID, log URI, and request method; TODO: find a better name + /// Sets store ID, log URI, and request method (unless already set). Does + /// not clobber the method so that, say, a HEAD hit for a GET entry keeps + /// the GET method that matches the entry key. Same for the other parts of + /// the trio because the entry filling code may expect them to be constant. + /// XXX: Avoid this method. We plan to remove it and make the trio constant + /// after addressing the XXX in MemStore::get(). void setUris(char const *aStoreId, char const *aLogUri, const HttpRequestMethod &aMethod); /// whether setUris() has been called diff --git a/src/MemStore.cc b/src/MemStore.cc index ad65db20c9..49eb2112fb 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -323,7 +323,7 @@ MemStore::get(const cache_key *key) // XXX: We do not know the URLs yet, only the key, but we need to parse and // store the response for the Root().get() callers to be happy because they // expect IN_MEMORY entries to already have the response headers and body. - e->makeMemObject(); + e->createMemObject(); anchorEntry(*e, index, *slot); diff --git a/src/Store.h b/src/Store.h index cb31b5c33d..a1f6ef9720 100644 --- a/src/Store.h +++ b/src/Store.h @@ -115,12 +115,16 @@ public: int validToSend() const; bool memoryCachable(); ///< checkCachable() and can be cached in memory - /// if needed, initialize mem_obj member w/o URI-related information - MemObject *makeMemObject(); + /// initialize mem_obj; assert if mem_obj already exists + /// avoid this method in favor of createMemObject(trio)! + void createMemObject(); - /// initialize mem_obj member (if needed) and supply URI-related info + /// initialize mem_obj with URIs/method; assert if mem_obj already exists void createMemObject(const char *storeId, const char *logUri, const HttpRequestMethod &aMethod); + /// initialize mem_obj (if needed) and set URIs/method (if missing) + void ensureMemObject(const char *storeId, const char *logUri, const HttpRequestMethod &aMethod); + void dump(int debug_lvl) const; void hashDelete(); void hashInsert(const cache_key *); diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 5abfe917ab..f6ae6ea4be 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -308,6 +308,7 @@ clientReplyContext::processExpired() } if (entry) { + entry->ensureMemObject(url, http->log_uri, http->request->method); debugs(88, 5, "collapsed on existing revalidation entry: " << *entry); collapsedRevalidation = crSlave; } else { @@ -993,7 +994,7 @@ clientReplyContext::purgeFoundObject(StoreEntry *entry) http->storeEntry(entry); http->storeEntry()->lock("clientReplyContext::purgeFoundObject"); - http->storeEntry()->createMemObject(storeId(), http->log_uri, + http->storeEntry()->ensureMemObject(storeId(), http->log_uri, http->request->method); sc = storeClientListAdd(http->storeEntry(), this); @@ -1845,22 +1846,7 @@ clientReplyContext::doGetMoreData() http->storeEntry()->lock("clientReplyContext::doGetMoreData"); - MemObject *mem_obj = http->storeEntry()->makeMemObject(); - if (!mem_obj->hasUris()) { - /* - * This if-block exists because we don't want to clobber - * a preexiting mem_obj->method value if the mem_obj - * already exists. For example, when a HEAD request - * is a cache hit for a GET response, we want to keep - * the method as GET. - */ - mem_obj->setUris(storeId(), http->log_uri, http->request->method); - /** - * Here we can see if the object was - * created using URL or alternative StoreID from helper. - */ - debugs(88, 3, "storeId: " << http->storeEntry()->mem_obj->storeId()); - } + http->storeEntry()->ensureMemObject(storeId(), http->log_uri, http->request->method); sc = storeClientListAdd(http->storeEntry(), this); #if USE_DELAY_POOLS diff --git a/src/store.cc b/src/store.cc index 9e4190fb95..2ffde12c8c 100644 --- a/src/store.cc +++ b/src/store.cc @@ -779,8 +779,7 @@ storeCreatePureEntry(const char *url, const char *log_url, const RequestFlags &f debugs(20, 3, "storeCreateEntry: '" << url << "'"); e = new StoreEntry(); - e->makeMemObject(); - e->mem_obj->setUris(url, log_url, method); + e->createMemObject(url, log_url, method); if (flags.cachable) { EBIT_CLR(e->flags, RELEASE_REQUEST); @@ -1678,18 +1677,25 @@ StoreEntry::url() const return mem_obj->storeId(); } -MemObject * -StoreEntry::makeMemObject() +void +StoreEntry::createMemObject() { - if (!mem_obj) - mem_obj = new MemObject(); - return mem_obj; + assert(!mem_obj); + mem_obj = new MemObject(); } void StoreEntry::createMemObject(const char *aUrl, const char *aLogUrl, const HttpRequestMethod &aMethod) { - makeMemObject(); + assert(!mem_obj); + ensureMemObject(aUrl, aLogUrl, aMethod); +} + +void +StoreEntry::ensureMemObject(const char *aUrl, const char *aLogUrl, const HttpRequestMethod &aMethod) +{ + if (!mem_obj) + mem_obj = new MemObject(); mem_obj->setUris(aUrl, aLogUrl, aMethod); } diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc index 5c6c38f128..9ca29a3593 100644 --- a/src/tests/stub_store.cc +++ b/src/tests/stub_store.cc @@ -54,8 +54,9 @@ int StoreEntry::checkNegativeHit() const STUB_RETVAL(0) int StoreEntry::locked() const STUB_RETVAL(0) int StoreEntry::validToSend() const STUB_RETVAL(0) bool StoreEntry::memoryCachable() STUB_RETVAL(false) -MemObject *StoreEntry::makeMemObject() STUB_RETVAL(NULL) +void StoreEntry::createMemObject() STUB void StoreEntry::createMemObject(const char *, const char *, const HttpRequestMethod &aMethod) STUB +void StoreEntry::ensureMemObject(const char *, const char *, const HttpRequestMethod &) STUB void StoreEntry::dump(int debug_lvl) const STUB void StoreEntry::hashDelete() STUB void StoreEntry::hashInsert(const cache_key *) STUB diff --git a/src/tests/testStoreController.cc b/src/tests/testStoreController.cc index 116a50d409..abc478d4c1 100644 --- a/src/tests/testStoreController.cc +++ b/src/tests/testStoreController.cc @@ -31,8 +31,7 @@ testStoreController::testStats() { Store::Init(); StoreEntry *logEntry = new StoreEntry; - logEntry->makeMemObject(); - logEntry->mem_obj->setUris("dummy_storeId", NULL, HttpRequestMethod()); + logEntry->createMemObject("dummy_storeId", NULL, HttpRequestMethod()); logEntry->store_status = STORE_PENDING; TestSwapDirPointer aStore (new TestSwapDir); TestSwapDirPointer aStore2 (new TestSwapDir); @@ -73,8 +72,7 @@ testStoreController::testMaxSize() { commonInit(); StoreEntry *logEntry = new StoreEntry; - logEntry->makeMemObject(); - logEntry->mem_obj->setUris("dummy_storeId", NULL, HttpRequestMethod()); + logEntry->createMemObject("dummy_storeId", NULL, HttpRequestMethod()); logEntry->store_status = STORE_PENDING; Store::Init(); TestSwapDirPointer aStore (new TestSwapDir); diff --git a/src/tests/testStoreHashIndex.cc b/src/tests/testStoreHashIndex.cc index 96ea8bdad9..6c861d190e 100644 --- a/src/tests/testStoreHashIndex.cc +++ b/src/tests/testStoreHashIndex.cc @@ -30,8 +30,7 @@ void testStoreHashIndex::testStats() { StoreEntry *logEntry = new StoreEntry; - logEntry->makeMemObject(); - logEntry->mem_obj->setUris("dummy_storeId", NULL, HttpRequestMethod()); + logEntry->createMemObject("dummy_storeId", NULL, HttpRequestMethod()); logEntry->store_status = STORE_PENDING; Store::Init(); TestSwapDirPointer aStore (new TestSwapDir); @@ -51,8 +50,7 @@ void testStoreHashIndex::testMaxSize() { StoreEntry *logEntry = new StoreEntry; - logEntry->makeMemObject(); - logEntry->mem_obj->setUris("dummy_storeId", NULL, HttpRequestMethod()); + logEntry->createMemObject("dummy_storeId", NULL, HttpRequestMethod()); logEntry->store_status = STORE_PENDING; Store::Init(); TestSwapDirPointer aStore (new TestSwapDir);