fixes for segfault from large number of files

fixes for makefile

Author: Eric Siron

README.md

@@ -84,6 +84,7 @@ Download and compile the source:
 cd ~
 git clone https://github.com/ProjectRunspace/xlatnagiosdata.git
 cd xlatnagiosdata
+git submodule update --init
 make all
 ```
 
@@ -122,11 +123,12 @@ Perform these steps to manually install xlatnagiosdata:
 2. Create a directory named ```/etc/xlatnagiosdata```
 3. From the download directory, copy ```./daemon/config/xlatnagiosdatad.toml``` to ```/etc/xlatnagiosdata```
 4. From the download directory, copy ```./daemon/config/xlatnagiosdatad.service``` to ```/etc/systemd/system``` (assuming that this is where your distribution places system service files)
-5. From the download directory, copy ```xlatnagiosdatad``` to ```/usr/local/bin```. You may use an alternative location, but you must edit the service file from #4 to match.
-6. If desired, edit the configuration file from step 3.
-7. Reload system daemons: ```sudo systemctl daemon-reload```
-8. Enable the xlatnagiosdata daemon: ```sudo systemctl enable xlatnagiosdatad``` (this sets the daemon to auto-start, you can save this step for later)
-9. Start the xlatnagiosdata daemon: ```sudo systemctl start xlatnagiosdatad```
+5. Open the file that you just copied to ```etc/systemd/system```. Find the line that reads ```User=root```. If Nagios runs under a different account, change accordingly.
+6. From the download directory, copy ```xlatnagiosdatad``` to ```/usr/local/bin```. You may use an alternative location, but you must edit the service file from #4 to match.
+7. If desired, edit the configuration file from step 3.
+8. Reload system daemons: ```sudo systemctl daemon-reload```
+9. Enable the xlatnagiosdata daemon: ```sudo systemctl enable xlatnagiosdatad``` (this sets the daemon to auto-start, you can save this step for later)
+10. Start the xlatnagiosdata daemon: ```sudo systemctl start xlatnagiosdatad```
 
 At this point, the daemon should be running. Unless you installed over a previous installation, it's probably not getting any data to gather. Skip to the **Post-Installation Setup** section.
 
@@ -173,10 +175,18 @@ After install, you need to do a few things:
 Nagios Core as installed from the Nagios web page installs to ```/usr/local/nagios```. Other repositories and repackaged solutions might use other locations. Under its root install location, Nagios creates a ```/var/spool``` sub-directory. Create an ```xlatnagiosdata``` directory. Example:
 
 ```
