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: SQUID_4_0_22~9 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=fc3f0aff05df8c7ed1d22b17a712e04aecba700b;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 0435cd26ed..182384e104 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -77,7 +77,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 4f958e8c74..53a750b156 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -42,7 +42,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 86a86e7a04..016adc7858 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 5bd311cc56..09206f946e 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 5ae8aed556..d4c54e2069 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); @@ -1844,22 +1845,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 1da265b034..9dc6981d28 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);