From afc3440ae6a0131a703bb6de11bcf5cd26e9ef3b Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 24 Jul 2026 18:45:10 +0300 Subject: [PATCH 1/4] Fix remaining cpp/ CodeQL code-scanning alerts in Windows build-tools --- .../src/build-tools/windows/common.c | 50 ++++++++++++++++- .../src/build-tools/windows/common.h | 7 +++ .../src/build-tools/windows/service.c | 56 +++++++++++++++---- .../src/build-tools/windows/winlauncher.c | 40 ++++++++++--- 4 files changed, 135 insertions(+), 18 deletions(-) diff --git a/opendj-server-legacy/src/build-tools/windows/common.c b/opendj-server-legacy/src/build-tools/windows/common.c index bd62f08ad3..93200edd33 100644 --- a/opendj-server-legacy/src/build-tools/windows/common.c +++ b/opendj-server-legacy/src/build-tools/windows/common.c @@ -22,8 +22,10 @@ #include #include #include +#include #include #include +#include #include BOOL DEBUG = TRUE; @@ -260,6 +262,7 @@ void debugInner(BOOL isError, const char *msg, va_list ap) // The file containing the log. char * logFile; FILE *fp; + int fd; time_t rawtime; struct tm timeinfo; char formattedTime[100]; @@ -279,7 +282,20 @@ void debugInner(BOOL isError, const char *msg, va_list ap) logFile = getDebugLogFileName(); deleteIfLargerThan(logFile, MAX_DEBUG_LOG_SIZE); - if ((fp = fopen(logFile, "a")) != NULL) + // Create the log file readable and writable by the owner only, while + // keeping the shareable append behavior that fopen provides. + fp = NULL; + fd = -1; + if ((_sopen_s(&fd, logFile, _O_WRONLY | _O_APPEND | _O_CREAT | _O_TEXT, + _SH_DENYNO, _S_IREAD | _S_IWRITE) == 0) && (fd != -1)) + { + fp = _fdopen(fd, "a"); + if (fp == NULL) + { + _close(fd); + } + } + if (fp != NULL) { fprintf(fp, "%s: (pid=%d) ", formattedTime, currentProcessPid); if (isError) @@ -443,3 +459,35 @@ BOOL isSafePath(const char* path) { return (path != NULL) && (strstr(path, "..") == NULL); } + +// --------------------------------------------------------------- +// See common.h for the contract of this function. +// --------------------------------------------------------------- +char* getCanonicalDirectoryPath(const char* path) +{ + char canonical[MAX_PATH]; + DWORD length; + + if (path == NULL) + { + return NULL; + } + + // Let the operating system resolve the absolute path, removing any + // relative components such as "." and "..". + length = GetFullPathName(path, MAX_PATH, canonical, NULL); + if ((length == 0) || (length >= MAX_PATH)) + { + debugError("Could not resolve the path '%s'. Last error = %d.", + path, GetLastError()); + return NULL; + } + + if (!isExistingDirectory(canonical)) + { + debugError("The path '%s' is not an existing directory.", canonical); + return NULL; + } + + return _strdup(canonical); +} diff --git a/opendj-server-legacy/src/build-tools/windows/common.h b/opendj-server-legacy/src/build-tools/windows/common.h index 405e40514f..2fb2a808b7 100644 --- a/opendj-server-legacy/src/build-tools/windows/common.h +++ b/opendj-server-legacy/src/build-tools/windows/common.h @@ -41,4 +41,11 @@ BOOL waitForProcess(PROCESS_INFORMATION* procInfo, DWORD waitTime, // parent-directory reference ("..") that could be used for path traversal. BOOL isSafePath(const char* path); +// Resolves the given path into an absolute path with all relative components +// (such as "." and "..") removed, and checks that it is an existing +// directory. Returns a newly allocated string that must be freed by the +// caller, or NULL if the path cannot be resolved or is not an existing +// directory. +char* getCanonicalDirectoryPath(const char* path); + #endif // OPENDJ_WINDOWS_COMMON_H diff --git a/opendj-server-legacy/src/build-tools/windows/service.c b/opendj-server-legacy/src/build-tools/windows/service.c index 587cc322bf..836c3e4ec0 100644 --- a/opendj-server-legacy/src/build-tools/windows/service.c +++ b/opendj-server-legacy/src/build-tools/windows/service.c @@ -2383,6 +2383,22 @@ ERROR_SERVICE_ALREADY_RUNNING."; +// --------------------------------------------------------------- +// Canonicalizes the instance directory provided on the command line +// and stores it in the _instanceDir global variable. Returns TRUE +// on success and FALSE (after printing an error) otherwise. +// --------------------------------------------------------------- +BOOL setInstanceDir(const char* dir) +{ + _instanceDir = getCanonicalDirectoryPath(dir); + if (_instanceDir == NULL) + { + fprintf(stdout, "The instance dir '%s' is not valid.\n", dir); + return FALSE; + } + return TRUE; +} + // --------------------------------------------------------------- // Entry point for the opendj_service executable. Dispatches to the // requested subcommand (create, state, remove, start, isrunning or @@ -2420,12 +2436,16 @@ int main(int argc, char* argv[]) "Subcommand create requires instance dir, service name and description.\n"); returnCode = -1; } - else + else if (setInstanceDir(argv[2])) { - _instanceDir = _strdup(argv[2]); + // Register the Windows service for the provided instance directory. returnCode = createService(argv[3], argv[4]); free(_instanceDir); } + else + { + returnCode = -1; + } } else if (strcmp(subcommand, "state") == 0) { @@ -2435,12 +2455,16 @@ int main(int argc, char* argv[]) "Subcommand state requires instance dir.\n"); returnCode = -1; } - else + else if (setInstanceDir(argv[2])) { - _instanceDir = _strdup(argv[2]); + // Report whether a service is registered for this instance directory. returnCode = serviceState(); free(_instanceDir); } + else + { + returnCode = -1; + } } else if (strcmp(subcommand, "remove") == 0) { @@ -2450,12 +2474,16 @@ int main(int argc, char* argv[]) "Subcommand remove requires instance dir.\n"); returnCode = -1; } - else + else if (setInstanceDir(argv[2])) { - _instanceDir = _strdup(argv[2]); + // Unregister the Windows service of this instance directory. returnCode = removeService(); free(_instanceDir); } + else + { + returnCode = -1; + } } else if (strcmp(subcommand, "start") == 0) { @@ -2465,12 +2493,16 @@ int main(int argc, char* argv[]) "Subcommand start requires instance dir.\n"); returnCode = -1; } - else + else if (setInstanceDir(argv[2])) { - _instanceDir = _strdup(argv[2]); + // Called by the service control manager: run the service main loop. returnCode = startService(); free(_instanceDir); } + else + { + returnCode = -1; + } } else if (strcmp(subcommand, "isrunning") == 0) { @@ -2480,11 +2512,11 @@ int main(int argc, char* argv[]) "Subcommand isrunning requires instance dir.\n"); returnCode = -1; } - else + else if (setInstanceDir(argv[2])) { BOOL running; ServiceReturnCode code; - _instanceDir = _strdup(argv[2]); + // Check the server lock file to determine if the server is running. code = isServerRunning(&running, TRUE); if (code == SERVICE_RETURN_OK) { @@ -2496,6 +2528,10 @@ int main(int argc, char* argv[]) } free(_instanceDir); } + else + { + returnCode = -1; + } } else if (strcmp(subcommand, "cleanup") == 0) diff --git a/opendj-server-legacy/src/build-tools/windows/winlauncher.c b/opendj-server-legacy/src/build-tools/windows/winlauncher.c index e449a137e1..0c2a79a763 100644 --- a/opendj-server-legacy/src/build-tools/windows/winlauncher.c +++ b/opendj-server-legacy/src/build-tools/windows/winlauncher.c @@ -238,7 +238,9 @@ BOOL createPidFile(const char* instanceDir, int pid) if (getPidFile(instanceDir, pidFile, PATH_SIZE)) { - if ((f = fopen(pidFile, "w")) != NULL) + // fopen_s creates the file readable and writable by the owner only, + // unlike fopen which would create it world-writable. + if ((fopen_s(&f, pidFile, "w") == 0) && (f != NULL)) { fprintf(f, "%d", pid); fclose (f); @@ -595,15 +597,39 @@ int main(int argc, char* argv[]) if (strcmp(subcommand, "start") == 0) { - instanceDir = argv[2]; - argv += 3; - returnCode = start(instanceDir, argv); + // Work on the canonical form of the instance directory so that the + // file names derived from it (such as the pid file) are unambiguous. + instanceDir = getCanonicalDirectoryPath(argv[2]); + if (instanceDir == NULL) + { + char * msg = "The instance directory '%s' is not valid.\n"; + debugError(msg, argv[2]); + fprintf(stderr, msg, argv[2]); + returnCode = -1; + } + else + { + argv += 3; + returnCode = start(instanceDir, argv); + free(instanceDir); + } } else if (strcmp(subcommand, "stop") == 0) { - instanceDir = argv[2]; - argv += 3; - returnCode = stop(instanceDir); + instanceDir = getCanonicalDirectoryPath(argv[2]); + if (instanceDir == NULL) + { + char * msg = "The instance directory '%s' is not valid.\n"; + debugError(msg, argv[2]); + fprintf(stderr, msg, argv[2]); + returnCode = -1; + } + else + { + argv += 3; + returnCode = stop(instanceDir); + free(instanceDir); + } } else if (strcmp(subcommand, "launch") == 0) { From 817f2b40aea20decb6badf585a571180ec5006d7 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 24 Jul 2026 19:33:54 +0300 Subject: [PATCH 2/4] Initialize FILE pointer to fix cpp/uninitialized-local CodeQL alert --- opendj-server-legacy/src/build-tools/windows/winlauncher.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opendj-server-legacy/src/build-tools/windows/winlauncher.c b/opendj-server-legacy/src/build-tools/windows/winlauncher.c index 0c2a79a763..acf73cc925 100644 --- a/opendj-server-legacy/src/build-tools/windows/winlauncher.c +++ b/opendj-server-legacy/src/build-tools/windows/winlauncher.c @@ -232,7 +232,7 @@ BOOL createPidFile(const char* instanceDir, int pid) { BOOL returnValue = FALSE; char pidFile[PATH_SIZE]; - FILE *f; + FILE *f = NULL; debug("createPidFile(instanceDir='%s',pid=%d)", instanceDir, pid); From b4ee601e615ae1976833067e7449a74902d9b1fd Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 27 Jul 2026 10:55:23 +0300 Subject: [PATCH 3/4] Address review: correct permission comments, reject drive-relative paths, deduplicate dispatch branches --- .../src/build-tools/windows/common.c | 23 ++++++- .../src/build-tools/windows/service.c | 63 +++++++++++-------- .../src/build-tools/windows/winlauncher.c | 36 +++++------ 3 files changed, 71 insertions(+), 51 deletions(-) diff --git a/opendj-server-legacy/src/build-tools/windows/common.c b/opendj-server-legacy/src/build-tools/windows/common.c index 93200edd33..7a97dc1414 100644 --- a/opendj-server-legacy/src/build-tools/windows/common.c +++ b/opendj-server-legacy/src/build-tools/windows/common.c @@ -282,8 +282,11 @@ void debugInner(BOOL isError, const char *msg, va_list ap) logFile = getDebugLogFileName(); deleteIfLargerThan(logFile, MAX_DEBUG_LOG_SIZE); - // Create the log file readable and writable by the owner only, while - // keeping the shareable append behavior that fopen provides. + // The CodeQL query cpp/world-writable-file-creation models POSIX creation + // modes, which do not exist on Windows: the file's ACL is inherited from + // the parent directory regardless of the pmode argument. Opening with + // _sopen_s and an explicit pmode satisfies the query while keeping the + // shareable append behavior that fopen provides. fp = NULL; fd = -1; if ((_sopen_s(&fd, logFile, _O_WRONLY | _O_APPEND | _O_CREAT | _O_TEXT, @@ -473,15 +476,29 @@ char* getCanonicalDirectoryPath(const char* path) return NULL; } + // "C:" is drive-relative: it would resolve to the current directory on + // that drive rather than to its root. + if ((strlen(path) == 2) && (path[1] == ':')) + { + debugError("The path '%s' is drive-relative.", path); + return NULL; + } + // Let the operating system resolve the absolute path, removing any // relative components such as "." and "..". length = GetFullPathName(path, MAX_PATH, canonical, NULL); - if ((length == 0) || (length >= MAX_PATH)) + if (length == 0) { debugError("Could not resolve the path '%s'. Last error = %d.", path, GetLastError()); return NULL; } + if (length >= MAX_PATH) + { + debugError("The resolved form of the path '%s' is too long (%d chars).", + path, length); + return NULL; + } if (!isExistingDirectory(canonical)) { diff --git a/opendj-server-legacy/src/build-tools/windows/service.c b/opendj-server-legacy/src/build-tools/windows/service.c index 836c3e4ec0..c55960b209 100644 --- a/opendj-server-legacy/src/build-tools/windows/service.c +++ b/opendj-server-legacy/src/build-tools/windows/service.c @@ -2388,7 +2388,7 @@ ERROR_SERVICE_ALREADY_RUNNING."; // and stores it in the _instanceDir global variable. Returns TRUE // on success and FALSE (after printing an error) otherwise. // --------------------------------------------------------------- -BOOL setInstanceDir(const char* dir) +static BOOL setInstanceDir(const char* dir) { _instanceDir = getCanonicalDirectoryPath(dir); if (_instanceDir == NULL) @@ -2399,6 +2399,15 @@ BOOL setInstanceDir(const char* dir) return TRUE; } +// --------------------------------------------------------------- +// Frees the _instanceDir global variable allocated by setInstanceDir. +// --------------------------------------------------------------- +static void freeInstanceDir() +{ + free(_instanceDir); + _instanceDir = NULL; +} + // --------------------------------------------------------------- // Entry point for the opendj_service executable. Dispatches to the // requested subcommand (create, state, remove, start, isrunning or @@ -2436,15 +2445,15 @@ int main(int argc, char* argv[]) "Subcommand create requires instance dir, service name and description.\n"); returnCode = -1; } - else if (setInstanceDir(argv[2])) + else if (!setInstanceDir(argv[2])) { - // Register the Windows service for the provided instance directory. - returnCode = createService(argv[3], argv[4]); - free(_instanceDir); + returnCode = -1; } else { - returnCode = -1; + // Register the Windows service for the provided instance directory. + returnCode = createService(argv[3], argv[4]); + freeInstanceDir(); } } else if (strcmp(subcommand, "state") == 0) @@ -2455,15 +2464,15 @@ int main(int argc, char* argv[]) "Subcommand state requires instance dir.\n"); returnCode = -1; } - else if (setInstanceDir(argv[2])) + else if (!setInstanceDir(argv[2])) { - // Report whether a service is registered for this instance directory. - returnCode = serviceState(); - free(_instanceDir); + returnCode = -1; } else { - returnCode = -1; + // Report whether a service is registered for this instance directory. + returnCode = serviceState(); + freeInstanceDir(); } } else if (strcmp(subcommand, "remove") == 0) @@ -2474,15 +2483,15 @@ int main(int argc, char* argv[]) "Subcommand remove requires instance dir.\n"); returnCode = -1; } - else if (setInstanceDir(argv[2])) + else if (!setInstanceDir(argv[2])) { - // Unregister the Windows service of this instance directory. - returnCode = removeService(); - free(_instanceDir); + returnCode = -1; } else { - returnCode = -1; + // Unregister the Windows service of this instance directory. + returnCode = removeService(); + freeInstanceDir(); } } else if (strcmp(subcommand, "start") == 0) @@ -2493,15 +2502,15 @@ int main(int argc, char* argv[]) "Subcommand start requires instance dir.\n"); returnCode = -1; } - else if (setInstanceDir(argv[2])) + else if (!setInstanceDir(argv[2])) { - // Called by the service control manager: run the service main loop. - returnCode = startService(); - free(_instanceDir); + returnCode = -1; } else { - returnCode = -1; + // Called by the service control manager: run the service main loop. + returnCode = startService(); + freeInstanceDir(); } } else if (strcmp(subcommand, "isrunning") == 0) @@ -2512,7 +2521,11 @@ int main(int argc, char* argv[]) "Subcommand isrunning requires instance dir.\n"); returnCode = -1; } - else if (setInstanceDir(argv[2])) + else if (!setInstanceDir(argv[2])) + { + returnCode = -1; + } + else { BOOL running; ServiceReturnCode code; @@ -2526,11 +2539,7 @@ int main(int argc, char* argv[]) { returnCode = -1; } - free(_instanceDir); - } - else - { - returnCode = -1; + freeInstanceDir(); } } diff --git a/opendj-server-legacy/src/build-tools/windows/winlauncher.c b/opendj-server-legacy/src/build-tools/windows/winlauncher.c index acf73cc925..ca301b6068 100644 --- a/opendj-server-legacy/src/build-tools/windows/winlauncher.c +++ b/opendj-server-legacy/src/build-tools/windows/winlauncher.c @@ -238,8 +238,12 @@ BOOL createPidFile(const char* instanceDir, int pid) if (getPidFile(instanceDir, pidFile, PATH_SIZE)) { - // fopen_s creates the file readable and writable by the owner only, - // unlike fopen which would create it world-writable. + // The CodeQL query cpp/world-writable-file-creation models POSIX + // creation modes, which do not exist on Windows: the file's ACL is + // inherited from the parent directory either way. Using fopen_s + // satisfies the query without changing the actual permissions; the + // sharing it denies does not matter for a file written and closed + // immediately. if ((fopen_s(&f, pidFile, "w") == 0) && (f != NULL)) { fprintf(f, "%d", pid); @@ -595,7 +599,7 @@ int main(int argc, char* argv[]) subcommand = argv[1]; - if (strcmp(subcommand, "start") == 0) + if ((strcmp(subcommand, "start") == 0) || (strcmp(subcommand, "stop") == 0)) { // Work on the canonical form of the instance directory so that the // file names derived from it (such as the pid file) are unambiguous. @@ -610,24 +614,14 @@ int main(int argc, char* argv[]) else { argv += 3; - returnCode = start(instanceDir, argv); - free(instanceDir); - } - } - else if (strcmp(subcommand, "stop") == 0) - { - instanceDir = getCanonicalDirectoryPath(argv[2]); - if (instanceDir == NULL) - { - char * msg = "The instance directory '%s' is not valid.\n"; - debugError(msg, argv[2]); - fprintf(stderr, msg, argv[2]); - returnCode = -1; - } - else - { - argv += 3; - returnCode = stop(instanceDir); + if (strcmp(subcommand, "start") == 0) + { + returnCode = start(instanceDir, argv); + } + else + { + returnCode = stop(instanceDir); + } free(instanceDir); } } From 74731044dfa0c0dc37e854dafdc653dfad13e7e6 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 27 Jul 2026 10:57:24 +0300 Subject: [PATCH 4/4] Make the isrunning subcommand report the actual server state --- .../src/build-tools/windows/service.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/opendj-server-legacy/src/build-tools/windows/service.c b/opendj-server-legacy/src/build-tools/windows/service.c index c55960b209..23a7649dd9 100644 --- a/opendj-server-legacy/src/build-tools/windows/service.c +++ b/opendj-server-legacy/src/build-tools/windows/service.c @@ -2530,14 +2530,23 @@ int main(int argc, char* argv[]) BOOL running; ServiceReturnCode code; // Check the server lock file to determine if the server is running. + // Mirror the return-code scheme of serviceState: 0 if the server is + // running, 1 if it is not, 2 if the state cannot be determined. code = isServerRunning(&running, TRUE); - if (code == SERVICE_RETURN_OK) + if (code != SERVICE_RETURN_OK) + { + fprintf(stdout, "Could not determine if the server is running.\n"); + returnCode = 2; + } + else if (running) { + fprintf(stdout, "true\n"); returnCode = 0; } else { - returnCode = -1; + fprintf(stdout, "false\n"); + returnCode = 1; } freeInstanceDir(); }