chore(build): robust windows service wrapper#6229
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a new GitHub Actions workflow to build and commit the Windows service binary. Introduces safer, bounded string handling across win64svc sources, updates Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant GH as GitHub Actions
participant MSYS as MSYS2/MinGW-w64
participant CMake as CMake/Ninja
participant Repo as Repository
Dev->>GH: Push branch / trigger workflow
GH->>Repo: Checkout repository & read win64svc SHA
GH->>MSYS: Install toolchain (gcc, cmake, ninja, git)
GH->>CMake: Configure (Ninja) and build win64svc
CMake-->>GH: Produce questdb.exe
GH->>GH: Verify binary (exists, objdump checks)
alt Binary updated
GH->>Repo: Stage & commit questdb.exe (with skip-ci)
GH->>Repo: Push to same branch
else No changes
GH->>Dev: Post summary indicating no update
end
sequenceDiagram
autonumber
participant SC as svcctrl
participant OS as Operating System / libc
participant SCM as Service Control Manager
SC->>SC: Compute needed length with snprintf(NULL,0,...)
alt Length calc fails or too large
SC->>SC: Log error and return E_PATH_TOO_LONG
else Length OK
SC->>OS: Allocate buffer (len+1)
alt Allocation fails
SC->>SC: Log error and return E_CREATE_SERVICE
else Allocation OK
SC->>SC: Build command into buffer with snprintf
SC->>SCM: Create/configure service using built command
SC->>OS: Free buffer
SC-->>SC: Return status
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
win64svc/src/svcctrl.c (1)
41-41: Optional: Remove redundantsizeof(char).
sizeof(char)is always 1 by definition, so the multiplication is redundant. While harmless, removing it improves clarity.- char *szPath = malloc((pathLen + 1) * sizeof(char)); + char *szPath = malloc(pathLen + 1);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/src/main/bin/questdb.exeis excluded by!**/*.exe
📒 Files selected for processing (5)
.github/workflows/rebuild_win64svc.yml(1 hunks)win64svc/src/common.h(1 hunks)win64svc/src/questdb.c(7 hunks)win64svc/src/service.c(3 hunks)win64svc/src/svcctrl.c(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: New pull request (Coverage Report Coverage Report)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (Hosted Running tests on windows-other)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-cairo)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-griffin)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (10)
win64svc/src/common.h (1)
13-13: LGTM!The new error code follows the existing sequential pattern and provides explicit signaling for path length validation failures introduced in the updated codebase.
win64svc/src/service.c (1)
157-157: LGTM!The replacement of
sprintfwith boundedsnprintfprevents potential buffer overflows in log message formatting.Also applies to: 166-166
.github/workflows/rebuild_win64svc.yml (1)
1-141: Solid workflow structure.The workflow appropriately:
- Uses MSYS2/MinGW-w64 for reproducible builds
- Captures win64svc commit SHA for audit trail
- Verifies binary output and dependencies
- Stages and commits with detailed metadata
- Handles no-change scenarios gracefully
- Marks commits as
[skip ci]to prevent infinite loopswin64svc/src/svcctrl.c (2)
27-53: Solid dynamic command construction.The two-step approach (calculate size, then allocate and build) ensures the buffer is sized exactly for the command string, with appropriate error handling on calculation failure and allocation failure.
Consider validating the final
snprintfcall at line 49 to ensure it matches the calculated length, as a defensive measure:// Build the command string safely - snprintf(szPath, pathLen + 1, "\"%s\" service -d %s%s -j \"%s\"", + int written = snprintf(szPath, pathLen + 1, "\"%s\" service -d %s%s -j \"%s\"", config->exeName, config->dir, config->forceCopy ? " -f" : "", config->javaExec); + if (written != pathLen) { + eprintf("Unexpected mismatch in command length (expected %d, got %d)\n", pathLen, written); + free(szPath); + CloseServiceHandle(hSCM); + return E_CREATE_SERVICE; + }
79-79: LGTM!Proper memory management: the allocated buffer is freed on both error and success paths.
Also applies to: 89-89
win64svc/src/questdb.c (5)
49-68: Excellent pathCopy refactor.The updated signature adds:
- Input validation (NULL checks, destSize > 0)
- Bounds checking before copy
- Clear error return code
- Guaranteed null termination
This establishes a safe pattern for path extraction operations across the codebase.
264-265: LGTM!Bounded
strncpywith explicit null termination prevents buffer overflows when copying Java executable paths.Also applies to: 284-285
270-279: LGTM!The Java executable discovery logic properly:
- Validates
pathCopyreturn codes- Checks remaining buffer space before
strcat- Falls back gracefully through local runtime → JAVA_HOME
Also applies to: 287-294
306-311: LGTM!Proper bounds checking on log directory path construction prevents buffer overflows.
238-238: LGTM!Replacing unbounded string operations with
snprintfthroughout the file ensures buffer safety.Also applies to: 390-390, 450-450
jerrinot
force-pushed
the
jh_windows_service_wrapper_robust
branch
from
6dfdfa8 to
c22af37
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
win64svc/src/questdb.c (1)
319-325: Past review concerns not addressed.The issues identified in the previous review remain:
- Line 320: The condition
len > 0should belen >= 0sincesnprintfcan legitimately return 0 for an empty result.- Line 321:
strftimereturns 0 on error or buffer overflow, but this return value is not validated, potentially leaving the buffer in an undefined state.Apply this diff to fix:
len = snprintf(log, MAX_PATH, "%s\\log\\stdout-stderr-", config->dir); - if (len > 0 && len < MAX_PATH) { - strftime(log + len, MAX_PATH - len, "%Y-%m-%dT%H-%M-%S.txt", t); + if (len >= 0 && len < MAX_PATH) { + size_t written = strftime(log + len, MAX_PATH - len, "%Y-%m-%dT%H-%M-%S.txt", t); + if (written == 0) { + eprintf("Log file path too long\n"); + return NULL; + } } else { eprintf("Log file path too long\n"); return NULL;
🧹 Nitpick comments (1)
win64svc/src/service.c (1)
164-164: Consider validating snprintf return values.While the bounds are correctly specified, the return values aren't checked for truncation. If
javaExecandjavaArgsare long, the log messages could be silently truncated, potentially omitting important debugging information.Optional: Add truncation checks after each snprintf:
- snprintf(buf, sizeof(buf), "Starting %s %s", gConfig->javaExec, gConfig->javaArgs); + int len = snprintf(buf, sizeof(buf), "Starting %s %s", gConfig->javaExec, gConfig->javaArgs); + if (len >= sizeof(buf)) { + log_event(EVENTLOG_WARNING_TYPE, gConfig->serviceName, "Log message truncated"); + } log_event(EVENTLOG_INFORMATION_TYPE, gConfig->serviceName, buf);Apply similar logic at line 173.
Also applies to: 173-173
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/src/main/bin/questdb.exeis excluded by!**/*.exe
📒 Files selected for processing (5)
.github/workflows/rebuild_win64svc.yml(1 hunks)win64svc/src/common.h(2 hunks)win64svc/src/questdb.c(7 hunks)win64svc/src/service.c(3 hunks)win64svc/src/svcctrl.c(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/rebuild_win64svc.yml
🔇 Additional comments (7)
win64svc/src/common.h (1)
13-13: LGTM!The new error codes follow the existing naming and numbering conventions. They provide clear semantic meaning for path-related errors and are appropriately used across the codebase.
Also applies to: 26-26
win64svc/src/service.c (1)
60-84: Past review concerns addressed.All error returns now consistently use
INVALID_HANDLE_VALUE, thesnprintfcheck correctly validateslen >= 0,strftimereturn value is validated, and a null-check forlocaltimewas added. The safer string handling with bounds checking is correctly implemented.win64svc/src/svcctrl.c (1)
27-53: LGTM! Robust dynamic path construction.The two-step approach (calculate length, then allocate and build) correctly eliminates the risk of buffer overflow. Error handling is thorough with proper cleanup (closing SCM handle and freeing memory) on all failure paths.
Also applies to: 79-79, 89-89
win64svc/src/questdb.c (4)
49-68: LGTM! Improved pathCopy with bounds checking.The updated signature with
destSizeparameter and error return enables safe, size-checked path extraction. Null-pointer validation, buffer overflow prevention, and explicit null termination are all correctly implemented.
106-110: Past review concern addressed.The error code is now correctly set to
ECONFIG_PATH_ERRORwhenpathCopyfails, providing accurate error reporting for path extraction failures.
259-300: LGTM! Comprehensive safety improvements in buildJavaExec.The refactored function now uses bounded operations throughout:
strncpywith explicit null termination for user-provided pathspathCopyreturn value checking before string concatenation- Length validation before all
strcatoperations- File existence verification before setting runtime flags
This eliminates buffer overflow risks while maintaining the existing logic.
444-446: LGTM! Complete error handling for path failures.The new
ECONFIG_PATH_ERRORcase provides clear error messaging for path extraction failures, and thesnprintfusage at line 454 adds bounds checking to prevent buffer overflows in error logging.Also applies to: 454-454
Built from win64svc commit: a110dcc Build timestamp: 2025-10-06 10:06:41 UTC Toolchain: MinGW-w64 on Windows Server 2022 GitHub Action: https://github.com/jerrinot/questdb/actions/runs/18277194437
jerrinot
force-pushed
the
jh_windows_service_wrapper_robust
branch
from
722aefa to
e6329e6
Compare
Built from win64svc commit: a110dcc Build timestamp: 2025-10-06 10:28:49 UTC Toolchain: MinGW-w64 on Windows Server 2022 GitHub Action: https://github.com/jerrinot/questdb/actions/runs/18277823240
2 participants
Adding automation to rebuild the Windows service wrapper.