Merge pull request #61 from intel-innersource/coverity/fixes-2

planetchili · web-flow · commit cb64e12776bc · 2024-07-11T09:24:23.000+09:00
Coverity fixes
diff --git a/IntelPresentMon/AppCef/source/util/async/StoreFile.h b/IntelPresentMon/AppCef/source/util/async/StoreFile.h
@@ -25,13 +25,10 @@ namespace p2c::client::util::async
                 file << Traverse(pArgObj)["payload"].AsWString();
                 return Result{ true, CefValueNull() };
             }
-            else {
-                auto s = std::format("Unable to open (for writing) file path: {}", filePath.string());
-                pmlog_error(s);
-                throw std::runtime_error{ s };
-            }
-
-            return Result{ false, CefValueNull() };
+            // if opening failed, that's an error
+            auto s = std::format("Unable to open (for writing) file path: {}", filePath.string());
+            pmlog_error(s);
+            throw std::runtime_error{ s };
         }
     };
 }
diff --git a/IntelPresentMon/AppCef/source/winmain.cpp b/IntelPresentMon/AppCef/source/winmain.cpp
@@ -206,19 +206,22 @@ int APIENTRY WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLi
     // name this process / thread
     log::IdentificationTable::AddThisProcess(str::ToWide(opt.cefType.AsOptional().value_or("main-client")));
     log::IdentificationTable::AddThisThread(L"main");
-    // connect to the diagnostic layer
+    // connect to the diagnostic layer (not generally used by appcef since we connect to logging directly)
     std::optional<pmapi::DiagnosticHandler> diag;
-    if (opt.enableDiagnostic && opt.cefType && *opt.cefType == "renderer") {
-        diag.emplace(
-            (PM_DIAGNOSTIC_LEVEL)opt.logLevel.AsOptional().value_or(log::GlobalPolicy::Get().GetLogLevel()),
-            PM_DIAGNOSTIC_OUTPUT_FLAGS_DEBUGGER | PM_DIAGNOSTIC_OUTPUT_FLAGS_QUEUE,
-            [](const PM_DIAGNOSTIC_MESSAGE& msg) {
+    try {
+        if (opt.enableDiagnostic && opt.cefType && *opt.cefType == "renderer") {
+            diag.emplace(
+                (PM_DIAGNOSTIC_LEVEL)opt.logLevel.AsOptional().value_or(log::GlobalPolicy::Get().GetLogLevel()),
+                PM_DIAGNOSTIC_OUTPUT_FLAGS_DEBUGGER | PM_DIAGNOSTIC_OUTPUT_FLAGS_QUEUE,
+                [](const PM_DIAGNOSTIC_MESSAGE& msg) {
                 auto ts = msg.pTimestamp ? str::ToWide(msg.pTimestamp) : std::wstring{};
                 pmlog_(log::Level(msg.level)).note(
                     std::format(L"@@ D I A G @@ => <{}> {}", ts, str::ToWide(msg.pText)));
             }
-        );
-    }
+            );
+        }
+    } pmcatch_report;
+    
     // configure the logging system (partially based on command line options)
     ConfigureLogging();
 
diff --git a/IntelPresentMon/CommonUtilities/Exception.cpp b/IntelPresentMon/CommonUtilities/Exception.cpp
@@ -78,7 +78,8 @@ namespace pmon::util
 				std::rethrow_exception(pEx);
 			}
 			catch (const std::exception& e) {
-				return std::format("[{}] {}", typeid(e).name(), e.what());
+				try { return std::format("[{}] {}", typeid(e).name(), e.what()); }
+				catch (...) { return {}; }
 			}
 			catch (...) {
 				return "Unrecognized exception";
diff --git a/IntelPresentMon/CommonUtilities/Memory.h b/IntelPresentMon/CommonUtilities/Memory.h
@@ -33,5 +33,11 @@ namespace pmon::util
 			std::unique_ptr<T>::reset(CloningUptr{ rhs }.std::unique_ptr<T>::release());
 			return *this;
 		}
+		CloningUptr& operator=(CloningUptr&& rhs) noexcept
+		{
+			std::unique_ptr<T>::reset(rhs.release());
+			return *this;
+		}
+		~CloningUptr() = default;
 	};
 }
