utils for local free and security

Author: planetchili

IntelPresentMon/CommonUtilities/CommonUtilities.vcxproj

@@ -109,6 +109,7 @@
     <ClInclude Include="win\Privileges.h" />
     <ClInclude Include="win\Process.h" />
     <ClInclude Include="win\ProcessMapBuilder.h" />
+    <ClInclude Include="win\Security.h" />
     <ClInclude Include="win\Utilities.h" />
     <ClInclude Include="win\WinAPI.h" />
   </ItemGroup>
@@ -165,9 +166,11 @@
     <ClCompile Include="win\Handle.cpp" />
     <ClCompile Include="win\HrErrorCodeProvider.cpp" />
     <ClCompile Include="win\HrError.cpp" />
+    <ClCompile Include="Memory.cpp" />
     <ClCompile Include="win\MessageBox.cpp" />
     <ClCompile Include="win\Privileges.cpp" />
     <ClCompile Include="win\ProcessMapBuilder.cpp" />
+    <ClCompile Include="win\Security.cpp" />
     <ClCompile Include="win\Utilities.cpp" />
   </ItemGroup>
   <ItemGroup>

IntelPresentMon/CommonUtilities/CommonUtilities.vcxproj.filters

@@ -291,6 +291,9 @@
     <ClInclude Include="file\FileUtils.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="win\Security.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="cli\CliFramework.cpp">
@@ -461,6 +464,12 @@
     <ClCompile Include="file\FileUtils.cpp">
       <Filter>Source Files</Filter>
     </ClCompile>
+    <ClCompile Include="win\Security.cpp">
+      <Filter>Source Files</Filter>
+    </ClCompile>
+    <ClCompile Include="Memory.cpp">
+      <Filter>Source Files</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <None Include="vcpkg.json" />

IntelPresentMon/CommonUtilities/Memory.cpp

@@ -0,0 +1,13 @@
+#include "Memory.h"
+#include "win/WinAPI.h"
+#include "log/Log.h"
+
+namespace pmon::util
+{
+	void LocalFree(void* p) noexcept
+	{
+		if (::LocalFree(p) != NULL) {
+			pmlog_warn("Failed to free memory via LocalFree");
+		}
+	}
+}

IntelPresentMon/CommonUtilities/Memory.h

@@ -100,4 +100,23 @@ namespace pmon::util
 	OutPtrProxy<SP> OutPtr(SP& smartPtr) {
 		return OutPtrProxy<SP>(smartPtr);
 	}
+
+	void LocalFree(void* p) noexcept;
+
+	// Generic LocalFree deleter
+	template<class T = void>
+	struct LocalFreeDeleter
+	{
+		using pointer = T*;
+		void operator()(pointer p) const noexcept
+		{
+			if (p) {
+				util::LocalFree(p);
+			}
+		}
+	};
+
+	// unique_ptr that uses LocalFree.
+	template<class T = void>
+	using UniqueLocalPtr = std::unique_ptr<T, LocalFreeDeleter<T>>;
 }

IntelPresentMon/CommonUtilities/file/TempFile.cpp

@@ -4,6 +4,7 @@
 #include "../win/HrError.h"
 #include "../str/String.h"
 #include "../Exception.h"
+#include "../Memory.h"
 #include <fstream>
 #include <random>
 #include <format>
@@ -143,22 +144,18 @@ namespace pmon::util::file
 		BuildExplicitAccessWithNameA(&ea[2], (LPSTR)"Authenticated Users",
 			modifyMask, SET_ACCESS, NO_INHERITANCE);
 
