From 836d3c0b158f6e7bc795d1e6d881c873d98728e8 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 19 Feb 2023 00:23:31 +0000 Subject: [PATCH] Ensure StoreEntry presence during store_client lifetime (#1281) Most likely, all store_client users already lock their StoreEntry objects, so this change is unlikely to affect any current use cases. However, objects like store_client that store StoreEntry objects for long-term/asynchronous reuse should lock their entries rather than rely on others to do that for them. Being able to rely on StoreEntry locking also clarifies existing logic in several store_client methods. --- src/StoreClient.h | 2 +- src/store_client.cc | 19 ++++++++++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/StoreClient.h b/src/StoreClient.h index 37f7d04a93..47106617d8 100644 --- a/src/StoreClient.h +++ b/src/StoreClient.h @@ -58,7 +58,7 @@ class store_client CBDATA_CLASS(store_client); public: - store_client(StoreEntry *); + explicit store_client(StoreEntry *); ~store_client(); /// An offset into the stored response bytes, including the HTTP response diff --git a/src/store_client.cc b/src/store_client.cc index 1262685fd4..c6486dae1e 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -216,6 +216,9 @@ store_client::store_client(StoreEntry *e) : object_ok(true), copiedSize(0) { + Assure(entry); + entry->lock("store_client"); + flags.disk_io_pending = false; flags.store_copying = false; ++ entry->refcount; @@ -228,7 +231,10 @@ store_client::store_client(StoreEntry *e) : } store_client::~store_client() -{} +{ + assert(entry); + entry->unlock("store_client"); +} /* copy bytes requested by the client */ void @@ -527,7 +533,7 @@ store_client::readBody(const char *, ssize_t len) if (len > 0 && rep && entry->mem_obj->inmem_lo == 0 && entry->objectLen() <= (int64_t)Config.Store.maxInMemObjSize && Config.onoff.memory_cache_disk) { storeGetMemSpace(len); - // The above may start to free our object so we need to check again + // recheck for the above call may purge entry's data from the memory cache if (entry->mem_obj->inmem_lo == 0) { /* Copy read data back into memory. * copyInto.offset includes headers, which is what mem cache needs @@ -717,15 +723,14 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data) #endif + // We must lock to safely dereference e below, after deleting sc and after + // calling CheckQuickAbortIsReasonable(). + e->lock("storeUnregister"); + // XXX: We might be inside sc store_client method somewhere up the call // stack. TODO: Convert store_client to AsyncJob to make destruction async. delete sc; - assert(e->locked()); - // An entry locked by others may be unlocked (and destructed) by others, so - // we must lock again to safely dereference e after CheckQuickAbortIsReasonable(). - e->lock("storeUnregister"); - if (CheckQuickAbortIsReasonable(e)) e->abort(); else -- 2.47.2