From 35a34fdd35edfd7dc0b83a523ba865fa7d378b00 Mon Sep 17 00:00:00 2001 From: Jake Smith Date: Thu, 24 Oct 2024 12:51:11 +0100 Subject: [PATCH] HPCC-32789 Fix potential race deadlock in Thor splitter If the splitter reading arms (COutput) were reading from the same page (CRowSet chunk) as the write ahead was writing to, then the write ahead could expand that row set and cause the reader to read unexpected row data (e.g. null rows). This caused the splitter arm to premuturely finish, leaving the splitter unbalanced and stalling as the writeahead blocked soon afterwards since the finished arm was too far behind. The bug was that the row set should never expand. It is pre-sized to avoid that. However, the condition of 'fullness' was incorrect, relying only on a dynamic calculation of total row size. The fix is to also check that the number of rows does not exceed the capacity. NB: The bug predates the new splitter code, however the new splitter implementation also changed the way the splitter arms interacted with writeahead. Previously the arm would call writeahead once it hit max explicitly, rather than blocking in the underlying ISharedSmartBuffer implementation. But it would still be possible I think to hit this bug (albeit less likely). Signed-off-by: Jake Smith --- thorlcr/thorutil/thbuf.cpp | 10 +++++++--- thorlcr/thorutil/thmem.hpp | 4 +++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/thorlcr/thorutil/thbuf.cpp b/thorlcr/thorutil/thbuf.cpp index ad2d42bdb60..8edf9dfd942 100644 --- a/thorlcr/thorutil/thbuf.cpp +++ b/thorlcr/thorutil/thbuf.cpp @@ -1272,6 +1272,10 @@ class CRowSet : public CSimpleInterface, implements IInterface { return rows.get(r); } + inline bool isFull() const + { + return rows.isFull(); + } }; class Chunk : public CInterface @@ -1463,8 +1467,7 @@ class CSharedWriteAheadBase : public CSimpleInterface, implements ISharedSmartBu } } rowsRead++; - const void *retrow = rowSet->getRow(row++); - return retrow; + return rowSet->getRow(row++); } virtual void stop() { @@ -1693,7 +1696,8 @@ class CSharedWriteAheadBase : public CSimpleInterface, implements ISharedSmartBu unsigned len=rowSize(row); CriticalBlock b(crit); bool paged = false; - if (totalOutChunkSize >= minChunkSize) // chunks required to be at least minChunkSize + // NB: The isFull condition ensures that we never expand inMemRows, which would cause a race with readers reading same set + if (totalOutChunkSize >= minChunkSize || inMemRows->isFull()) // chunks required to be at least minChunkSize, or if hits max capacity { unsigned reader=anyReaderBehind(); if (NotFound != reader) diff --git a/thorlcr/thorutil/thmem.hpp b/thorlcr/thorutil/thmem.hpp index a89d4cdd0f2..f642481f49b 100644 --- a/thorlcr/thorutil/thmem.hpp +++ b/thorlcr/thorutil/thmem.hpp @@ -327,7 +327,8 @@ class graph_decl CThorExpandingRowArray : public CSimpleInterface if (!resize(numRows+1)) return false; } - rows[numRows++] = row; + rows[numRows] = row; + numRows++; return true; } bool binaryInsert(const void *row, ICompare &compare, bool dropLast=false); // NB: takes ownership on success @@ -356,6 +357,7 @@ class graph_decl CThorExpandingRowArray : public CSimpleInterface } inline rowidx_t ordinality() const { return numRows; } inline rowidx_t queryMaxRows() const { return maxRows; } + inline bool isFull() const { return numRows >= maxRows; } inline const void **getRowArray() { return rows; } void swap(CThorExpandingRowArray &src);