|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 5/5] Replace RegisterEventSource with TraceLoggingRegister
On 06/02/2026 09:45, Owen Smith wrote:
> - Added TraceLogging levels for Information and Error events.
> - Replaced RegisterEventSource due to potential security issues.
>
Could you add some comments to explain the potential issue with
RegisterEventSource?
> Signed-off-by: david ambu <david.preetham@xxxxxxxxx>
>
> * defined seperate macros for Info and Error logging
> * use a switch on log level, rather than if/else
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
I think it'd be great if you could provide similar patches for
xencons_monitor and xenagent as well.
> ---
> src/monitor/monitor.c | 290 ++++++++++++++++++++++--------------------
> 1 file changed, 150 insertions(+), 140 deletions(-)
>
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index 77d7dd8..a9b69f3 100644
> --- a/src/monitor/monitor.c
> +++ b/src/monitor/monitor.c
> @@ -40,6 +40,8 @@
> #include <powrprof.h>
> #include <malloc.h>
> #include <assert.h>
> +#include <TraceLoggingProvider.h>
> +#include <winmeta.h>
>
> #include <version.h>
>
> @@ -56,7 +58,6 @@ typedef struct _MONITOR_CONTEXT {
> SERVICE_STATUS Status;
> SERVICE_STATUS_HANDLE Service;
> HKEY ParametersKey;
> - HANDLE EventLog;
> HANDLE StopEvent;
> HANDLE RequestEvent;
> HANDLE Timer;
> @@ -96,17 +97,25 @@ MONITOR_CONTEXT MonitorContext;
> #define STORAGE_CLASS_GUID \
> "{4d36e97b-e325-11ce-bfc1-08002be10318}"
>
> +TRACELOGGING_DEFINE_PROVIDER(g_xenbus_monitor,
The provider handle name should use the same convention as other global
variables (e.g. MonitorTraceLoggingProvider).
> + MONITOR_NAME,
> + //{E8ADEEB7-6DD1-447E-A49B-A6571CC74039}
> + (0xe8adeeb7, 0x6dd1, 0x447e, 0xa4, 0x9b, 0xa6,
> 0x57, 0x1c, 0xc7, 0x40, 0x39)
Is it worth having a DEFINE_GUID for this somewhere?
> + );
> +
> +typedef enum {
> + LOG_INFO,
> + LOG_ERROR
> +} LOG_LEVEL;
> +
> static VOID
> #pragma prefast(suppress:6262) // Function uses '1036' bytes of stack:
> exceeds /analyze:stacksize'1024'
> __Log(
> + _In_ LOG_LEVEL Level,
> _In_ PCSTR Format,
> ...
> )
> {
> -#if DBG
> - PMONITOR_CONTEXT Context = &MonitorContext;
> - const TCHAR *Strings[1];
> -#endif
> TCHAR Buffer[MAXIMUM_BUFFER_SIZE];
> va_list Arguments;
> size_t Length;
> @@ -133,24 +142,31 @@ __Log(
>
> OutputDebugString(Buffer);
>
> -#if DBG
> - Strings[0] = Buffer;
> -
> - if (Context->EventLog != NULL)
> - ReportEvent(Context->EventLog,
> - EVENTLOG_INFORMATION_TYPE,
> - 0,
> - MONITOR_LOG,
> - NULL,
> - ARRAYSIZE(Strings),
> - 0,
> - Strings,
> - NULL);
> -#endif
> + switch (Level) {
> + case LOG_INFO:
> + TraceLoggingWrite(g_xenbus_monitor,
> + "Information",
> + TraceLoggingLevel(WINEVENT_LEVEL_INFO),
> + TraceLoggingString(Buffer, "Info")
Buffer is a PTSTR, so we should have an appropriate #define for
TraceLoggingString.
> + );
> + break;
> + case LOG_ERROR:
> + TraceLoggingWrite(g_xenbus_monitor,
> + "Error",
> + TraceLoggingLevel(WINEVENT_LEVEL_ERROR),
> + TraceLoggingString(Buffer, "Error")
> + );
> + break;
> + default:
> + break;
> + }
> }
>
> -#define Log(_Format, ...) \
> - __Log(__MODULE__ "|" __FUNCTION__ ": " _Format, __VA_ARGS__)
> +#define LogInfo(_Format, ...) \
> + __Log(LOG_INFO, __MODULE__ "|" __FUNCTION__ ": " _Format,
> __VA_ARGS__)
> +
> +#define LogError(_Format, ...) \
> + __Log(LOG_ERROR, __MODULE__ "|" __FUNCTION__ ": " _Format,
> __VA_ARGS__)
>
> static PTSTR
> GetErrorMessage(
> @@ -215,7 +231,7 @@ ReportStatus(
> BOOL Success;
> HRESULT Error;
>
> - Log("====> (%s)", ServiceStateName(CurrentState));
> + LogInfo("====> (%s)", ServiceStateName(CurrentState));
>
> Context->Status.dwCurrentState = CurrentState;
> Context->Status.dwWin32ExitCode = Win32ExitCode;
> @@ -239,7 +255,7 @@ ReportStatus(
> if (!Success)
> goto fail1;
>
> - Log("<====");
> + LogInfo("<====");
>
> return;
>
> @@ -249,7 +265,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
> }
> @@ -326,11 +342,11 @@ DoReboot(
> _In_ DWORD Timeout
> )
> {
> - Log("waiting for pending install events...");
> + LogInfo("waiting for pending install events...");
>
> (VOID) CM_WaitNoPendingInstallEvents(INFINITE);
>
> - Log("initiating shutdown...");
> + LogInfo("initiating shutdown...");
>
> #pragma prefast(suppress:28159)
> (VOID) InitiateSystemShutdownEx(NULL,
> @@ -366,7 +382,7 @@ GetPromptTimeout(
> Type != REG_DWORD)
> Value = 0;
>
> - Log("%u", Value);
> + LogInfo("%u", Value);
>
> return Value;
> }
> @@ -445,18 +461,18 @@ GetDisplayName(
> return DisplayName;
>
> fail5:
> - Log("fail5");
> + LogError("fail5");
>
> fail4:
> - Log("fail4");
> + LogError("fail4");
>
> free(DisplayName);
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> RegCloseKey(ServiceKey);
>
> @@ -466,7 +482,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
> @@ -531,7 +547,7 @@ DoPromptForReboot(
> WTS_CONNECTSTATE_CLASS State = SessionInfo[Index].State;
> DWORD Response;
>
> - Log("[%u]: %s [%s]",
> + LogInfo("[%u]: %s [%s]",
> SessionId,
> Name,
> WTSStateName(State));
Could you align the arguments here (and elsewhere)?
> @@ -565,7 +581,7 @@ DoPromptForReboot(
> return ERROR_SUCCESS;
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
> *Prompt->PResponse = 0;
>
> fail1:
> @@ -598,7 +614,7 @@ PromptForReboot(
> return;
> Context->RebootPrompted = TRUE;
>
> - Log("====> (%s)", DriverName);
> + LogInfo("====> (%s)", DriverName);
>
> Prompt = calloc(1, sizeof (REBOOT_PROMPT));
> if (Prompt == NULL) {
> @@ -665,20 +681,20 @@ PromptForReboot(
> return;
>
> fail4:
> - Log("fail4");
> + LogError("fail4");
> free(DisplayName);
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
> @@ -772,7 +788,7 @@ TryAutoReboot(
> if (RebootCount >= AutoReboot)
> goto prompt;
>
> - Log("AutoRebooting (reboot %u of %u)\n",
> + LogInfo("AutoRebooting (reboot %u of %u)\n",
> RebootCount,
> AutoReboot);
>
> @@ -844,18 +860,18 @@ done:
> return;
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> free(DisplayName);
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
> @@ -878,7 +894,7 @@ RegEnumSubKeys(
> DWORD Index;
> HRESULT Error;
>
> - Log("====>");
> + LogInfo("====>");
>
> Error = RegQueryInfoKey(BaseKey,
> NULL,
> @@ -920,7 +936,7 @@ RegEnumSubKeys(
> goto fail3;
> }
>
> - Log("%s", SubKeyName);
> + LogInfo("%s", SubKeyName);
>
> if (!Callback(BaseKey, SubKeyName, Context))
> break;
> @@ -931,12 +947,12 @@ RegEnumSubKeys(
> return ERROR_SUCCESS;
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> free(SubKeyName);
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> fail1:
> Error = GetLastError();
> @@ -944,7 +960,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
> @@ -1008,7 +1024,7 @@ CheckRequestSubKeys(
> BOOLEAN Found;
> HRESULT Error;
>
> - Log("====>");
> + LogInfo("====>");
>
> Found = FALSE;
>
> @@ -1025,7 +1041,7 @@ CheckRequestSubKeys(
> (VOID) RegFlushKey(Context->ParametersKey);
> }
>
> - Log("<====");
> + LogInfo("<====");
>
> return;
>
> @@ -1035,7 +1051,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
> }
> @@ -1048,7 +1064,7 @@ CheckRequestKey(
> PMONITOR_CONTEXT Context = &MonitorContext;
> HRESULT Error;
>
> - Log("====>");
> + LogInfo("====>");
>
> CheckRequestSubKeys();
>
> @@ -1061,7 +1077,7 @@ CheckRequestKey(
> if (Error != ERROR_SUCCESS)
> goto fail1;
>
> - Log("<====");
> + LogInfo("<====");
>
> return;
>
> @@ -1071,7 +1087,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
> }
> @@ -1086,7 +1102,7 @@ AcquireShutdownPrivilege(
> BOOL Success;
> HRESULT Error;
>
> - Log("====>");
> + LogInfo("====>");
>
> New.PrivilegeCount = 1;
>
> @@ -1118,17 +1134,17 @@ AcquireShutdownPrivilege(
>
> CloseHandle(Token);
>
> - Log("<====");
> + LogInfo("<====");
>
> return TRUE;
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> CloseHandle(Token);
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> fail1:
> Error = GetLastError();
> @@ -1136,7 +1152,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
> @@ -1194,20 +1210,20 @@ GetRequestKeyName(
> goto fail4;
> }
>
> - Log("%s", *RequestKeyName);
> + LogInfo("%s", *RequestKeyName);
>
> return TRUE;
>
> fail4:
> - Log("fail4");
> + LogError("fail4");
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> free(*RequestKeyName);
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> fail1:
> Error = GetLastError();
> @@ -1215,7 +1231,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
> @@ -1321,37 +1337,37 @@ GetDialogParameters(
> return TRUE;
>
> fail10:
> - Log("fail10");
> + LogError("fail10");
>
> fail9:
> - Log("fail9");
> + LogError("fail9");
>
> free(Context->Question);
>
> fail8:
> - Log("fail8");
> + LogError("fail8");
>
> fail7:
> - Log("fail7");
> + LogError("fail7");
>
> fail6:
> - Log("fail6");
> + LogError("fail6");
>
> free(Context->Text);
>
> fail5:
> - Log("fail5");
> + LogError("fail5");
>
> fail4:
> - Log("fail4");
> + LogError("fail4");
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> free(Context->Title);
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> fail1:
> Error = GetLastError();
> @@ -1359,7 +1375,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
> @@ -1392,7 +1408,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
> @@ -1494,24 +1510,24 @@ done:
> return TRUE;
>
> fail6:
> - Log("fail6");
> + LogError("fail6");
>
> fail5:
> - Log("fail5");
> + LogError("fail5");
>
> fail4:
> - Log("fail4");
> + LogError("fail4");
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> free(Value);
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> fail1:
> - Log("fail1");
> + LogError("fail1");
>
> return TRUE;
> }
> @@ -1574,10 +1590,10 @@ RemoveAllStartOverrides(
> return TRUE;
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> fail1:
> - Log("fail1");
> + LogError("fail1");
>
> return FALSE;
> }
> @@ -1597,7 +1613,10 @@ MonitorMain(
> UNREFERENCED_PARAMETER(argc);
> UNREFERENCED_PARAMETER(argv);
>
> - Log("====>");
> + if (TraceLoggingRegister(g_xenbus_monitor) != ERROR_SUCCESS)
> + LogInfo("TraceLoggingRegister failed");
> +
> + LogInfo("====>");
>
> (VOID) RemoveStartOverride("stornvme");
> (VOID) RemoveAllStartOverrides();
> @@ -1620,11 +1639,6 @@ MonitorMain(
> if (Context->Service == NULL)
> goto fail3;
>
> - Context->EventLog = RegisterEventSource(NULL,
> - MONITOR_NAME);
> - if (Context->EventLog == NULL)
> - goto fail4;
> -
> Context->Status.dwServiceType = SERVICE_WIN32_OWN_PROCESS;
> Context->Status.dwServiceSpecificExitCode = 0;
>
> @@ -1636,26 +1650,26 @@ MonitorMain(
> NULL);
>
> if (Context->StopEvent == NULL)
> - goto fail5;
> + goto fail4;
>
> Context->RequestEvent = CreateEvent(NULL,
> TRUE,
> FALSE,
> NULL);
> if (Context->RequestEvent == NULL)
> - goto fail6;
> + goto fail5;
>
> Context->ResponseEvent = CreateEvent(NULL,
> FALSE,
> FALSE,
> NULL);
> if (Context->ResponseEvent == NULL)
> - goto fail7;
> + goto fail6;
> Context->Response = 0;
>
> Success = GetRequestKeyName(&RequestKeyName);
> if (!Success)
> - goto fail8;
> + goto fail7;
>
> Error = RegCreateKeyEx(HKEY_LOCAL_MACHINE,
> RequestKeyName,
> @@ -1667,15 +1681,15 @@ MonitorMain(
> &Context->RequestKey,
> NULL);
> if (Error != ERROR_SUCCESS)
> - goto fail9;
> + goto fail8;
>
> Success = GetDialogParameters();
> if (!Success)
> - goto fail10;
> + goto fail9;
>
> Context->Timer = CreateWaitableTimer(NULL, FALSE, NULL);
> if (Context->Timer == NULL)
> - goto fail11;
> + goto fail10;
>
> DueTime.QuadPart = -10000LL * REBOOT_RETRY_DELAY;
>
> @@ -1686,7 +1700,7 @@ MonitorMain(
> NULL,
> FALSE);
> if (!Success)
> - goto fail12;
> + goto fail11;
>
> SetEvent(Context->RequestEvent);
>
> @@ -1701,12 +1715,12 @@ MonitorMain(
> Events[2] = Context->ResponseEvent;
> Events[3] = Context->Timer;
>
> - Log("waiting (%u)...", ARRAYSIZE(Events));
> + LogInfo("waiting (%u)...", ARRAYSIZE(Events));
> Object = WaitForMultipleObjects(ARRAYSIZE(Events),
> Events,
> FALSE,
> INFINITE);
> - Log("awake");
> + LogInfo("awake");
>
> switch (Object) {
> case WAIT_OBJECT_0:
> @@ -1752,65 +1766,59 @@ done:
>
> ReportStatus(SERVICE_STOPPED, NO_ERROR, 0);
>
> - (VOID) DeregisterEventSource(Context->EventLog);
> -
> RegCloseKey(Context->ParametersKey);
>
> (VOID) RemoveStartOverride("stornvme");
> (VOID) RemoveAllStartOverrides();
>
> - Log("<====");
> + LogInfo("<====");
>
> + TraceLoggingUnregister(g_xenbus_monitor);
> return;
>
> -fail12:
> - Log("fail12");
> +fail11:
> + LogError("fail11");
>
> CloseHandle(Context->Timer);
>
> -fail11:
> - Log("fail11");
> -
> fail10:
> - Log("fail10");
> -
> - RegCloseKey(Context->RequestKey);
> + LogError("fail10");
>
> fail9:
> - Log("fail9");
> + LogError("fail9");
>
> - free(RequestKeyName);
> + RegCloseKey(Context->RequestKey);
>
> fail8:
> - Log("fail8");
> + LogError("fail8");
>
> - CloseHandle(Context->ResponseEvent);
> + free(RequestKeyName);
>
> fail7:
> - Log("fail7");
> + LogError("fail7");
>
> - CloseHandle(Context->RequestEvent);
> + CloseHandle(Context->ResponseEvent);
>
> fail6:
> - Log("fail6");
> + LogError("fail6");
>
> - CloseHandle(Context->StopEvent);
> + CloseHandle(Context->RequestEvent);
>
> fail5:
> - Log("fail5");
> + LogError("fail5");
>
> - ReportStatus(SERVICE_STOPPED, GetLastError(), 0);
> -
> - (VOID) DeregisterEventSource(Context->EventLog);
> + CloseHandle(Context->StopEvent);
>
> fail4:
> - Log("fail4");
> + LogError("fail4");
> +
> + ReportStatus(SERVICE_STOPPED, GetLastError(), 0);
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> RegCloseKey(Context->ParametersKey);
>
> @@ -1820,9 +1828,11 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
> +
> + TraceLoggingUnregister(g_xenbus_monitor);
> }
>
> static BOOL
> @@ -1835,7 +1845,7 @@ MonitorCreate(
> TCHAR Path[MAX_PATH];
> HRESULT Error;
>
> - Log("====>");
> + LogInfo("====>");
>
> if(!GetModuleFileName(NULL, Path, MAX_PATH))
> goto fail1;
> @@ -1867,17 +1877,17 @@ MonitorCreate(
> CloseServiceHandle(Service);
> CloseServiceHandle(SCManager);
>
> - Log("<====");
> + LogInfo("<====");
>
> return TRUE;
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> CloseServiceHandle(SCManager);
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> fail1:
> Error = GetLastError();
> @@ -1885,7 +1895,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
> @@ -1903,7 +1913,7 @@ MonitorDelete(
> SERVICE_STATUS Status;
> HRESULT Error;
>
> - Log("====>");
> + LogInfo("====>");
>
> SCManager = OpenSCManager(NULL,
> NULL,
> @@ -1934,20 +1944,20 @@ MonitorDelete(
> CloseServiceHandle(Service);
> CloseServiceHandle(SCManager);
>
> - Log("<====");
> + LogInfo("<====");
>
> return TRUE;
>
> fail4:
> - Log("fail4");
> + LogError("fail4");
>
> fail3:
> - Log("fail3");
> + LogError("fail3");
>
> CloseServiceHandle(Service);
>
> fail2:
> - Log("fail2");
> + LogError("fail2");
>
> CloseServiceHandle(SCManager);
>
> @@ -1957,7 +1967,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
> @@ -1975,14 +1985,14 @@ MonitorEntry(
> };
> HRESULT Error;
>
> - Log("%s (%s) ====>",
> + LogInfo("%s (%s) ====>",
> MAJOR_VERSION_STR "." MINOR_VERSION_STR "." MICRO_VERSION_STR "."
> BUILD_NUMBER_STR,
> DAY_STR "/" MONTH_STR "/" YEAR_STR);
>
> if (!StartServiceCtrlDispatcher(Table))
> goto fail1;
>
> - Log("%s (%s) <====",
> + LogInfo("%s (%s) <====",
> MAJOR_VERSION_STR "." MINOR_VERSION_STR "." MICRO_VERSION_STR "."
> BUILD_NUMBER_STR,
> DAY_STR "/" MONTH_STR "/" YEAR_STR);
>
> @@ -1994,7 +2004,7 @@ fail1:
> {
> PTSTR Message;
> Message = GetErrorMessage(Error);
> - Log("fail1 (%s)", Message);
> + LogError("fail1 (%s)", Message);
> LocalFree(Message);
> }
>
--
Ngoc Tu Dinh | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |