fix for indextool --apply-killlists crashes repeatedly with SIGSEGV #4837

src/gtests/gtests_json.cpp

@@ -15,6 +15,8 @@
 #include <cstring>
 
 #include "fileio.h"
+#include "indexfiles.h"
+#include "index_rotator.h"
 #include "json/cJSON.h"
 #include "sphinx.h"
 #include "sphinxjson.h"
@@ -63,6 +65,62 @@ TEST_F ( JsonFileParseTest, ValidJson )
 }
 
 
+TEST ( IndexFiles, ReadsVersionFromJsonHeader )
+{
+	CSphString sBase;
+	sBase.SetSprintf ( "__indexfiles_%d_json_header", GetOsProcessId() );
+	CSphString sHeader;
+	sHeader.SetSprintf ( "%s.sph", sBase.cstr() );
+
+	CSphString sError;
+	CSphWriterNonThrottled tWriter;
+	ASSERT_TRUE ( tWriter.OpenFile ( sHeader, sError ) ) << sError.cstr();
+	tWriter.PutBytes ( R"({"index_format_version":67})", strlen ( R"({"index_format_version":67})" ) );
+	tWriter.CloseFile();
+	ASSERT_FALSE ( tWriter.IsError() );
+
+	IndexFiles_c tFiles ( sBase );
+	ASSERT_TRUE ( tFiles.CheckHeader() ) << tFiles.ErrorMsg();
+	DWORD uVersion;
+	ASSERT_TRUE ( tFiles.GetVersion ( uVersion ) ) << tFiles.ErrorMsg();
+	EXPECT_EQ ( uVersion, 67U );
+
+	unlink ( sHeader.cstr() );
+}
+
+
+TEST ( IndexRotator, IgnoresMalformedJsonNewHeader )
+{
+	CSphString sBase;
+	sBase.SetSprintf ( "__indexfiles_%d_rotation", GetOsProcessId() );
+	CSphString sHeader;
+	sHeader.SetSprintf ( "%s.sph", sBase.cstr() );
+	CSphString sNewHeader;
+	sNewHeader.SetSprintf ( "%s.new.sph", sBase.cstr() );
+
+	auto fnWriteHeader = [] ( const CSphString & sFile, const char * sData )
+	{
+		CSphString sError;
+		CSphWriterNonThrottled tWriter;
+		if ( !tWriter.OpenFile ( sFile, sError ) )
+			return false;
+		tWriter.PutBytes ( sData, strlen ( sData ) );
+		tWriter.CloseFile();
+		return !tWriter.IsError();
+	};
+
+	ASSERT_TRUE ( fnWriteHeader ( sHeader, R"({"index_format_version":67})" ) );
+	ASSERT_TRUE ( fnWriteHeader ( sNewHeader, "{\n" ) );
+
+	CheckIndexRotate_c tCheck ( sBase );
+	EXPECT_FALSE ( tCheck.RotateFromNew() );
+	EXPECT_TRUE ( tCheck.RotateReenable() );
+
+	unlink ( sHeader.cstr() );
+	unlink ( sNewHeader.cstr() );
+}
+
+
 TEST_F ( JsonFileParseTest, NonJsonFormat )
 {
 	Write ( "not json" );

src/index_rotator.cpp

@@ -15,9 +15,20 @@
 #include "detail/indexlink.h"
 
 namespace {
+inline bool CheckHeader ( const CSphString & sPath, const char * sType = "" )
+{
+	IndexFiles_c tFiles ( sPath );
+	if ( !tFiles.CheckHeader ( sType ) )
+		return false;
+
+	DWORD uVersion;
+	return tFiles.GetVersion ( uVersion );
+}
+
+
 inline RotateFrom_e Check ( const CSphString& sPath ) noexcept
 {
-	switch ( ( IndexFiles_c ( sPath ).CheckHeader() ? 1 : 0 ) + ( IndexFiles_c ( sPath ).CheckHeader ( ".new" ) ? 2 : 0 ) )
+	switch ( ( CheckHeader ( sPath ) ? 1 : 0 ) + ( CheckHeader ( sPath, ".new" ) ? 2 : 0 ) )
 	{
 	case 0: return RotateFrom_e::NONE;
 	case 1: return RotateFrom_e::REENABLE;

src/indexcheck.cpp

@@ -622,6 +622,31 @@ bool DiskIndexChecker_c::Impl_c::ReadLegacyHeader ( CSphString& sError )
 
 }
 
+bool ReadIndexJsonHeaderVersion ( CSphVector<BYTE> & dData, const CSphString & sHeader, DWORD & uVersion, CSphString & sError )
+{
+	using namespace bson;
+
+	if ( sphJsonParse ( dData, sHeader, sError )!=JsonFileParse_e::OK )
+		return false;
+
+	Bson_c tBson ( dData );
+	if ( tBson.IsEmpty() || !tBson.IsAssoc() )
+	{
+		sError = "Something wrong read from json header - it is either empty, either not root object.";
+		return false;
+	}
+
+	uVersion = (DWORD)Int ( tBson.ChildByName ( "index_format_version" ) );
+	if ( uVersion<=1 || uVersion>INDEX_FORMAT_VERSION )
+	{
+		sError.SetSprintf ( "%s is v.%u, binary is v.%u", sHeader.cstr(), uVersion, INDEX_FORMAT_VERSION );
+		return false;
+	}
+
+	return true;
+}
+
+
 bool DiskIndexChecker_c::Impl_c::ReadHeader ( CSphString& sError )
 {
 	bool bHeaderIsJson;
@@ -640,33 +665,19 @@ bool DiskIndexChecker_c::Impl_c::ReadHeader ( CSphString& sError )
 
 
 	auto sHeader = GetFilename ( SPH_EXT_SPH );
-	const char* szHeader = sHeader.scstr();
 	using namespace bson;
 
 	CSphVector<BYTE> dData;
-	if ( sphJsonParse ( dData, GetFilename ( SPH_EXT_SPH ), sError )!=JsonFileParse_e::OK )
+	if ( !ReadIndexJsonHeaderVersion ( dData, sHeader, m_uVersion, sError ) )
 		return false;
 
 	Bson_c tBson ( dData );
-	if ( tBson.IsEmpty() || !tBson.IsAssoc() )
-	{
-		sError = "Something wrong read from json header - it is either empty, either not root object.";
-		return false;
-	}
-
-	// version
-	m_uVersion = (DWORD)Int ( tBson.ChildByName ( "index_format_version" ) );
-	if ( m_uVersion <= 1 || m_uVersion > INDEX_FORMAT_VERSION )
-	{
-		sError.SetSprintf ( "%s is v.%u, binary is v.%u", szHeader, m_uVersion, INDEX_FORMAT_VERSION );
-		return false;
-	}
 
 	// we don't support anything prior to v64 with json format
 	DWORD uMinFormatVer = 64;
 	if ( m_uVersion < uMinFormatVer )
 	{
-		sError.SetSprintf ( "tables prior to v.%u are no longer supported (use index_converter tool); %s is v.%u", uMinFormatVer, szHeader, m_uVersion );
+		sError.SetSprintf ( "tables prior to v.%u are no longer supported (use index_converter tool); %s is v.%u", uMinFormatVer, sHeader.cstr(), m_uVersion );
 		return false;
 	}
 

src/indexcheck.h

@@ -44,6 +44,9 @@ class DebugCheckError_i
 
 DebugCheckError_i* MakeDebugCheckError ( FILE* fp, DocID_t* pExtract );
 
+// Read and validate the format version from a JSON plain-index header.
+bool ReadIndexJsonHeaderVersion ( CSphVector<BYTE> & dData, const CSphString & sHeader, DWORD & uVersion, CSphString & sError );
+
 // disk index checker
 class DiskIndexChecker_c
 {

src/indexfiles.cpp

@@ -16,6 +16,7 @@
 #include "fileio.h"
 #include "fileutils.h"
 #include "sphinxint.h"
+#include "indexcheck.h"
 #include "tokenizer/tokenizer.h"
 
 static IndexFileExt_t g_dIndexFilesExts[SPH_EXT_TOTAL] =
@@ -103,7 +104,7 @@ bool IndexFiles_c::HasAllFiles ( const char * sType )
 {
 	for ( const auto & dExt : g_dIndexFilesExts )
 	{
-		if ( m_uVersion<dExt.m_uMinVer || dExt.m_bOptional )
+		if ( GetVersionForFiles()<dExt.m_uMinVer || dExt.m_bOptional )
 			continue;
 
 		if ( !sphIsReadable ( FullPath ( dExt.m_szExt, sType ) ) )
@@ -140,7 +141,7 @@ bool IndexFiles_c::TryRename ( const CSphString& sFrom, const CSphString& sTo )
 	for ( int i = 0; i<SPH_EXT_TOTAL; i++ )
 	{
 		const auto & dExt = g_dIndexFilesExts[i];
-		if ( m_uVersion<dExt.m_uMinVer || !dExt.m_bCopy )
+		if ( GetVersionForFiles()<dExt.m_uMinVer || !dExt.m_bCopy )
 			continue;
 
 		auto sFullFrom = FullPath ( dExt.m_szExt, "", sFrom );
@@ -265,37 +266,55 @@ bool IndexFiles_c::RenameSuffix ( const CSphString& sFrom, const CSphString& sTo
 
 bool IndexFiles_c::CheckHeader ( const char * sType )
 {
-	auto sPath = FullPath ( sphGetExt(SPH_EXT_SPH), sType );
+	m_sHeaderPath = FullPath ( sphGetExt(SPH_EXT_SPH), sType );
+	m_uVersion.reset();
 	BYTE dBuffer[8];
 
 	CSphAutoreader rdHeader ( dBuffer, sizeof ( dBuffer ) );
-	if ( !rdHeader.Open ( sPath, m_sLastError ) )
+	if ( !rdHeader.Open ( m_sHeaderPath, m_sLastError ) )
 		return false;
 
-	// check magic header
 	auto uMagic = rdHeader.GetDword();
-	if ( dBuffer[0] == '{' ) // that is new style json header, no need to check further...
+	if ( dBuffer[0] == '{' ) // new style JSON header
 		return true;
 
 	const char* sMsg = CheckFmtMagic ( uMagic );
 	if ( sMsg )
 	{
-		m_sLastError.SetSprintf ( sMsg, sPath.cstr() );
+		m_sLastError.SetSprintf ( sMsg, m_sHeaderPath.cstr() );
 		return false;
 	}
 
-	// get version
 	DWORD uVersion = rdHeader.GetDword ();
 	if ( uVersion==0 || uVersion>INDEX_FORMAT_VERSION )
 	{
-		m_sLastError.SetSprintf ( "%s is v.%u, binary is v.%u", sPath.cstr(), uVersion, INDEX_FORMAT_VERSION );
+		m_sLastError.SetSprintf ( "%s is v.%u, binary is v.%u", m_sHeaderPath.cstr(), uVersion, INDEX_FORMAT_VERSION );
 		return false;
 	}
 	m_uVersion = uVersion;
 	return true;
 }
 
 
+bool IndexFiles_c::GetVersion ( DWORD & uVersion )
+{
+	if ( !m_uVersion )
+	{
+		if ( m_sHeaderPath.IsEmpty() && !CheckHeader() )
+			return false;
+
+		CSphVector<BYTE> dData;
+		DWORD uHeaderVersion;
+		if ( !ReadIndexJsonHeaderVersion ( dData, m_sHeaderPath, uHeaderVersion, m_sLastError ) )
+			return false;
+		m_uVersion = uHeaderVersion;
+	}
+
+	uVersion = *m_uVersion;
+	return true;
+}
+
+
 bool IndexFiles_c::ReadKlistTargets ( StrVec_t & dTargets, const char * szType )
 {
 	CSphString sPath = FullPath ( sphGetExt(SPH_EXT_SPK), szType );

src/indexfiles.h

@@ -16,6 +16,7 @@
 #include "sphinxint.h"
 #include "indexfilebase.h"
 
+#include <optional>
 #include <utility>
 
 enum ESphExt : BYTE
@@ -59,16 +60,18 @@ const char*					sphGetExt ( ESphExt eExt );
 /// encapsulates all common actions over index files in general (copy/rename/delete etc.)
 class IndexFiles_c : public IndexFileBase_c
 {
-	DWORD		m_uVersion = INDEX_FORMAT_VERSION;
-	CSphString	m_sIndexName;	// used for information purposes (logs)
+	std::optional<DWORD>	m_uVersion;
+	CSphString		m_sHeaderPath;
+	CSphString		m_sIndexName;	// used for information purposes (logs)
 	CSphString	m_sLastError;
 	bool		m_bFatal = false; // if fatal fail happened (unable to rename during rollback)
 	CSphString FullPath ( const char * szExt, const CSphString& sSuffix = "", const CSphString& sBase = "" );
+	DWORD GetVersionForFiles() const { return m_uVersion.value_or ( INDEX_FORMAT_VERSION ); }
 	inline void SetName ( CSphString sIndex ) { m_sIndexName = std::move(sIndex); }
 
 public:
 	IndexFiles_c() = default;
-	explicit IndexFiles_c ( CSphString sBase, const char* sIndex=nullptr, DWORD uVersion = INDEX_FORMAT_VERSION )
+	explicit IndexFiles_c ( CSphString sBase, const char* sIndex=nullptr, std::optional<DWORD> uVersion = std::nullopt )
 		: IndexFileBase_c { std::move ( sBase ) }
 		, m_uVersion ( uVersion )
 	{
@@ -79,13 +82,14 @@ class IndexFiles_c : public IndexFileBase_c
 	inline const char * ErrorMsg () const { return m_sLastError.cstr(); }
 	inline bool IsFatal() const { return m_bFatal; }
 
-	// read .sph and adopt index version from there.
+	// check that .sph is readable and has a supported legacy header, if applicable.
 	bool CheckHeader ( const char * sType="" );
 
 	// read the beginning of .spk and parse killlist targets
 	bool ReadKlistTargets ( StrVec_t & dTargets, const char * sType="" );
 
-	DWORD GetVersion() const { return m_uVersion; }
+	// lazily read the version from a JSON header when CheckHeader() could not obtain it.
+	bool GetVersion ( DWORD & uVersion );
 
 	// simple make decorated path, like '.old' -> /path/to/index.old
 	CSphString MakePath ( const char * szSuffix = "" );

src/indextool.cpp

@@ -887,12 +887,16 @@ static void ApplyKilllists ( CSphConfig & hConf )
 			fprintf ( stdout, "WARNING: unable to index header for table %s\n", tIndex.m_sName.cstr() );
 			continue;
 		}
-		tIndex.m_uVersion = tIndexFiles.GetVersion();
+		if ( !tIndexFiles.GetVersion ( tIndex.m_uVersion ) )
+		{
+			fprintf ( stdout, "WARNING: unable to read header version for table %s: %s\n", tIndex.m_sName.cstr(), tIndexFiles.ErrorMsg() );
+			continue;
+		}
 
 		// no lookups prior to v.54
-		if ( tIndexFiles.GetVersion() < 54 )
+		if ( tIndex.m_uVersion < 54 )
 		{
-			fprintf ( stdout, "WARNING: table '%s' version: %u, min supported is 54\n", tIndex.m_sName.cstr(), tIndexFiles.GetVersion() );
+			fprintf ( stdout, "WARNING: table '%s' version: %u, min supported is 54\n", tIndex.m_sName.cstr(), tIndex.m_uVersion );
 			continue;
 		}
 

test/indextool/CMakeLists.txt

@@ -13,6 +13,13 @@ else ()
 			-P ${CMAKE_CURRENT_SOURCE_DIR}/test.cmake
 			WORKING_DIRECTORY "${MANTICORE_BINARY_DIR}/test" )
 	SET_TESTS_PROPERTIES ( Perform_indextool PROPERTIES LABELS INDEXTOOL )
+	add_test ( NAME Apply_killlists_with_legacy_json_header COMMAND ${CMAKE_COMMAND}
+			-D INDEXER=$<TARGET_FILE:indexer>
+			-D INDEXTOOL=$<TARGET_FILE:indextool>
+			-D LEGACY_FIXTURE=${CMAKE_SOURCE_DIR}/test/test_406/data
+			-P ${CMAKE_CURRENT_SOURCE_DIR}/test_apply_killlists_legacy_json.cmake
+			WORKING_DIRECTORY "${MANTICORE_BINARY_DIR}/test" )
+	SET_TESTS_PROPERTIES ( Apply_killlists_with_legacy_json_header PROPERTIES LABELS INDEXTOOL )
 endif ()