-sudo mkdir /usr/local/nagios/xlatnagiosdata
+sudo mkdir /usr/local/nagios/var/spool/xlatnagiosdata
 ```
 
-**Note**: If you use a different location than ```/usr/local/nagios/xlatnagiosdata```, make sure to edit ```/etc/xlatnagiosdata/xlatnagiosdatad.toml``` to change the "spool_directory" key accordingly.
+Since the nagios service commonly runs under an account other than root, and because this service runs as root, you need to make sure that both accounts can access this directory. The easiest way to do this is give this new folder the same ownership as all its peer folders (view with ```ls -la```). Make sure that ```root``` belongs to the same group as the Nagios account. Example:
+
+```
+sudo chown nagios /usr/local/nagios/var/spool/xlatnagiosdata
+sudo chgrp nagcmd /usr/local/nagios/var/spool/xlatnagiosdata
+sudo adduser root nagcmd
+```
+
+**Note**: If you use a different location than ```/usr/local/nagios/var/spool/xlatnagiosdata```, make sure to edit ```/etc/xlatnagiosdata/xlatnagiosdatad.toml``` to change the "spool_directory" key accordingly.
 
 ### 2. Enable Nagios to Write Performance Data
 
@@ -224,12 +234,12 @@ In any .cfg file of your choosing, typically in the ```objects``` directory unde
 ```
 define command {
    command_name process-host-perfdata-xlatnagiosdata
-   command_line mv /usr/local/nagios/var/host-perfdata /var/spool/xlatnagiosdata/$TIMET$.perfdata.host
+   command_line mv /usr/local/nagios/var/host-perfdata /usr/local/nagios/var/spool/xlatnagiosdata/$TIMET$.perfdata.host
 }
 
 define command {
    command_name process-service-perfdata-xlatnagiosdata
-   command_line mv /usr/local/nagios/var/service-perfdata /var/spool/xlatnagiosdata/$TIMET$.perfdata.service
+   command_line mv /usr/local/nagios/var/service-perfdata /usr/local/nagios/var/spool/xlatnagiosdata/$TIMET$.perfdata.service
 }
 ```
 

daemon/source/filedatacollector.cpp

@@ -13,27 +13,25 @@
 #include "logwriter.hpp"
 #include "utility.hpp"
 
-constexpr const size_t MaxBlockSize{1024};									  // 1024 lines per block (approximate; may go over for large files with many small lines)
-constexpr const size_t MaxFileLineLength{1024};								  // 1024 characters per line (also approximate and seems high, but it's a reasonable start)
-constexpr const size_t MaxReadChunk{MaxFileLineLength * MaxBlockSize}; // max amount of data to read into a block before sending block to caller
-static_assert(MaxReadChunk < std::numeric_limits<long>::max(), "MaxReadChunk cannot exceed the maximum size of a long");
+constexpr const size_t MaxFileSize{std::numeric_limits<long>::max()};
+static_assert(MaxFileSize <= std::numeric_limits<size_t>::max(), "MaxFileSize cannot exceed maximum size of type long");
+constexpr const size_t MaxBlockSize{1024}; // 1024 lines per block
 
 // collector logging constants
 constexpr const std::string_view AddedPerfFileForProcessing{"Added file for perfdata processing"};
 constexpr const std::string_view DeleteFile{"Delete file"};
 constexpr const std::string_view FileRead{"Read file"};
 constexpr const std::string_view FileSeek{"Move to position in file"};
+constexpr const std::string_view FileTooLarge{"File too large to process"};
 constexpr const std::string_view SpoolDirectory{"Locating spool directory"};
 constexpr const std::string_view ExtractedLine{"Extracted cleaned line"};
 constexpr const std::string_view GettingNextBlock{"Getting next data block from disk"};
-constexpr const std::string_view IndecipherableContent{"Indecipherable content in file"};
 constexpr const std::string_view NoMoreLines{"No more lines to process"};
 constexpr const std::string_view OpenFile{"Open file"};
-constexpr const std::string_view RolledBackCharacters{"Rolled back characters in a partial line"};
 constexpr const std::string_view SkippedEmpty{"Skipped empty file"};
 
 // returns true if it had to log error, false if it logged a debug message
-static bool LogIOError(ILogWriter &Log, const std::string_view &Activity, const std::string_view &Item, std::set<std::string> &ItemLog, FILE *CurrentFile)
+static bool LogIOError(ILogWriter &Log, const std::string_view &Activity, const std::string_view &Item, FILE *CurrentFile)
 {
 	int &ErrorCode = errno;
 	if (CurrentFile)
@@ -42,7 +40,6 @@ static bool LogIOError(ILogWriter &Log, const std::string_view &Activity, const
 	}
 	if (ErrorCode != 0)
 	{
-		ItemLog.insert(std::string{Item});
 		Log.WriteErrorAnnotated(Activity, Item, std::strerror(ErrorCode));
 		errno = 0;
 		if (CurrentFile)
@@ -55,89 +52,51 @@ static bool LogIOError(ILogWriter &Log, const std::string_view &Activity, const
 	return false;
 }
 
-// TODO: this might be a lot for a single function
-static void GetNextLineBlock(std::queue<std::string> &UnprocessedLines, std::queue<PendingFile> &PendingFiles, std::queue<std::string> &CompletedFiles, ILogWriter &Log, std::set<std::string> &ErrorFiles)
+static void GetNextLineBlock(std::queue<std::string> &UnprocessedLines, std::queue<PendingFile> &PendingFiles, std::queue<std::string> &CompletedFiles, ILogWriter &Log)
 {
 	if (!PendingFiles.empty())
 	{
 		errno = 0;
-		size_t CharsProcessed{0};
-		auto GetCharsToRead{[&]() -> size_t
-								  {
-									  if (CharsProcessed < MaxReadChunk)
-									  {
-										  size_t MaxChars{MaxReadChunk - CharsProcessed};
-										  return PendingFiles.front().FileSize < MaxChars ? PendingFiles.front().FileSize : MaxChars;
-									  }
-									  return 0;
-								  }};
 		std::vector<char> Buffer{};
-		size_t CharsToRead;
-		while (!PendingFiles.empty() && UnprocessedLines.size() < MaxBlockSize && (CharsToRead = GetCharsToRead()) > 0)
+		while (!PendingFiles.empty() && UnprocessedLines.size() < MaxBlockSize)
 		{
-			bool FileErrorState{false};
-			auto const &[FileName, FileSize, StartPosition]{PendingFiles.front()};
-			if (Buffer.size() < CharsToRead)
+			bool FileInErrorState{false};
+			auto const &[FileName, FileSize]{PendingFiles.front()};
+			if (Buffer.size() < FileSize)
 			{
-				Buffer.resize(CharsToRead);
+				Buffer.resize(FileSize, '\0');
 			}
 
 			auto CurrentFile{fopen(PendingFiles.front().FileName.c_str(), "r")};
-			FileErrorState = LogIOError(Log, OpenFile, PendingFiles.front().FileName, ErrorFiles, CurrentFile);
+			FileInErrorState = LogIOError(Log, OpenFile, PendingFiles.front().FileName, CurrentFile);
 			size_t CharactersRead{0};
 			if (CurrentFile)
 			{
-				fseek(CurrentFile, StartPosition, SEEK_SET);
-				FileErrorState = LogIOError(Log, FileSeek, PendingFiles.front().FileName, ErrorFiles, CurrentFile);
-				if (!std::feof(CurrentFile) && !std::ferror(CurrentFile) && CharactersRead < CharsToRead)
-				{
-					CharactersRead += std::fread(Buffer.data(), sizeof(char), CharsToRead, CurrentFile);
-					FileErrorState = LogIOError(Log, FileRead, PendingFiles.front().FileName, ErrorFiles, CurrentFile);
-				}
-			}
-			if (CurrentFile && !FileErrorState)
-			{
-				if (std::feof(CurrentFile))
+				FileInErrorState = LogIOError(Log, FileSeek, PendingFiles.front().FileName, CurrentFile);
+				if (!std::feof(CurrentFile) && !std::ferror(CurrentFile) && CharactersRead < FileSize)
 				{
-					CompletedFiles.push(std::move(PendingFiles.front().FileName));
-				}
-				else
-				{
-					PendingFiles.front().StartPosition = ftell(CurrentFile);
+					CharactersRead += std::fread(Buffer.data(), sizeof(char), FileSize, CurrentFile);
+					FileInErrorState = LogIOError(Log, FileRead, PendingFiles.front().FileName, CurrentFile);
 				}
 			}
-			if (CurrentFile == nullptr || std::feof(CurrentFile) || FileErrorState)
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wparentheses"
+			if ((CurrentFile && !FileInErrorState) && ((CharactersRead == PendingFiles.front().FileSize)) || std::feof(CurrentFile))
 			{
-				PendingFiles.pop();
+				CompletedFiles.push(std::move(PendingFiles.front().FileName));
 			}
+			PendingFiles.pop();
+#pragma GCC diagnostic pop
+
 			if (CurrentFile != nullptr)
 			{
 				std::fclose(CurrentFile);
 			}
 
 			if (CharactersRead > 0)
 			{
-				// non zero-length file always ends with \n.
-				// start at the end and walk backward until we find a newline
-				// this allows proper reset of the file position for the next read and prevents attempts to parse partial lines
-				long NonNewlineChars{0};
-				for (auto BufferIterator{Buffer.crbegin()}; BufferIterator != Buffer.crend() && *BufferIterator != '\n'; BufferIterator++)
-				{
-					NonNewlineChars++;
-				}
-				if (NonNewlineChars == static_cast<long>(CharactersRead) - 1)
-				{
-					// TODO: this means that there isn't a newline in the entire thing, which is probably ill-formed input
-					Log.WriteWarnAnnotated(IndecipherableContent, PendingFiles.front().FileName, std::string_view{Buffer.data(), CharactersRead});
-				}
-				else if (NonNewlineChars > 0)
-				{
-					// if we have non-newline characters, move the file position to the start of the last line
-					// TODO: if this is somehow less than zero, potential for lost perfdata, but almost certainly means ill-formed input. loop will handle gracefully
-					PendingFiles.front().StartPosition -= NonNewlineChars;
-					Log.WriteDebugAnnoted(RolledBackCharacters, std::to_string(NonNewlineChars));
-				}
-				std::string_view BufferView{Buffer.data(), CharactersRead - NonNewlineChars};
+				std::string_view BufferView{Buffer.data(), CharactersRead};
 				Utility::DelimitedBlockProcessor RawDataProcessor{BufferView, '\n'};
 				while (RawDataProcessor.More())
 				{
@@ -184,7 +143,14 @@ FileDataCollector::FileDataCollector(const std::string &SourcePath, ILogWriter &
 				{
 					Log.WriteDebugAnnoted(AddedPerfFileForProcessing, FilePath);
 					PendingFile NextFile{std::move(FilePath), direntry.file_size()};
-					PendingFiles.push(std::move(NextFile));
+					if (direntry.file_size() > MaxFileSize)
+					{
+						Log.WriteErrorAnnotated(FileTooLarge, NextFile.FileName, std::to_string(direntry.file_size()));
+					}
+					else
+					{
+						PendingFiles.push(std::move(NextFile));
+					}
 				}
 			}
 		}
@@ -208,7 +174,7 @@ std::string FileDataCollector::GetNextLine()
 		Log.WriteDebug(GettingNextBlock);
 		while (!PendingFiles.empty() && UnprocessedLines.size() < MaxBlockSize)
 		{
-			GetNextLineBlock(UnprocessedLines, PendingFiles, CompletedFiles, Log, ErrorFiles);
+			GetNextLineBlock(UnprocessedLines, PendingFiles, CompletedFiles, Log);
 		}
 	}
 	if (UnprocessedLines.empty())
@@ -226,11 +192,10 @@ std::string FileDataCollector::GetNextLine()
 
 FileDataCollector::~FileDataCollector()
 {
-	std::set<std::string> DestructorFileLog{}; // not doing anything with this, used for LogIOError
 	while (!CompletedFiles.empty())
 	{
 		std::remove(CompletedFiles.front().c_str());
-		LogIOError(Log, DeleteFile, CompletedFiles.front(), DestructorFileLog, nullptr);
+		LogIOError(Log, DeleteFile, CompletedFiles.front(), nullptr);
 		CompletedFiles.pop();
 	}
 }

daemon/source/filedatacollector.hpp

@@ -10,15 +10,13 @@ struct PendingFile
 	PendingFile(std::string FileName, size_t FileSize) : FileName{FileName}, FileSize{FileSize} {}
 	std::string FileName;
 	size_t FileSize;
-	long StartPosition{0};
 };
 
 class FileDataCollector
 {
 private:
 	std::string SourcePath{};
 	ILogWriter &Log;
-	std::set<std::string> ErrorFiles{};
 	std::queue<PendingFile> PendingFiles{};
 	std::queue<std::string> CompletedFiles{};
 	std::queue<std::string> UnprocessedLines{};

daemon/source/influxtranslator.cpp

@@ -124,7 +124,7 @@ std::vector<std::string> InfluxTranslator::TranslateNagiosData(const NagiosPerfo
 		LineLength += SetItem(Fields, "crit", PerfData.Crit, true);
 		LineLength += SetItem(Fields, "min", PerfData.Min, true);
 		LineLength += SetItem(Fields, "max", PerfData.Max, true);
-		LineLength += SetItem(Fields, "unit", ConvertFromNagiosUnit(PerfData.Unit, UnitTranslationMap), true);
+		LineLength += SetItem(Tags, "unit", ConvertFromNagiosUnit(PerfData.Unit, UnitTranslationMap), true);
 		TranslatedData.emplace_back(TranslateLine(LineLength));
 	}
 	return TranslatedData;

daemon/source/outputfile.cpp

@@ -39,7 +39,6 @@ int OutputFile::PrepareFile()
 
 void OutputFile::Cleanup()
 {
-	std::mutex CleanupMutex;
 	std::unique_lock TimedLock{CleanupMutex};
 	std::unique_lock WriterLock{WriterMutex, std::defer_lock};
 	while (!CloseTimer.TimedOut() && !CleanupThread.get_stop_token().stop_requested())

daemon/source/outputfile.hpp

@@ -23,6 +23,7 @@ class OutputFile
 	void Cleanup();
 
 	std::condition_variable CleanupWaitCondition;
+	std::mutex CleanupMutex;
 	std::mutex WriterMutex;
 	ThreadTimer CloseTimer;
 	std::jthread CleanupThread{};

daemon/source/utility.cpp

@@ -5,8 +5,6 @@
 #include <tuple>
 #include "utility.hpp"
 
-#include <iostream>
-
 class NumberRunner
 {
 protected:

makefile

@@ -1,9 +1,9 @@
-CXXFLAGS = -Wall -Wextra -pedantic -std=c++20 -c -fno-exceptions -fno-rtti
+CXXFLAGS = -Wall -Wextra -pedantic -std=c++20 -c -fno-rtti
 DEBUG ?= 0
 ifeq ($(DEBUG), 1)
-	CXXFLAGS +=-g3 -DDEBUG
+	CXXFLAGS +=-g3 -ggdb -DDEBUG
 else
-	CXXFLAGS +=-g0 -DNDEBUG -O3
+	CXXFLAGS +=-g0 -DNDEBUG -O3 -fno-exceptions
 endif
 
 CXX = g++ $(CXXFLAGS)
@@ -96,7 +96,7 @@ install:
 	systemctl daemon-reload
 	systemctl enable $(DAEMON_EXECUTABLE)
 	systemctl start $(DAEMON_EXECUTABLE)
-	journalctl -u $(DAEMON_EXECUTABLE)
+	journalctl -xeu $(DAEMON_EXECUTABLE) --no-pager
 	@echo
 	@echo Check journalctl output for successful start.
 	@echo Modify main nagios.cfg file:
@@ -109,12 +109,12 @@ install:
 	@echo Create rules in a .cfg file in /usr/local/nagios/etc/objects:
 	@echo "  define command {"
 	@echo "    command_name process-host-perfdata-$(PACKAGE)"
-	@echo "    command_line mv /usr/local/nagios/var/host-perfdata /var/spool/$(PACKAGE)/\$$TIMET\$$.perfdata.host"
+	@echo "    command_line mv /usr/local/nagios/var/host-perfdata /usr/local/nagios/var/spool/$(PACKAGE)/\$$TIMET\$$.perfdata.host"
 	@echo "  }"
 	@echo
 	@echo "  define command {"
 	@echo "    command_name process-service-perfdata-$(PACKAGE)"
-	@echo "    command_line mv /usr/local/nagios/var/service-perfdata /var/spool/$(PACKAGE)/\$$TIMET\$$.perfdata.service"
+	@echo "    command_line mv /usr/local/nagios/var/service-perfdata /usr/local/var/spool/$(PACKAGE)/\$$TIMET\$$.perfdata.service"
 	@echo "  }"
 	@echo
 	@echo To generate data, mark hosts and services as "process_perf_data 1"
@@ -134,6 +134,7 @@ uninstall:
 	-rm $(INSTALL_SERVICE_DIR)$(DAEMON_SERVICE_FILE)
 	-systemctl daemon-reload
 	-rm $(INSTALL_EXECUTABLE_DIR)$(DAEMON_EXECUTABLE)
+	-rm -rf /var/run/$(DAEMON_EXECUTABLE)
 
 tar:
 	@echo $(MAKEFILE_DIRECTORY)