Fix segfault when passing invalid include path in fbtrace config via fbtracemgr (#9091)

* Fix segfault when passing invalid `include` path in fbtrace config via fbtracemgr

* Move config tests to existing file

* Allow to skip test if test config cannot be created

* Resolve path to `tmp` in config unit tests

`include` does not work with symlinks

---------

Co-authored-by: Artyom Abakumov <artyom.abakumov@red-soft.ru>

Author: Noremos

src/common/config/config_file.cpp

@@ -117,6 +117,8 @@ class MainStream : public ConfigFile::Stream
 class TextStream : public ConfigFile::Stream
 {
 public:
+	inline static constexpr const char* STREAM_NAME = "Passed text";
+
 	explicit TextStream(const char* configText)
 		: s(configText), l(0)
 	{
@@ -275,7 +277,7 @@ ConfigFile::Stream::~Stream()
  *	Parse line, taking quotes into account
  */
 
-ConfigFile::LineType ConfigFile::parseLine(const char* fileName, const String& inputPar, Parameter& par)
+ConfigFile::LineType ConfigFile::parseLine(const StreamName fileName, const String& inputPar, Parameter& par)
 {
 	int inString = 0;
 	String::size_type valStart = 0;
@@ -449,7 +451,7 @@ void ConfigFile::adjustMacroReplacePositions(const String& value, const String&
 		to += getDirSeparatorLength(value, to);
 }
 
-bool ConfigFile::macroParse(String& value, const char* fileName) const
+bool ConfigFile::macroParse(String& value, const StreamName fileName) const
 {
 	String::size_type pos = 0;
 	String::size_type subFrom;
@@ -506,7 +508,7 @@ bool ConfigFile::macroParse(String& value, const char* fileName) const
  *	Find macro value
  */
 
-bool ConfigFile::translate(const char* fileName, const String& from, String& to) const
+bool ConfigFile::translate(const StreamName fileName, const String& from, String& to) const
 {
 	if (from == "root")
 	{
@@ -518,19 +520,20 @@ bool ConfigFile::translate(const char* fileName, const String& from, String& to)
 	}
 	else if (from == "this")
 	{
-		if (!fileName)
+		if (!fileName.has_value())
 		{
 			return false;
 		}
 
-		PathName tempPath(fileName);
+		const char* fileNameData = fileName.value_or("");
+		PathName tempPath(fileNameData);
 
 #ifdef UNIX
 		if (PathUtils::isSymLink(tempPath))
 		{
 			// If $(this) is a symlink, expand it.
 			TEXT temp[MAXPATHLEN];
-			const int n = readlink(fileName, temp, sizeof(temp));
+			const int n = readlink(fileNameData, temp, sizeof(temp));
 
 			if (n != -1)
 			{
@@ -539,7 +542,7 @@ bool ConfigFile::translate(const char* fileName, const String& from, String& to)
 				if (PathUtils::isRelative(tempPath))
 				{
 					PathName parent;
-					PathUtils::splitLastComponent(parent, tempPath, fileName);
+					PathUtils::splitLastComponent(parent, tempPath, fileNameData);
 					PathUtils::concatPath(tempPath, parent, temp);
 				}
 			}
@@ -640,9 +643,9 @@ const ConfigFile::Parameter* ConfigFile::findParameter(const KeyType& name, cons
  *	Take into an account fault line
  */
 
-void ConfigFile::badLine(const char* fileName, const String& line)
+void ConfigFile::badLine(const StreamName fileName, const String& line)
 {
-	(Arg::Gds(isc_conf_line) << (fileName ? fileName : "Passed text") << line).raise();
+	(Arg::Gds(isc_conf_line) << fileName.value_or(TextStream::STREAM_NAME) << line).raise();
 }
 
 /******************************************************************************
@@ -655,7 +658,7 @@ void ConfigFile::parse(Stream* stream)
 	String inputLine;
 	Parameter* previous = NULL;
 	unsigned int line;
-	const char* streamName = stream->getFileName();
+	const StreamName streamName = stream->getFileName();
 
 	parameters.setSortMode(FB_ARRAY_SORT_MANUAL);
 
@@ -756,24 +759,26 @@ void ConfigFile::parse(Stream* stream)
  *	Parse include operator
  */
 
-void ConfigFile::include(const char* currentFileName, const PathName& parPath)
+void ConfigFile::include(const StreamName currentFileName, const PathName& parPath)
 {
+	const auto fileNameForError = currentFileName.value_or(TextStream::STREAM_NAME);
+
 #ifdef DEBUG_INCLUDES
 	fprintf(stderr, "include into %s file(s) %s\n", currentFileName, parPath.c_str());
 #endif
 	// We should better limit include depth
 	AutoSetRestore<unsigned> depth(&includeLimit, includeLimit + 1);
 	if (includeLimit > INCLUDE_LIMIT)
 	{
-		(Arg::Gds(isc_conf_include) << currentFileName << parPath << Arg::Gds(isc_include_depth)).raise();
+		(Arg::Gds(isc_conf_include) << fileNameForError << parPath << Arg::Gds(isc_include_depth)).raise();
 	}
 
 	// for relative paths first of all prepend with current path (i.e. path of current conf file)
 	PathName path;
 	if (PathUtils::isRelative(parPath))
 	{
 		PathName dummy;
-		PathUtils::splitLastComponent(path, dummy, currentFileName);
+		PathUtils::splitLastComponent(path, dummy, currentFileName.value_or(""));
 	}
 	PathUtils::concatPath(path, path, parPath);
 
@@ -796,12 +801,12 @@ void ConfigFile::include(const char* currentFileName, const PathName& parPath)
 	}
 
 	// analyze components for wildcards
-	if (!wildCards(currentFileName, pathPrefix, components))
+	if (!wildCards(pathPrefix, components))
 	{
 		// no matches found - check for presence of wild symbols in path
 		if (!hadWildCards)
 		{
-			(Arg::Gds(isc_conf_include) << currentFileName << parPath << Arg::Gds(isc_include_miss)).raise();
+			(Arg::Gds(isc_conf_include) << fileNameForError << parPath << Arg::Gds(isc_include_miss)).raise();
 		}
 	}
 }
@@ -814,7 +819,7 @@ void ConfigFile::include(const char* currentFileName, const PathName& parPath)
  *		- returns true if some match was found
  */
 
-bool ConfigFile::wildCards(const char* currentFileName, const PathName& pathPrefix, FilesArray& components)
+bool ConfigFile::wildCards(const PathName& pathPrefix, FilesArray& components)
 {
 	// Any change in directory can cause config change
 	PathName prefix(pathPrefix);
@@ -855,7 +860,7 @@ bool ConfigFile::wildCards(const char* currentFileName, const PathName& pathPref
 
 		if (mustBeDir)	// should be directory
 		{
-			found = wildCards(currentFileName, name, components) || found;
+			found = wildCards(name, components) || found;
 		}
 		else
 		{
@@ -962,4 +967,3 @@ bool ConfigFile::Parameter::asBoolean() const
 		value.equalsNoCase("yes") ||
 		value.equalsNoCase("y");
 }
-

src/common/config/config_file.h

@@ -28,6 +28,8 @@
 #include "../common/classes/objects_array.h"
 #include "../common/classes/fb_string.h"
 #include "../common/classes/auto.h"
+#include "../common/utils_proto.h"
+
 
 /**
 	Since the original (isc.cpp) code wasn't able to provide powerful and
@@ -50,6 +52,8 @@ class ConfigCache;
 
 class ConfigFile : public Firebird::AutoStorage, public Firebird::RefCounted
 {
+	using StreamName = fb_utils::SafePointer<const char>;
+
 public:
 	// flags for config file
 	static inline constexpr USHORT HAS_SUB_CONF		= 0x01;
@@ -126,7 +130,7 @@ class ConfigFile : public Firebird::AutoStorage, public Firebird::RefCounted
 	}
 
 	// Substitute macro values in a string
-	bool macroParse(String& value, const char* fileName) const;
+	bool macroParse(String& value, const StreamName fileName) const;
 
 private:
 	enum LineType {LINE_BAD, LINE_REGULAR, LINE_START_SUB, LINE_END_SUB, LINE_INCLUDE};
@@ -140,11 +144,11 @@ class ConfigFile : public Firebird::AutoStorage, public Firebird::RefCounted
 	// utilities
 	bool getLine(Stream* stream, String&, unsigned int&);
 	void parse(Stream* stream);
-	LineType parseLine(const char* fileName, const String& input, Parameter& par);
-	bool translate(const char* fileName, const String& from, String& to) const;
-	[[noreturn]] void badLine(const char* fileName, const String& line);
-	void include(const char* currentFileName, const Firebird::PathName& path);
-	bool wildCards(const char* currentFileName, const Firebird::PathName& pathPrefix, FilesArray& components);
+	LineType parseLine(const StreamName fileName, const String& input, Parameter& par);
+	bool translate(const StreamName fileName, const String& from, String& to) const;
+	[[noreturn]] void badLine(const StreamName fileName, const String& line);
+	void include(const StreamName currentFileName, const Firebird::PathName& path);
+	bool wildCards(const Firebird::PathName& pathPrefix, FilesArray& components);
 	bool substituteStandardDir(const String& from, String& to) const;
 	void adjustMacroReplacePositions(const String& value, const String& macro, String::size_type& from, String::size_type& to) const;
 	unsigned getDirSeparatorLength(const String& value, String::size_type subFrom) const;

src/common/tests/CommonFixtures.h

@@ -2,37 +2,26 @@
 #define COMMON_FIXTURES
 #include "boost/test/unit_test.hpp"
 
+#include "CommonUtils.h"
+
 #include <filesystem>
-#include <random>
 
 namespace TestsUtils
 {
 
 namespace fs = std::filesystem;
 
-inline std::string generateRandomString(std::size_t length)
-{
-	std::random_device rd;
-	std::mt19937 generator(rd());
-
-	std::uniform_int_distribution<> distribution(0, 9);
-
-	std::string randomString;
-	for (std::size_t i = 0; i < length; ++i)
-	{
-		randomString += '0' + distribution(generator);
-	}
-
-	return randomString;
-}
-
 struct TempPathFixture
 {
 	fs::path tempPathFX;
 
 	TempPathFixture()
 	{
-		tempPathFX = fs::temp_directory_path() / (generateRandomString(10) + "_common_test.tmp");
+		auto tempDir = fs::temp_directory_path();
+		// Resolve symlink (/var on macos)
+		tempDir = fs::canonical(tempDir);
+
+		tempPathFX = tempDir / (generateRandomString(10) + "_common_test.tmp");
 	}
 
 	~TempPathFixture()

src/common/tests/CommonUtils.h

@@ -0,0 +1,83 @@
+#ifndef TEST_COMMON_UTILS
+#define TEST_COMMON_UTILS
+
+#include "firebird.h"
+#include "fb_exception.h"
+
+#include "boost/test/unit_test.hpp"
+
+#include <string>
+#include <string_view>
+#include <random>
+
+namespace TestsUtils
+{
+	inline std::string generateRandomString(std::size_t length)
+	{
+		std::random_device rd;
+		std::mt19937 generator(rd());
+
+		std::uniform_int_distribution<> distribution(0, 9);
+
+		std::string randomString;
+		for (std::size_t i = 0; i < length; ++i)
+		{
+			randomString += '0' + distribution(generator);
+		}
+
+		return randomString;
+	}
+
+	// Use std::string because it works better with BOOST_TEST
+	inline std::string getErrorMessage(const Firebird::status_exception& ex)
+	{
+		const ISC_STATUS* status = ex.value();
+
+		std::string buffer;
+		TEXT temp[BUFFER_LARGE];
+		while (fb_interpret(temp, sizeof(temp), &status))
+		{
+			buffer += temp;
+			buffer += " ";
+		}
+
+		if (!buffer.empty())
+			buffer.resize(buffer.length() - 1);
+
+		return buffer;
+	}
+
+	// Wrapper to pass const char array as template argument
+	template<std::size_t N>
+	struct ConstexprString
+	{
+		char value[N];
+
+		constexpr ConstexprString(const char (&str)[N])
+		{
+			std::copy_n(str, N, value);
+		}
+
+		constexpr operator std::string_view() const
+		{
+			return std::string_view(value, N - 1); // Exclude '\0'
+		}
+	};
+
+	inline bool checkErrorMessage(const Firebird::status_exception& ex, const std::string_view expected)
+	{
+		const auto message = getErrorMessage(ex);
+		BOOST_TEST_INFO(std::string("Expected exception: ") + expected.data());
+		BOOST_TEST_INFO("Caught exception:   " + message); // Space for alignment
+		return message == expected;
+	}
+
+	template<ConstexprString Expecter>
+	inline bool checkErrorMessage(const Firebird::status_exception& ex)
+	{
+		return checkErrorMessage(ex, static_cast<std::string_view>(Expecter));
+	}
+
+}
+
+#endif

src/common/tests/ConfigFileTest.cpp

@@ -27,6 +27,53 @@ BOOST_FIXTURE_TEST_CASE(IncludeInUserSessionBug, TestsUtils::TempPathFixture)
 	BOOST_CHECK_THROW(ConfigFile file({}, text.data(), ConfigFile::DENY_INCLUDE), Firebird::Exception);
 }
 
+BOOST_AUTO_TEST_CASE(InvalidIncludeInTextStreamBug)
+{
+	MemoryPool& pool = *getDefaultMemoryPool();
+
+	const std::string_view text = R"(
+database
+{
+enabled = true
+}
+include /a/b/c/d/f.d
+	)";
+
+
+	// Should be an exception, not a segfault
+	BOOST_CHECK_EXCEPTION(ConfigFile file({}, text.data(), 0), Firebird::status_exception,
+	TestsUtils::checkErrorMessage<"Invalid include operator in Passed text for </a/b/c/d/f.d> File to include not found">);
+}
+
+BOOST_FIXTURE_TEST_CASE(RecursiveInclude, TestsUtils::TempPathFixture)
+{
+	MemoryPool& pool = *getDefaultMemoryPool();
+	const auto pathStr = tempPathFX.string();
+
+	std::string text = R"(
+database
+{
+enabled = true
+}
+include )";
+
+	text += pathStr;
+
+	std::ofstream out(pathStr);
+	out << "include " + pathStr;
+	out.close();
+
+	Firebird::string error;
+	error.printf("Invalid include operator in %s for <%s> Include depth too big", pathStr.data(), pathStr.data());
+
+	// Should be an exception, not a segfault
+	BOOST_CHECK_EXCEPTION(ConfigFile file({}, text.data(), 0), Firebird::status_exception,
+	[&error](const Firebird::status_exception& ex)
+	{
+		return TestsUtils::checkErrorMessage(ex, error.data());
+	});
+}
+
 
 BOOST_AUTO_TEST_SUITE_END() // AutoPtrFunctionalTests
 BOOST_AUTO_TEST_SUITE_END()	// CommonClassesSuite