-		PACL newDacl = nullptr;
-		DWORD dwRes = SetEntriesInAclA(3, ea, nullptr, &newDacl);
-		if (dwRes != ERROR_SUCCESS) {
-			throw Except<win::HrError>(dwRes, "SetEntriesInAcl failed");
+		UniqueLocalPtr<ACL> pNewDacl;
+		if (auto res = SetEntriesInAclA(3, ea, nullptr, OutPtr(pNewDacl)); res != ERROR_SUCCESS) {
+			throw Except<win::HrError>(HRESULT(res), "SetEntriesInAcl failed");
 		}
 
-		dwRes = SetNamedSecurityInfoA(
+		if (auto res = SetNamedSecurityInfoA(
 			(LPSTR)path_.string().c_str(),
 			SE_FILE_OBJECT,
 			DACL_SECURITY_INFORMATION | UNPROTECTED_DACL_SECURITY_INFORMATION,
-			nullptr, nullptr, newDacl, nullptr);
-
-		LocalFree(newDacl);
-
-		if (dwRes != ERROR_SUCCESS) {
-			throw Except<win::HrError>(dwRes, "SetNamedSecurityInfo failed");
+			nullptr, nullptr, pNewDacl.get(), nullptr);
+			res != ERROR_SUCCESS) {
+			throw Except<win::HrError>(HRESULT(res), "SetNamedSecurityInfo failed");
 		}
 
 		return *this;

IntelPresentMon/CommonUtilities/pipe/Pipe.cpp

@@ -1,5 +1,5 @@
 #include "Pipe.h"
-#include <sddl.h>
+#include "../win/Security.h"
 #include <string_view>
 
 namespace pmon::util::pipe
@@ -144,15 +144,11 @@ namespace pmon::util::pipe
 			.lpSecurityDescriptor = nullptr,
 			.bInheritHandle = FALSE,
 		};
-		// if we have a security string, create the descriptor and have it owned by above structure
+		// if we have a security string, create the descriptor
+		UniqueLocalPtr<void> pSecDesc;
 		if (!security.empty()) {
-			if (!ConvertStringSecurityDescriptorToSecurityDescriptorA(
-				security.c_str(), SDDL_REVISION_1,
-				&securityAttributes.lpSecurityDescriptor, NULL)) {
-				pmlog_error(std::format(
-					"Failed creating security descriptor for pipe [{}], descriptor string was '{}'",
-					name, security)).hr().raise<PipeError>();
-			}
+			pSecDesc = win::MakeSecurityDescriptor(security);
+			securityAttributes.lpSecurityDescriptor = pSecDesc.get();
 		}
 		// if we have a security string, call create pipe with above structure, else call with nullptr
 		SECURITY_ATTRIBUTES* pSecurityAttributes = security.empty() ? nullptr : &securityAttributes;
@@ -166,12 +162,6 @@ namespace pmon::util::pipe
 			4096,					// in buffer
 			0,						// timeout
 			pSecurityAttributes));	// security
-		// regardless of result, if we are using security, free the descriptor owned by above structure
-		if (!security.empty()) {
-			if (LocalFree(securityAttributes.lpSecurityDescriptor) != NULL) {
-				pmlog_warn("Failed freeing memory for security descriptor");
-			}
-		}
 		if (!handle) {
 			pmlog_error("Server failed to create named pipe instance").hr().raise<PipeError>();
 		}

IntelPresentMon/CommonUtilities/win/Security.cpp

@@ -0,0 +1,17 @@
+#include "Security.h"
+#include <sddl.h>
+#include "HrError.h"
+
+namespace pmon::util::win
+{
+	UniqueLocalPtr<void> MakeSecurityDescriptor(const std::string& desc)
+	{
+		// using <void> and not <SECURITY_DESCRIPTOR> because PSECURITY_DESCRIPTOR is void*
+		UniqueLocalPtr<void> pDesc;
+		if (!ConvertStringSecurityDescriptorToSecurityDescriptorA(desc.c_str(), SDDL_REVISION_1,
+			OutPtr(pDesc), nullptr)) {
+			throw Except<HrError>("ConvertStringSecurityDescriptorToSecurityDescriptorA failed");
+		}
+		return pDesc;
+	}
+}

IntelPresentMon/CommonUtilities/win/Security.h

@@ -0,0 +1,9 @@
+#pragma once
+#include "WinAPI.h"
+#include "../Memory.h"
+#include <string>
+
+namespace pmon::util::win
+{
+	UniqueLocalPtr<void> MakeSecurityDescriptor(const std::string& desc);
+}

IntelPresentMon/CommonUtilities/win/Utilities.cpp

@@ -1,5 +1,6 @@
 #include "Utilities.h"
 #include "../log/Log.h"
+#include "../Memory.h"
 #include "HrError.h"
 #include <chrono>
 #include <thread>
@@ -12,31 +13,25 @@ namespace pmon::util::win
 	std::string GetErrorDescription(HRESULT hr) noexcept
 	{
 		try {
+			UniqueLocalPtr<CHAR> descriptionLocal;
 			char* descriptionWinalloc = nullptr;
-			const auto result = FormatMessageA(
+			if (!FormatMessageA(
 				FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
 				nullptr, hr, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
-				reinterpret_cast<LPSTR>(&descriptionWinalloc), 0, nullptr
-			);
-
-			std::string description;
-			if (!result) {
+				reinterpret_cast<LPSTR>(static_cast<char**>(OutPtr(descriptionLocal))), 0, nullptr))
+			{
 				pmlog_warn("Failed formatting windows error");
+				return "COULD NOT FORMAT";
 			}
-			else {
-				description = descriptionWinalloc;
-				if (LocalFree(descriptionWinalloc)) {
-					pmlog_warn("Failed freeing memory for windows error formatting");
-				}
-				if (description.ends_with("\r\n")) {
-					description.resize(description.size() - 2);
-				}
+			std::string description = descriptionLocal.get();
+			if (description.ends_with("\r\n")) {
+				description.resize(description.size() - 2);
 			}
 			return description;
 		}
 		catch (...) {
-			pmlog_warn(ReportException("Exception in win::GetErrorDescription"));
-			return {};
+			pmlog_warn(ReportException("Failed formatting windows error"));
+			return "COULD NOT FORMAT";
 		}
 	}