Fix #429, memory leak in grouper

manticoresoftware · Aug 31, 2018 · 116c5f1abebee9a0d99afe93546a1d8e4c6c6958 · 116c5f1
1 parent 56fdbc9
commit 116c5f1abebee9a0d99afe93546a1d8e4c6c6958
Unified Split

Showing with 63 additions and 49 deletions.

+3 −1 src/searchd.cpp

+12 −15 src/sphinx.cpp

+7 −9 src/sphinx.h

+41 −24 src/sphinxsort.cpp
diff --git a/src/searchd.cpp b/src/searchd.cpp
@@ -5252,7 +5252,9 @@ void RemapResult ( const ISphSchema * pTarget, AggrResult_t * pRes )
 				);
 		}
 		int iLimit = Min ( iCur + pRes->m_dMatchCounts[iSchema], pRes->m_dMatches.GetLength() );
-		dSchema.SubsetPtrs ( dRowItems );
+
+		// inverse dRowItems - we'll free only those NOT enumerated yet
+		dRowItems = dSchema.SubsetPtrs ( dRowItems );
 		for ( int i=iCur; i<iLimit; i++ )
 		{
 			CSphMatch & tMatch = pRes->m_dMatches[i];
diff --git a/src/sphinx.cpp b/src/sphinx.cpp
@@ -6004,26 +6004,29 @@ void CSphSchemaHelper::FreeDataPtrs ( CSphMatch * pMatch ) const

 void CSphSchemaHelper::CopyPtrs ( CSphMatch * pDst, const CSphMatch &rhs ) const
 {
-	CopyDataSpecial ( pDst, rhs, m_dDataPtrAttrs );
+	// notes on PACKEDFACTORS
+	// not immediately obvious: this is not needed while pushing matches to sorters; factors are held in an outer hash table
+	// but it is necessary to copy factors when combining results from several indexes via a sorter because at this moment matches are the owners of factor data
+	CopyPtrsSpecial ( pDst, rhs.m_pDynamic, m_dDataPtrAttrs );
 }

-void CSphSchemaHelper::SubsetPtrs ( CSphVector<int> &dDiscarded )
+CSphVector<int> CSphSchemaHelper::SubsetPtrs ( CSphVector<int> &dDiscarded ) const
 {
 	CSphVector<int> dFiltered;
 	dDiscarded.Uniq ();
 	for ( int iPtr : m_dDataPtrAttrs )
 		if ( !dDiscarded.BinarySearch ( iPtr ) )
 			dFiltered.Add ( iPtr );
 	dFiltered.Uniq ();
-	dDiscarded.SwapData ( dFiltered );
+	return dFiltered;
 }

 void CSphSchemaHelper::CloneMatchSpecial ( CSphMatch * pDst, const CSphMatch &rhs, const CSphVector<int> &dSpecials ) const
 {
 	assert ( pDst );
 	FreeDataSpecial ( pDst, dSpecials );
 	pDst->Combine ( rhs, GetDynamicSize () );
-	CopyDataSpecial ( pDst, rhs, dSpecials );
+	CopyPtrsSpecial ( pDst, rhs.m_pDynamic, dSpecials );
 }

 void CSphSchemaHelper::FreeDataSpecial ( CSphMatch * pMatch, const CSphVector<int> &dSpecials )
@@ -6034,23 +6037,17 @@ void CSphSchemaHelper::FreeDataSpecial ( CSphMatch * pMatch, const CSphVector<in

 	for ( auto iOffset : dSpecials )
 	{
-		BYTE * pData = *( BYTE ** ) ( pMatch->m_pDynamic + iOffset );
-		if ( pData )
-		{
-			delete[] pData;
-			*( BYTE ** ) ( pMatch->m_pDynamic + iOffset ) = NULL;
-		}
+		BYTE * &pData = *( BYTE ** ) ( pMatch->m_pDynamic + iOffset );
+		SafeDeleteArray ( pData );
 	}
 }

-void CSphSchemaHelper::CopyDataSpecial ( CSphMatch * pDst, const CSphMatch &rhs, const CSphVector<int> &dSpecials )
+void CSphSchemaHelper::CopyPtrsSpecial ( CSphMatch * pDst, const void* _pSrc, const CSphVector<int> &dSpecials )
 {
-	// notes on PACKEDFACTORS
-	// not immediately obvious: this is not needed while pushing matches to sorters; factors are held in an outer hash table
-	// but it is necessary to copy factors when combining results from several indexes via a sorter because at this moment matches are the owners of factor data
+	auto pSrc = (const CSphRowitem *) _pSrc;
 	for ( auto i : dSpecials )
 	{
-		const BYTE * pData = *( BYTE ** ) ( rhs.m_pDynamic + i );
+		const BYTE * pData = *( BYTE ** ) ( pSrc + i );
 		if ( pData )
 		{
 			int nBytes = sphUnpackPtrAttr ( pData, &pData );
diff --git a/src/sphinx.h b/src/sphinx.h
@@ -1552,10 +1552,6 @@ class ISphSchema
 	/// simple copy; clones either the entire dynamic part, or a part thereof
 	virtual void					CloneMatch ( CSphMatch * pDst, const CSphMatch & rhs ) const = 0;

-	// clone all raw attrs and only specified dynamics
-	virtual void					CloneMatchSpecial ( CSphMatch * pDst, const CSphMatch &rhs,
-														const CSphVector<int> &dSpecials ) const = 0;
-
 	virtual void					SwapAttrs ( CSphVector<CSphColumnInfo> & dAttrs ) = 0;
 };

@@ -1566,10 +1562,12 @@ class CSphSchemaHelper : public ISphSchema
 public:
 	void	FreeDataPtrs ( CSphMatch * pMatch ) const final;
 	void	CloneMatch ( CSphMatch * pDst, const CSphMatch & rhs ) const final;
-	void	CloneMatchSpecial ( CSphMatch * pDst, const CSphMatch &rhs, const CSphVector<int> &dSpecials ) const final;

-	// exclude vec of rowitems from dataPtrAttrs and return diff back
-	void SubsetPtrs ( CSphVector<int> &dSpecials );
+	/// clone all raw attrs and only specified ptrs
+	void	CloneMatchSpecial ( CSphMatch * pDst, const CSphMatch &rhs, const CSphVector<int> &dSpecials ) const;
+
+	/// exclude vec of rowitems from dataPtrAttrs and return diff back
+	CSphVector<int> SubsetPtrs ( CSphVector<int> &dSpecials ) const ;

 	/// get dynamic row part size
 	int GetDynamicSize () const final { return m_dDynamicUsed.GetLength (); }
@@ -1586,8 +1584,8 @@ class CSphSchemaHelper : public ISphSchema

 public:
 	// free/copy by specified vec of rowitems, assumed to be from SubsetPtrs() call.
-	static void	FreeDataSpecial ( CSphMatch * pMatch, const CSphVector<int> &dSpecials );
-	static void	CopyDataSpecial ( CSphMatch * pDst, const CSphMatch &rhs, const CSphVector<int> &dSpecials );
+	static void FreeDataSpecial ( CSphMatch * pMatch, const CSphVector<int> &dSpecials );
+	static void	CopyPtrsSpecial ( CSphMatch * pDst, const void* pSrc, const CSphVector<int> &dSpecials );
 };


diff --git a/src/sphinxsort.cpp b/src/sphinxsort.cpp
@@ -1582,12 +1582,15 @@ struct MatchCloner_t
 	CSphFixedVector<CSphRowitem>	m_dRowBuf { 0 };
 	CSphVector<CSphAttrLocator>		m_dAttrsRaw;
 	CSphVector<CSphAttrLocator>		m_dAttrsPtr;
-	const ISphSchema *				m_pSchema = nullptr;
+	CSphVector<int>					m_dMyPtrRows; // rowids matching m_dAttrsPtr
+	CSphVector<int>					m_dOtherPtrRows; // rest rowids NOT matching m_dAttrsPtr
+	const CSphSchemaHelper *		m_pSchema = nullptr;
+	bool							m_bPtrRowsCommited = false;

 public:
 	void SetSchema ( const ISphSchema * pSchema )
 	{
-		m_pSchema = pSchema;
+		m_pSchema = ( CSphSchemaHelper * ) pSchema; /// lazy hack
 		m_dRowBuf.Reset ( m_pSchema->GetDynamicSize() );
 	}

@@ -1596,45 +1599,36 @@ struct MatchCloner_t
 	{
 		assert ( m_pSchema && pDst && pSrc && pGroup );
 		assert ( pDst!=pGroup );
-		m_pSchema->CloneMatch ( pDst, *pSrc );
+		assert ( m_bPtrRowsCommited );
+
+		// clone
+		m_pSchema->CloneMatchSpecial ( pDst, *pSrc, m_dOtherPtrRows );

 		for ( auto &AttrsRaw : m_dAttrsRaw )
 			pDst->SetAttr ( AttrsRaw, pGroup->GetAttr ( AttrsRaw ) );

-		for ( auto &AttrsPtr : m_dAttrsPtr )
-		{
-			assert ( !pDst->GetAttr ( AttrsPtr ) );
-			auto sSrc = ( const char * ) pGroup->GetAttr ( AttrsPtr );
-			const char * sDst = nullptr;
-			if ( sSrc && *sSrc )
-				sDst = strdup(sSrc); // wtf?
-
-			pDst->SetAttr ( AttrsPtr, (SphAttr_t)sDst );
-		}
+		m_pSchema->FreeDataSpecial ( pDst, m_dMyPtrRows );
+		m_pSchema->CopyPtrsSpecial ( pDst, pGroup->m_pDynamic, m_dMyPtrRows );
 	}

 	void Clone ( CSphMatch * pDst, const CSphMatch * pSrc )
 	{
 		assert ( m_pSchema && pDst && pSrc );
+		assert ( m_bPtrRowsCommited );
 		if ( !pDst->m_pDynamic ) // no old match has no data to copy, just a fresh but old match
 		{
 			m_pSchema->CloneMatch ( pDst, *pSrc );
 			return;
 		}

 		memcpy ( m_dRowBuf.Begin(), pDst->m_pDynamic, m_dRowBuf.GetLengthBytes() );
-
-		// don't let cloning operation to free old string data
-		// as it will be copied back
-		for ( auto &AttrsPtr : m_dAttrsPtr )
-			pDst->SetAttr ( AttrsPtr, 0 );
-
-		m_pSchema->CloneMatch ( pDst, *pSrc );
+		m_pSchema->CloneMatchSpecial ( pDst, *pSrc, m_dOtherPtrRows );

 		for ( auto &AttrsRaw : m_dAttrsRaw )
 			pDst->SetAttr ( AttrsRaw, sphGetRowAttr ( m_dRowBuf.Begin(), AttrsRaw ) );
-		for ( auto &AttrsPtr : m_dAttrsPtr )
-			pDst->SetAttr ( AttrsPtr, sphGetRowAttr ( m_dRowBuf.Begin(), AttrsPtr) );
+
+		for ( auto &AttrsRaw : m_dAttrsPtr )
+			pDst->SetAttr ( AttrsRaw, sphGetRowAttr ( m_dRowBuf.Begin (), AttrsRaw ) );
 	}

 	inline void AddRaw ( const CSphAttrLocator& tLoc )
@@ -1646,9 +1640,31 @@ struct MatchCloner_t
 	{
 		m_dAttrsPtr.Add ( tLoc );
 	}
-	inline void ResetRaw()
+	inline void ResetAttrs()
 	{
 		m_dAttrsRaw.Resize(0);
+		m_dAttrsPtr.Resize ( 0 );
+	}
+
+	inline void CommitPtrs ()
+	{
+		assert ( m_pSchema );
+		static const int SIZE_OF_ROW = 8 * sizeof ( CSphRowitem );
+
+		if ( m_bPtrRowsCommited )
+			m_dMyPtrRows.Resize(0);
+
+		for ( const CSphAttrLocator &tLoc : m_dAttrsPtr )
+			m_dMyPtrRows.Add ( tLoc.m_iBitOffset / SIZE_OF_ROW );
+
+		m_dOtherPtrRows = m_pSchema->SubsetPtrs ( m_dMyPtrRows );
+
+#ifndef NDEBUG
+		// sanitize check
+		m_dMyPtrRows = m_pSchema->SubsetPtrs ( m_dOtherPtrRows );
+		assert ( m_dMyPtrRows.GetLength ()==m_dAttrsPtr.GetLength () );
+#endif
+		m_bPtrRowsCommited = true;
 	}
 };

@@ -1688,7 +1704,7 @@ static void FixupSorterLocators ( CSphGroupSorterSettings & tSettings, ISphSchem
 		SafeDelete(i);

 	dAggregates.Resize(0);
-	tPregroup.ResetRaw ();
+	tPregroup.ResetAttrs ();
 }

 class BaseGroupSorter_c : protected CSphGroupSorterSettings
@@ -1805,6 +1821,7 @@ class BaseGroupSorter_c : protected CSphGroupSorterSettings
 			if ( tAttr.m_eAggrFunc!=SPH_AGGR_CAT )
 				m_tPregroup.AddRaw ( tAttr.m_tLocator );
 		}
+		m_tPregroup.CommitPtrs();
 	}
 };