From 728a3d69b55c96f099dca4cccd8a0d7b1eacdaf7 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 7 Mar 2022 13:41:08 -0600 Subject: [PATCH 1/3] Check bounds before accessing Vol::evacuate array --- iocore/cache/CacheWrite.cc | 9 ++++++++- iocore/cache/P_CacheVol.h | 17 +++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/iocore/cache/CacheWrite.cc b/iocore/cache/CacheWrite.cc index 605588d790f..4cdaeb98e98 100644 --- a/iocore/cache/CacheWrite.cc +++ b/iocore/cache/CacheWrite.cc @@ -288,6 +288,13 @@ iobufferblock_memcpy(char *p, int len, IOBufferBlock *ab, int offset) EvacuationBlock * Vol::force_evacuate_head(Dir *evac_dir, int pinned) { + auto bucket = dir_evac_bucket(evac_dir); + if (!evac_bucket_valid(bucket)) { + DDebug("cache_evac", "dir_evac_bucket out of bounds, skipping evacuate: %lld(%d), %d, %d", bucket, evacuate_size, + (int)dir_offset(evac_dir), (int)dir_phase(evac_dir)); + return nullptr; + } + // build an evacuation block for the object EvacuationBlock *b = evacuation_block_exists(evac_dir, this); // if we have already started evacuating this document, its too late @@ -300,7 +307,7 @@ Vol::force_evacuate_head(Dir *evac_dir, int pinned) b = new_EvacuationBlock(mutex->thread_holding); b->dir = *evac_dir; DDebug("cache_evac", "force: %d, %d", (int)dir_offset(evac_dir), (int)dir_phase(evac_dir)); - evacuate[dir_evac_bucket(evac_dir)].push(b); + evacuate[bucket].push(b); } b->f.pinned = pinned; b->f.evacuate_head = 1; diff --git a/iocore/cache/P_CacheVol.h b/iocore/cache/P_CacheVol.h index 95449289a53..b2f69b13ce2 100644 --- a/iocore/cache/P_CacheVol.h +++ b/iocore/cache/P_CacheVol.h @@ -206,6 +206,12 @@ struct Vol : public Continuation { int dir_check(bool fix); int db_check(bool fix); + bool + evac_bucket_valid(off_t bucket) + { + return (bucket >= 0 && bucket < evacuate_size); + } + int is_io_in_progress() { @@ -456,10 +462,13 @@ int vol_init(Vol *d, char *s, off_t blocks, off_t skip, bool clear); TS_INLINE EvacuationBlock * evacuation_block_exists(Dir *dir, Vol *p) { - EvacuationBlock *b = p->evacuate[dir_evac_bucket(dir)].head; - for (; b; b = b->link.next) - if (dir_offset(&b->dir) == dir_offset(dir)) - return b; + auto bucket = dir_evac_bucket(dir); + if (p->evac_bucket_valid(bucket)) { + EvacuationBlock *b = p->evacuate[bucket].head; + for (; b; b = b->link.next) + if (dir_offset(&b->dir) == dir_offset(dir)) + return b; + } return nullptr; } From 838e6266c3ff30ee6de255fabb3bd5251c41fd72 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 7 Mar 2022 14:32:50 -0600 Subject: [PATCH 2/3] fix format specifier for debug --- iocore/cache/CacheWrite.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iocore/cache/CacheWrite.cc b/iocore/cache/CacheWrite.cc index 4cdaeb98e98..eb2200122c4 100644 --- a/iocore/cache/CacheWrite.cc +++ b/iocore/cache/CacheWrite.cc @@ -290,7 +290,7 @@ Vol::force_evacuate_head(Dir *evac_dir, int pinned) { auto bucket = dir_evac_bucket(evac_dir); if (!evac_bucket_valid(bucket)) { - DDebug("cache_evac", "dir_evac_bucket out of bounds, skipping evacuate: %lld(%d), %d, %d", bucket, evacuate_size, + DDebug("cache_evac", "dir_evac_bucket out of bounds, skipping evacuate: %ld(%d), %d, %d", bucket, evacuate_size, (int)dir_offset(evac_dir), (int)dir_phase(evac_dir)); return nullptr; } From 59a7fbd80b5f3967c525b05149b4c1df3916404a Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Mon, 7 Mar 2022 17:55:57 -0600 Subject: [PATCH 3/3] Bounds check a few more places --- iocore/cache/CacheWrite.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/iocore/cache/CacheWrite.cc b/iocore/cache/CacheWrite.cc index eb2200122c4..20baeb393f8 100644 --- a/iocore/cache/CacheWrite.cc +++ b/iocore/cache/CacheWrite.cc @@ -507,7 +507,7 @@ CacheVC::evacuateDocDone(int /* event ATS_UNUSED */, Event * /* e ATS_UNUSED */) (int)dir_phase(&overwrite_dir), (int)dir_offset(&dir), (int)dir_phase(&dir)); int i = dir_evac_bucket(&overwrite_dir); // nasty beeping race condition, need to have the EvacuationBlock here - EvacuationBlock *b = vol->evacuate[i].head; + EvacuationBlock *b = vol->evac_bucket_valid(i) ? vol->evacuate[i].head : nullptr; for (; b; b = b->link.next) { if (dir_offset(&b->dir) == dir_offset(&overwrite_dir)) { // If the document is single fragment (although not tied to the vector), @@ -658,6 +658,7 @@ Vol::evacuateDocReadDone(int event, Event *e) Doc *doc = reinterpret_cast(doc_evacuator->buf->data()); CacheKey next_key; EvacuationBlock *b = nullptr; + auto bucket = dir_evac_bucket(&doc_evacuator->overwrite_dir); if (doc->magic != DOC_MAGIC) { Debug("cache_evac", "DOC magic: %X %d", (int)dir_tag(&doc_evacuator->overwrite_dir), (int)dir_offset(&doc_evacuator->overwrite_dir)); @@ -667,7 +668,9 @@ Vol::evacuateDocReadDone(int event, Event *e) DDebug("cache_evac", "evacuateDocReadDone %X offset %d", (int)doc->key.slice32(0), (int)dir_offset(&doc_evacuator->overwrite_dir)); - b = evacuate[dir_evac_bucket(&doc_evacuator->overwrite_dir)].head; + if (evac_bucket_valid(bucket)) { + b = evacuate[bucket].head; + } while (b) { if (dir_offset(&b->dir) == dir_offset(&doc_evacuator->overwrite_dir)) { break; @@ -935,7 +938,7 @@ agg_copy(char *p, CacheVC *vc) inline void Vol::evacuate_cleanup_blocks(int i) { - EvacuationBlock *b = evacuate[i].head; + EvacuationBlock *b = evac_bucket_valid(i) ? evacuate[i].head : nullptr; while (b) { if (b->f.done && ((header->phase != dir_phase(&b->dir) && header->write_pos > this->vol_offset(&b->dir)) || (header->phase == dir_phase(&b->dir) && header->write_pos <= this->vol_offset(&b->dir)))) {