diff --git a/IntelPresentMon/CommonUtilities/log/BasicFileDriver.cpp b/IntelPresentMon/CommonUtilities/log/BasicFileDriver.cpp
@@ -33,7 +33,7 @@ namespace pmon::util::log
 	}
 	void BasicFileDriver::SetFileStrategy(std::shared_ptr<IFileStrategy> pFileStrategy)
 	{
-		pFileStrategy_ = pFileStrategy;
+		pFileStrategy_ = std::move(pFileStrategy);
 	}
 	void BasicFileDriver::Flush()
 	{
diff --git a/IntelPresentMon/CommonUtilities/log/Channel.h b/IntelPresentMon/CommonUtilities/log/Channel.h
@@ -21,6 +21,10 @@ namespace pmon::util::log
 			friend struct QueueAccessor_;
 		public:
 			~ChannelInternal_();
+
+			ChannelInternal_(const ChannelInternal_&) = delete;
+			ChannelInternal_& operator=(const ChannelInternal_&) = delete;
+
 			void Flush();
 			void SignalExit();
 			void DisableTraceResolution();
@@ -57,6 +61,8 @@ namespace pmon::util::log
 	{
 	public:
 		Channel(std::vector<std::pair<std::string, std::shared_ptr<IChannelComponent>>> componentPtrs = {});
+		Channel(const Channel&) = delete;
+		Channel& operator=(const Channel&) = delete;
 		~Channel();
 		void Submit(Entry&&) noexcept override;
 		void Submit(const Entry&) noexcept override;
diff --git a/IntelPresentMon/CommonUtilities/log/DiagnosticDriver.cpp b/IntelPresentMon/CommonUtilities/log/DiagnosticDriver.cpp
@@ -1,6 +1,7 @@
 #include "DiagnosticDriver.h"
 #include "../Exception.h"
 #include "../win/WinAPI.h"
+#include "PanicLogger.h"
 #include <iostream>
 #include <span>
 #include <algorithm>
@@ -19,7 +20,8 @@ namespace pmon::util::log
 	DiagnosticDriver::~DiagnosticDriver()
 	{
 		dying_ = true;
-		manualUnblockEvent_.Set();
+		try { manualUnblockEvent_.Set(); }
+		catch (...) { pmlog_panic_(L"Failed to set the manual unblock event"); }
 	}
 	void DiagnosticDriver::Submit(const Entry& e)
 	{
diff --git a/IntelPresentMon/CommonUtilities/log/EntryBuilder.cpp b/IntelPresentMon/CommonUtilities/log/EntryBuilder.cpp
@@ -55,15 +55,18 @@ namespace pmon::util::log
 	}
 	EntryBuilder& EntryBuilder::mark(const TimePoint& tp) noexcept
 	{
-		const auto now = std::chrono::high_resolution_clock::now();
-		const auto duration = std::chrono::duration<double, std::milli>(now - tp.value).count();
+		try {
+			const auto now = std::chrono::high_resolution_clock::now();
+			const auto duration = std::chrono::duration<double, std::milli>(now - tp.value).count();
 
-		if (note_.empty()) {
-			note_ += std::format(L"    Marked: {:.3f}ms", duration);
-		}
-		else {
-			note_ += std::format(L"\n    Marked: {:.3f}ms", duration);
+			if (note_.empty()) {
+				note_ += std::format(L"    Marked: {:.3f}ms", duration);
+			}
+			else {
+				note_ += std::format(L"\n    Marked: {:.3f}ms", duration);
+			}
 		}
+		catch (...) { pmlog_panic_(L"Failed to mark time in EntryBuilder"); }
 		return *this;
 	}
 
@@ -153,32 +156,37 @@ namespace pmon::util::log
 	}
 	EntryBuilder::~EntryBuilder()
 	{
-		if (pDest_) {
-			auto tracing = captureTrace_.value_or((int)level_ <= (int)GlobalPolicy::Get().GetTraceLevel());
-			// do line override check
-			if (LineTable::GetTraceOverride()) {
-				if (auto pEntry = LineTable::TryLookup(GetSourceFileName(), sourceLine_)) {
-					switch (pEntry->traceOverride_) {
-					case LineTable::TraceOverride::ForceOn: tracing = true; break;
-					case LineTable::TraceOverride::ForceOff: tracing = false; break;
+		try {
+			if (pDest_) {
+				auto tracing = captureTrace_.value_or((int)level_ <= (int)GlobalPolicy::Get().GetTraceLevel());
+				// do line override check
+				if (LineTable::GetTraceOverride()) {
+					if (auto pEntry = LineTable::TryLookup(GetSourceFileName(), sourceLine_)) {
+						switch (pEntry->traceOverride_) {
+						case LineTable::TraceOverride::ForceOn: tracing = true; break;
+						case LineTable::TraceOverride::ForceOff: tracing = false; break;
+						}
 					}
 				}
-			}
-			if (tracing) {
-				try {
-					pTrace_ = StackTrace::Here(traceSkipDepth_);
-					if (GlobalPolicy::Get().GetResolveTraceInClientThread()) {
-						pTrace_->Resolve();
+				if (tracing) {
+					try {
+						pTrace_ = StackTrace::Here(traceSkipDepth_);
+						if (GlobalPolicy::Get().GetResolveTraceInClientThread()) {
+							pTrace_->Resolve();
+						}
+					}
+					catch (...) {
+						pmlog_panic_(L"Failed to get current stacktrace");
 					}
 				}
-				catch (...) {
-					pmlog_panic_(L"Failed to get current stacktrace");
-				}
+				pDest_->Submit(std::move(*this));
+			}
+			else {
+				pmlog_panic_(L"Log entry completed with no destination channel set");
 			}
-			pDest_->Submit(std::move(*this));
 		}
-		else {
-			pmlog_panic_(L"Log entry completed with no destination channel set");
+		catch (...) {
+			pmlog_panic_(L"Error when completing log entry");
 		}
 	}
 }
diff --git a/IntelPresentMon/CommonUtilities/log/EntryBuilder.h b/IntelPresentMon/CommonUtilities/log/EntryBuilder.h
@@ -3,6 +3,7 @@
 #include <format>
 #include <memory>
 #include "TimePoint.h"
+#include "PanicLogger.h"
 
 namespace pmon::util::log
 {
@@ -13,15 +14,23 @@ namespace pmon::util::log
 	public:
 		EntryBuilder(Level lvl, const wchar_t* sourceFile, const wchar_t* sourceFunctionName, int sourceLine) noexcept;
 		EntryBuilder(Level lvl, std::wstring sourceFile, std::wstring, int sourceLine) noexcept;
+
+		EntryBuilder(const EntryBuilder&) = delete;
+		EntryBuilder & operator=(const EntryBuilder&) = delete;
+
+		~EntryBuilder();
 		template<typename T>
 		EntryBuilder& watch(const wchar_t* symbol, const T& value) noexcept
 		{
-			if (note_.empty()) {
-				note_ += std::format(L"   {} => {}", symbol, value);
-			}
-			else {
-				note_ += std::format(L"\n     {} => {}", symbol, value);
+			try {
+				if (note_.empty()) {
+					note_ += std::format(L"   {} => {}", symbol, value);
+				}
+				else {
+					note_ += std::format(L"\n     {} => {}", symbol, value);
+				}
 			}
+			catch (...) { pmlog_panic_(L"Failed to format watch in EntryBuilder"); }
 			return *this;
 		}
 		EntryBuilder& mark(const TimePoint& tp) noexcept;
@@ -45,7 +54,6 @@ namespace pmon::util::log
 			errorCode_ = { code };
 			return *this;
 		} 
-		~EntryBuilder();
 	private:
 		std::shared_ptr<IEntrySink> pDest_;
 		int traceSkipDepth_;
diff --git a/IntelPresentMon/CommonUtilities/log/ErrorCode.h b/IntelPresentMon/CommonUtilities/log/ErrorCode.h
@@ -63,6 +63,10 @@ namespace pmon::util::log
 		ErrorCode(const ErrorCode&);
 		ErrorCode(ErrorCode&&);
 		ErrorCode& operator=(ErrorCode&&);
+
+		ErrorCode & operator=(const ErrorCode&) = delete;
+		~ErrorCode() = default;
+
 		bool HasUnsigned() const;
 		bool HasSigned() const;
 		bool HasIntegral() const;
diff --git a/IntelPresentMon/CommonUtilities/log/IdentificationTable.cpp b/IntelPresentMon/CommonUtilities/log/IdentificationTable.cpp
@@ -122,7 +122,7 @@ namespace pmon::util::log
 		}
 		return {};
 	}
-	IdentificationTable::Bulk IdentificationTable::GetBulk_() const noexcept
+	IdentificationTable::Bulk IdentificationTable::GetBulk_() const
 	{
 		std::shared_lock lk{ mtx_ };
 		return Bulk{
diff --git a/IntelPresentMon/CommonUtilities/log/IdentificationTable.h b/IntelPresentMon/CommonUtilities/log/IdentificationTable.h
@@ -54,7 +54,7 @@ namespace pmon::util::log
 		void AddProcess_(uint32_t pid, const std::wstring& name);
 		std::optional<Thread> LookupThread_(uint32_t tid) const;
 		std::optional<Process> LookupProcess_(uint32_t pid) const;
-		Bulk GetBulk_() const noexcept;
+		Bulk GetBulk_() const;
 		void RegisterSink_(std::shared_ptr<IIdentificationSink> pSink);
 	private:
 		// functions
diff --git a/IntelPresentMon/CommonUtilities/log/NamedPipeMarshallReceiver.cpp b/IntelPresentMon/CommonUtilities/log/NamedPipeMarshallReceiver.cpp
@@ -31,7 +31,7 @@ namespace pmon::util::log
 
     NamedPipeMarshallReceiver::~NamedPipeMarshallReceiver()
     {
-        SignalExit();
+        pmquell(SignalExit());
     }
 
     std::optional<Entry> NamedPipeMarshallReceiver::Pop()
diff --git a/IntelPresentMon/CommonUtilities/log/NamedPipeMarshallReceiver.h b/IntelPresentMon/CommonUtilities/log/NamedPipeMarshallReceiver.h
@@ -16,6 +16,10 @@ namespace pmon::util::log
 	{
     public:
         NamedPipeMarshallReceiver(const std::wstring& pipeName, class IdentificationTable* pTable = nullptr);
+
+        NamedPipeMarshallReceiver(const NamedPipeMarshallReceiver&) = delete;
+        NamedPipeMarshallReceiver & operator=(const NamedPipeMarshallReceiver&) = delete;
+
         ~NamedPipeMarshallReceiver();
         std::optional<Entry> Pop() override;
         void SignalExit() override;
diff --git a/IntelPresentMon/CommonUtilities/log/StackTrace.h b/IntelPresentMon/CommonUtilities/log/StackTrace.h
@@ -23,6 +23,10 @@ namespace pmon::util::log
 		StackTrace() = default;
 		StackTrace(std::stacktrace trace);
 		StackTrace(const StackTrace& other);
+
+		StackTrace& operator=(const StackTrace&) = delete;
+		~StackTrace() = default;
+
 		void Resolve();
 		std::span<const FrameInfo> GetFrames() const;
 		bool Empty() const;
diff --git a/IntelPresentMon/CommonUtilities/win/Event.cpp b/IntelPresentMon/CommonUtilities/win/Event.cpp
@@ -1,6 +1,7 @@
 #include "Event.h"
 #include "WinAPI.h"
 #include <stdexcept>
+#include <cassert>
 
 namespace pmon::util::win
 {
@@ -39,17 +40,17 @@ namespace pmon::util::win
 	std::optional<uint32_t> WaitOnMultipleEvents(std::span<Event::HandleType> events, bool waitAll, uint32_t milli)
 	{
 		const auto nEvents = (DWORD)events.size();
+		assert(nEvents <= MAXIMUM_WAIT_OBJECTS);
 		const auto status = WaitForMultipleObjects(nEvents, events.data(), (BOOL)waitAll, (DWORD)milli);
-		const DWORD eventIndex = status - WAIT_OBJECT_0;
-		if (eventIndex >= 0 && eventIndex < nEvents) {
-			return eventIndex;
+		if (status >= WAIT_OBJECT_0) {
+			if (const DWORD eventIndex = status - WAIT_OBJECT_0; eventIndex < nEvents) {
+				return eventIndex;
+			}
 		}
-		else if (status == WAIT_TIMEOUT) {
+		if (status == WAIT_TIMEOUT) {
 			return {};
 		}
-		else {
-			// failed, bailed
-			throw std::runtime_error{ "Failed waiting on multiple objects" };
-		}
+		// failed, bailed
+		throw std::runtime_error{ "Failed waiting on multiple objects" };
 	}
 }
diff --git a/IntelPresentMon/CommonUtilities/win/Handle.cpp b/IntelPresentMon/CommonUtilities/win/Handle.cpp
@@ -17,16 +17,13 @@ namespace pmon::util::win
     }
     Handle::Handle(Handle&& other) noexcept
         :
-        handle_(other.handle_)
-    {
-        other.handle_ = nullptr;
-    }
-    Handle& Handle::operator=(Handle&& other) noexcept
+        handle_(std::exchange(other.handle_, nullptr))
+    {}
+    Handle& Handle::operator=(Handle&& other)
     {
         if (this != &other) {
             Clear();
-            handle_ = other.handle_;
-            other.handle_ = nullptr;
+            handle_ = std::exchange(other.handle_, nullptr);
         }
         return *this;
     }
@@ -50,9 +47,7 @@ namespace pmon::util::win
     }
     Handle::HandleType Handle::Release()
     {
-        const auto temp = handle_;
-        Clear();
-        return temp;
+        return std::exchange(handle_, nullptr);
     }
     Handle::operator bool() const noexcept
     {
diff --git a/IntelPresentMon/CommonUtilities/win/Handle.h b/IntelPresentMon/CommonUtilities/win/Handle.h
@@ -15,7 +15,7 @@ namespace pmon::util::win
         Handle(const Handle&) = delete;
         Handle& operator=(const Handle&) = delete;
         Handle(Handle&& other) noexcept;
-        Handle& operator=(Handle&& other) noexcept;
+        Handle& operator=(Handle&& other);
         operator HandleType() const;
         HandleType Get() const;
         // empties this container (closes owned handle if any)
diff --git a/IntelPresentMon/Core/source/pmon/RawFrameDataWriter.cpp b/IntelPresentMon/Core/source/pmon/RawFrameDataWriter.cpp
diff --git a/IntelPresentMon/PresentMonMiddleware/LogSetup.cpp b/IntelPresentMon/PresentMonMiddleware/LogSetup.cpp

Original file line number	Diff line number	Diff line change
`@@ -25,13 +25,10 @@ namespace p2c::client::util::async`
`25`	`25`	`file << Traverse(pArgObj)["payload"].AsWString();`
`26`	`26`	`return Result{ true, CefValueNull() };`
`27`	`27`	`}`
`28`		`- else {`
`29`		`- auto s = std::format("Unable to open (for writing) file path: {}", filePath.string());`
`30`		`- pmlog_error(s);`
`31`		`- throw std::runtime_error{ s };`
`32`		`- }`
`33`		`-`
`34`		`- return Result{ false, CefValueNull() };`
	`28`	`+ // if opening failed, that's an error`
	`29`	`+ auto s = std::format("Unable to open (for writing) file path: {}", filePath.string());`
	`30`	`+ pmlog_error(s);`
	`31`	`+ throw std::runtime_error{ s };`
`35`	`32`	`}`
`36`	`33`	`};`
`37`	`34`	`}`
Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,8 @@ namespace pmon::util`
`78`	`78`	`std::rethrow_exception(pEx);`
`79`	`79`	`}`
`80`	`80`	`catch (const std::exception& e) {`
`81`		`- return std::format("[{}] {}", typeid(e).name(), e.what());`
	`81`	`+ try { return std::format("[{}] {}", typeid(e).name(), e.what()); }`
	`82`	`+ catch (...) { return {}; }`
`82`	`83`	`}`
`83`	`84`	`catch (...) {`
`84`	`85`	`return "Unrecognized exception";`
Original file line number	Diff line number	Diff line change
`@@ -33,5 +33,11 @@ namespace pmon::util`
`33`	`33`	`std::unique_ptr<T>::reset(CloningUptr{ rhs }.std::unique_ptr<T>::release());`
`34`	`34`	`return *this;`
`35`	`35`	`}`
	`36`	`+ CloningUptr& operator=(CloningUptr&& rhs) noexcept`
	`37`	`+ {`
	`38`	`+ std::unique_ptr<T>::reset(rhs.release());`
	`39`	`+ return *this;`
	`40`	`+ }`
	`41`	`+ ~CloningUptr() = default;`
`36`	`42`	`};`
`37`	`43`	`}`
Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,7 @@ namespace pmon::util::log`
`33`	`33`	`}`
`34`	`34`	`void BasicFileDriver::SetFileStrategy(std::shared_ptr<IFileStrategy> pFileStrategy)`
`35`	`35`	`{`
`36`		`- pFileStrategy_ = pFileStrategy;`
	`36`	`+ pFileStrategy_ = std::move(pFileStrategy);`
`37`	`37`	`}`
`38`	`38`	`void BasicFileDriver::Flush()`
`39`	`39`	`{`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,7 @@`
`1`	`1`	`#include "DiagnosticDriver.h"`
`2`	`2`	`#include "../Exception.h"`
`3`	`3`	`#include "../win/WinAPI.h"`
	`4`	`+#include "PanicLogger.h"`
`4`	`5`	`#include <iostream>`
`5`	`6`	`#include <span>`
`6`	`7`	`#include <algorithm>`
`@@ -19,7 +20,8 @@ namespace pmon::util::log`
`19`	`20`	`DiagnosticDriver::~DiagnosticDriver()`
`20`	`21`	`{`
`21`	`22`	`dying_ = true;`
`22`		`- manualUnblockEvent_.Set();`
	`23`	`+ try { manualUnblockEvent_.Set(); }`
	`24`	`+ catch (...) { pmlog_panic_(L"Failed to set the manual unblock event"); }`
`23`	`25`	`}`
`24`	`26`	`void DiagnosticDriver::Submit(const Entry& e)`
`25`	`27`	`{`