feat: S13.4 Winsock UDP transport (resolver + datagram)#149
Merged
Conversation
Windows-side implementations of the Resolver and Datagram abstractions, bringing up UDP syslog delivery on Windows. Mirrors the POSIX side (GetAddrInfoResolver, PosixDatagram) with Winsock semantics (SOCKET, INVALID_SOCKET, closesocket, SOCKET_ERROR). Injection for unit tests uses CppUTest's UT_PTR_SET rather than GCC-style weak/strong symbol override (MSVC does not support the latter). Each production translation unit owns its Winsock function-pointer seam via a library-internal header in Platform/Windows/Source/; tests swap the pointers for WinsockFake_* at setup. The WinsockAddress companion (header + internal + .c) is literally identical to the POSIX version aside from the <netinet/in.h> / <winsock2.h> header swap — struct sockaddr_in is layout-compatible, so the SOLIDSYSLOG_ADDRESS_SIZE static assert from S13.11 holds unchanged. Closes #128. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds optional Windows Winsock support: CMake detection and Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver as SolidSyslogWinsockResolver
participant Winsock as Winsock API
participant Storage as SolidSyslogAddressStorage
Client->>Resolver: Resolve(storage)
Resolver->>Resolver: call getHost(), call getPort()
Resolver->>Winsock: getaddrinfo(host, NULL, hints)
alt Success
Winsock-->>Resolver: addrinfo* (ai_addr = sockaddr_in)
Resolver->>Storage: copy ai_addr → storage
Resolver->>Resolver: set sin_port = htons(port)
Resolver->>Winsock: freeaddrinfo(addrinfo*)
Resolver-->>Client: true
else Failure
Winsock-->>Resolver: error
Resolver-->>Client: false
end
sequenceDiagram
participant Client
participant Datagram as SolidSyslogWinsockDatagram
participant Winsock as Winsock API
participant Storage as SolidSyslogAddressStorage
Client->>Datagram: Open()
Datagram->>Winsock: socket(AF_INET, SOCK_DGRAM, 0)
alt Success
Winsock-->>Datagram: SOCKET fd
Datagram-->>Client: true
else Failure
Winsock-->>Datagram: INVALID_SOCKET
Datagram-->>Client: false
end
Client->>Datagram: SendTo(buf, len, storage)
Datagram->>Storage: cast storage → sockaddr_in*
Datagram->>Winsock: sendto(fd, buf, len, 0, sockaddr_in*, sizeof)
alt Success
Winsock-->>Datagram: bytes_sent
Datagram-->>Client: true
else Failure
Winsock-->>Datagram: SOCKET_ERROR
Datagram-->>Client: false
end
Client->>Datagram: Close()
alt fd != INVALID_SOCKET
Datagram->>Winsock: closesocket(fd)
Winsock-->>Datagram: 0
Datagram->>Datagram: fd = INVALID_SOCKET
end
Datagram-->>Client: (void)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 531 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Post-format pass on the two files the CI `format` check flagged that my local sweep missed (I had run the format check before adding the later test files). No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 531 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Platform/Windows/Source/SolidSyslogWinsockDatagramInternal.h (1)
18-20: Rename seam globals to camelCase for guideline compliance.These globals are variables, so underscore-based names (
Winsock_socket,Winsock_sendto,Winsock_closesocket) miss the repository camelCase rule.♻️ Suggested rename
- extern WinsockSocketFn Winsock_socket; - extern WinsockSendToFn Winsock_sendto; - extern WinsockCloseSocketFn Winsock_closesocket; + extern WinsockSocketFn winsockSocket; + extern WinsockSendToFn winsockSendto; + extern WinsockCloseSocketFn winsockClosesocket;As per coding guidelines,
**/*.{c,cpp,h,hpp,cc,cxx}variables must usecamelCasenaming convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Platform/Windows/Source/SolidSyslogWinsockDatagramInternal.h` around lines 18 - 20, The three extern globals use underscore_case; rename them to camelCase to match repo rules: change Winsock_socket -> winsockSocket, Winsock_sendto -> winsockSendTo, and Winsock_closesocket -> winsockCloseSocket. Update the extern declarations (WinsockSocketFn Winsock_socket, WinsockSendToFn Winsock_sendto, WinsockCloseSocketFn Winsock_closesocket) and every definition and use-site across the codebase to the new names, and run a build to ensure no remaining references; keep types and signatures unchanged.Tests/SolidSyslogWinsockResolverTest.cpp (1)
57-70: ConsidercamelCasefor test helper methods.The helper methods
Resolve()andResult()use PascalCase. Per coding guidelines, C++ methods should use camelCase (e.g.,resolve(),result()). This is a minor naming consistency concern.💅 Optional naming adjustment
- bool Resolve(enum SolidSyslogTransport transport = SOLIDSYSLOG_TRANSPORT_UDP) + bool resolve(enum SolidSyslogTransport transport = SOLIDSYSLOG_TRANSPORT_UDP) { struct SolidSyslogAddress* address = SolidSyslogAddress_FromStorage(&resultStorage); return resolver->Resolve(resolver, transport, address); } // NOLINTNEXTLINE(modernize-use-nodiscard) -- used through accessor syntax in tests - const struct sockaddr_in* Result() const + const struct sockaddr_in* result() const {As per coding guidelines: "C++ methods must use
camelCasenaming convention".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/SolidSyslogWinsockResolverTest.cpp` around lines 57 - 70, Rename the helper methods Resolve() and Result() to camelCase (resolve() and result()) throughout this test class and any tests that call them; update the definitions that call SolidSyslogAddress_FromStorage(&resultStorage) and return the sockaddr_in pointer (currently using the bytes reinterpret_cast and returning reinterpret_cast<const struct sockaddr_in*>(bytes)) to the new names and preserve the existing NOLINT comments and behavior; also update any uses of resolver->Resolve to avoid name conflicts (ensure calls to the resolver object keep its Resolve method intact) and run tests to confirm all call sites were updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 69-70: The check_symbol_exists call for sendto in winsock2.h can
fail because the ws2_32 library isn't specified for the linkage check; set
CMAKE_REQUIRED_LIBRARIES to ws2_32 before calling check_symbol_exists and
restore/clear it afterward so the check_symbol_exists("sendto" "winsock2.h"
HAVE_WINSOCK) correctly links on Windows and option(SOLIDSYSLOG_WINSOCK ...)
reflects the real result.
In `@Tests/Support/WinsockFake.c`:
- Around line 226-232: Guard against negative len and NULL/too-small address
pointers before copying in the sendto capture logic: validate that len >= 0
before casting to size_t and computing copySize for lastBufCopy, and validate
that to != NULL and tolen >= sizeof(struct sockaddr_in) before dereferencing
into lastAddr; if validation fails, set lastLen/lastAddrLen/lastFlags to safe
defaults and avoid the memcpy and address copy to prevent undefined behavior
(update the block that writes lastBufCopy, lastLen, lastFlags, lastAddr,
lastAddrLen accordingly).
---
Nitpick comments:
In `@Platform/Windows/Source/SolidSyslogWinsockDatagramInternal.h`:
- Around line 18-20: The three extern globals use underscore_case; rename them
to camelCase to match repo rules: change Winsock_socket -> winsockSocket,
Winsock_sendto -> winsockSendTo, and Winsock_closesocket -> winsockCloseSocket.
Update the extern declarations (WinsockSocketFn Winsock_socket, WinsockSendToFn
Winsock_sendto, WinsockCloseSocketFn Winsock_closesocket) and every definition
and use-site across the codebase to the new names, and run a build to ensure no
remaining references; keep types and signatures unchanged.
In `@Tests/SolidSyslogWinsockResolverTest.cpp`:
- Around line 57-70: Rename the helper methods Resolve() and Result() to
camelCase (resolve() and result()) throughout this test class and any tests that
call them; update the definitions that call
SolidSyslogAddress_FromStorage(&resultStorage) and return the sockaddr_in
pointer (currently using the bytes reinterpret_cast and returning
reinterpret_cast<const struct sockaddr_in*>(bytes)) to the new names and
preserve the existing NOLINT comments and behavior; also update any uses of
resolver->Resolve to avoid name conflicts (ensure calls to the resolver object
keep its Resolve method intact) and run tests to confirm all call sites were
updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06286230-38a5-4bdb-bfd9-ea67788a7a8c
📒 Files selected for processing (18)
CMakeLists.txtPlatform/Windows/CMakeLists.txtPlatform/Windows/Interface/SolidSyslogAddress.hPlatform/Windows/Interface/SolidSyslogWinsockDatagram.hPlatform/Windows/Interface/SolidSyslogWinsockResolver.hPlatform/Windows/Source/SolidSyslogAddress.cPlatform/Windows/Source/SolidSyslogAddressInternal.hPlatform/Windows/Source/SolidSyslogWinsockDatagram.cPlatform/Windows/Source/SolidSyslogWinsockDatagramInternal.hPlatform/Windows/Source/SolidSyslogWinsockResolver.cPlatform/Windows/Source/SolidSyslogWinsockResolverInternal.hTests/CMakeLists.txtTests/SolidSyslogWinsockDatagramTest.cppTests/SolidSyslogWinsockResolverTest.cppTests/Support/CMakeLists.txtTests/Support/WinsockFake.cTests/Support/WinsockFake.hTests/WinsockFakeTest.cpp
- CMakeLists.txt: set CMAKE_REQUIRED_LIBRARIES=ws2_32 before the check_symbol_exists(sendto "winsock2.h" HAVE_WINSOCK) call, save/restore the previous value. Some MSVC toolchains return false-negatives without it because symbol resolution happens at link time. - Tests/Support/WinsockFake.c: guard WinsockFake_sendto against negative len, NULL buf and short/NULL addr before copying. Protects the fake against theoretical UB; unaffected by current callers which always pass valid inputs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai — responses to the two nitpicks in the review summary: Nitpick 1 — Nitpick 2 — Test helper methods |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
I'll record both decisions to avoid re-raising them in future reviews. ✏️ Learnings added
🧠 Learnings used |
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 531 passed, 🙈 3 skipped) 🚧 Error MessagesCreated by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Tests/Support/WinsockFake.c (1)
240-243:⚠️ Potential issue | 🟡 MinorReset
lastAddrwhentois invalid to avoid stale captures.At Line 240,
lastAddris only updated on validto/tolen; otherwise prior state is retained and can leak into later assertions in the same test.Proposed fix
if (to != NULL && tolen >= (int) sizeof(struct sockaddr_in)) { lastAddr = *(const struct sockaddr_in*) to; } + else + { + lastAddr = (struct sockaddr_in) {0}; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Support/WinsockFake.c` around lines 240 - 243, The test helper leaves stale data in lastAddr when the incoming to/tolen are invalid; update the conditional around the assignment to set lastAddr to a zeroed/invalid struct when to is NULL or tolen is too small so prior captures cannot leak into later assertions—i.e., in the same block that currently does "if (to != NULL && tolen >= (int) sizeof(struct sockaddr_in)) { lastAddr = *(const struct sockaddr_in*) to; }" add an else branch that clears lastAddr (e.g., zeroes the struct or sets sin_family to 0) so callers can detect no-valid-address was captured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/Support/WinsockFake.c`:
- Around line 254-266: In WinsockFake_getaddrinfo, guard against a NULL res and
check inet_pton's return before using its output: first if (res == NULL) return
EAI_FAIL (or an appropriate EAI_*), then only call inet_pton when node is
non-NULL and inspect its return value; if inet_pton returns 1 proceed to copy
into fakeResolvedAddr.sin_addr and set *res = &fakeAddrInfo, otherwise return
EAI_NONAME (or another suitable getaddrinfo error) so invalid or unparsable node
strings don't produce silent failures; keep the existing getAddrInfoFails
short-circuit behavior using getAddrInfoFails and preserve
lastGetAddrInfoSocktype/hostname bookkeeping.
---
Duplicate comments:
In `@Tests/Support/WinsockFake.c`:
- Around line 240-243: The test helper leaves stale data in lastAddr when the
incoming to/tolen are invalid; update the conditional around the assignment to
set lastAddr to a zeroed/invalid struct when to is NULL or tolen is too small so
prior captures cannot leak into later assertions—i.e., in the same block that
currently does "if (to != NULL && tolen >= (int) sizeof(struct sockaddr_in)) {
lastAddr = *(const struct sockaddr_in*) to; }" add an else branch that clears
lastAddr (e.g., zeroes the struct or sets sin_family to 0) so callers can detect
no-valid-address was captured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f318bf91-bb40-44f3-a201-7588f9908ff4
📒 Files selected for processing (2)
CMakeLists.txtTests/Support/WinsockFake.c
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
MSVC's /WX turns C4232 into an error: taking the address of a __declspec(dllimport) function for static pointer initialisation is a "nonstandard extension" because the address isn't a compile-time constant. Introduce file-local WSAAPI forwarders (CallSocket, CallSendTo etc.) whose addresses are compile-time-constant, and point each seam at its forwarder instead of the raw Winsock symbol. No runtime behaviour change — the forwarder is a one-line call that the compiler usually inlines; the seam still points UT_PTR_SET at the fake. This surfaced after the earlier check_symbol_exists fix: before that fix, HAVE_WINSOCK was silently false on MSVC (the probe couldn't link sendto without ws2_32), so SOLIDSYSLOG_WINSOCK was OFF and the Winsock sources were never compiled on windows-latest — 362 Windows tests ran instead of the expected ~500. Now both the detection and the compile succeed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 531 passed, 🙈 3 skipped) 🚧 Error MessagesCreated by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
…nsock SolidSyslogDatagram.c and SolidSyslogResolver.c are pure vtable dispatchers with no platform dependency — they just forward to the instance's Open/SendTo/Close/Resolve hooks. They were gated on SOLIDSYSLOG_POSIX, so on Windows the Winsock datagram tests linked against undefined SolidSyslogDatagram_Open / _SendTo / _Close (LNK2019). Split the guard: dispatchers compile when either platform is enabled; Stream / UdpSender / TcpSender stay POSIX-only until S13.9 adds the Winsock stream counterpart. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Platform/Windows/Source/SolidSyslogWinsockResolver.c`:
- Around line 67-72: The code dereferences info->ai_addr and blindly casts
resolver->getPort() to uint16_t; add defensive checks after Winsock_getaddrinfo
succeeds: ensure info != NULL, info->ai_addr != NULL and info->ai_addrlen is at
least sizeof(struct sockaddr_in) before casting/assigning to the sockaddr_in
from SolidSyslogAddress_AsSockaddrIn(result); validate resolver->getPort() is
within 0..65535 before calling htons((uint16_t) resolver->getPort()) and handle
out-of-range or NULL cases by cleaning up with Winsock_freeaddrinfo(info) and
returning/propagating a failure code or logging an error instead of proceeding;
use the existing symbols Winsock_getaddrinfo, info, info->ai_addr,
info->ai_addrlen, SolidSyslogAddress_AsSockaddrIn, resolver->getPort and
Winsock_freeaddrinfo to locate where to add these checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68a05b44-95e8-4bad-a8af-148d0a42e3d8
📒 Files selected for processing (2)
Platform/Windows/Source/SolidSyslogWinsockDatagram.cPlatform/Windows/Source/SolidSyslogWinsockResolver.c
Comment on lines
+67
to
+72
| if (Winsock_getaddrinfo(resolver->getHost(), NULL, &hints, &info) == GETADDRINFO_SUCCESS) | ||
| { | ||
| struct sockaddr_in* sin = SolidSyslogAddress_AsSockaddrIn(result); | ||
| *sin = *(struct sockaddr_in*) info->ai_addr; | ||
| sin->sin_port = htons((uint16_t) resolver->getPort()); | ||
| Winsock_freeaddrinfo(info); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether resolver tests already cover malformed addrinfo + out-of-range ports.
rg -nP --type=cpp -C3 'SolidSyslogWinsockResolverTest|ai_addrlen|ai_addr|getPort|65535|negative|invalid port' Tests
rg -nP --type=c -C3 'Winsock_getaddrinfo|ai_addrlen|htons\(|getPort\(' Platform/Windows/Source/SolidSyslogWinsockResolver.cRepository: DavidCozens/solid-syslog
Length of output: 8774
🏁 Script executed:
fd -i winsock.*resolver.*test Tests/Repository: DavidCozens/solid-syslog
Length of output: 108
🏁 Script executed:
find Tests -name '*Winsock*Resolver*' -type fRepository: DavidCozens/solid-syslog
Length of output: 108
🏁 Script executed:
find Tests -name '*Resolver*' -type f | head -20Repository: DavidCozens/solid-syslog
Length of output: 153
🏁 Script executed:
cat -n Tests/SolidSyslogWinsockResolverTest.cppRepository: DavidCozens/solid-syslog
Length of output: 5488
🏁 Script executed:
find Tests -name 'WinsockFake*' -type fRepository: DavidCozens/solid-syslog
Length of output: 149
🏁 Script executed:
cat -n Tests/Support/WinsockFake.c | head -100Repository: DavidCozens/solid-syslog
Length of output: 3326
🏁 Script executed:
cat -n Tests/Support/WinsockFake.c | tail -n +100Repository: DavidCozens/solid-syslog
Length of output: 4753
🏁 Script executed:
grep -n "SetGetAddrInfo\|fakeAddrInfo\|ai_addr\|ai_addrlen" Tests/Support/WinsockFake.cRepository: DavidCozens/solid-syslog
Length of output: 437
🏁 Script executed:
grep -n "getPort\|getHost" Platform/Windows/Source/SolidSyslogWinsockResolver.cRepository: DavidCozens/solid-syslog
Length of output: 577
🏁 Script executed:
grep -n "int getPort\|const char.*getHost" Interface/SolidSyslogResolverDefinition.hRepository: DavidCozens/solid-syslog
Length of output: 50
🏁 Script executed:
find . -name 'SolidSyslogResolverDefinition.h' -type fRepository: DavidCozens/solid-syslog
Length of output: 111
🏁 Script executed:
cat -n Interface/SolidSyslogResolverDefinition.hRepository: DavidCozens/solid-syslog
Length of output: 579
🏁 Script executed:
cat -n Platform/POSIX/Source/SolidSyslogGetAddrInfoResolver.cRepository: DavidCozens/solid-syslog
Length of output: 154
🏁 Script executed:
find . -name '*GetAddrInfoResolver*' -type fRepository: DavidCozens/solid-syslog
Length of output: 231
🏁 Script executed:
fd -i getaddrinfo.*resolver Platform/Repository: DavidCozens/solid-syslog
Length of output: 180
🏁 Script executed:
cat -n Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.cRepository: DavidCozens/solid-syslog
Length of output: 2696
🏁 Script executed:
cat -n Tests/SolidSyslogGetAddrInfoResolverTest.cppRepository: DavidCozens/solid-syslog
Length of output: 5483
🏁 Script executed:
grep -rn "65535\|UINT16_MAX\|port.*range\|port.*valid" Platform/Windows/Source/ Platform/Posix/Source/Repository: DavidCozens/solid-syslog
Length of output: 50
🏁 Script executed:
grep -rn "getPort\|resolver->getPort" Tests/ | head -20Repository: DavidCozens/solid-syslog
Length of output: 1752
🏁 Script executed:
cat -n Platform/Windows/Source/SolidSyslogWinsockResolver.cRepository: DavidCozens/solid-syslog
Length of output: 3743
🏁 Script executed:
grep -rn "ai_addr\|ai_addrlen" Platform/ Interface/ | grep -v ".o\|Binary"Repository: DavidCozens/solid-syslog
Length of output: 50
🏁 Script executed:
rg -n "uint16_t.*port|port.*uint16_t|htons" Platform/Windows/Source/ Platform/Posix/Source/Repository: DavidCozens/solid-syslog
Length of output: 327
Validate resolver response and port range bounds.
At lines 70–71, the code dereferences info->ai_addr without NULL validation and casts getPort() to uint16_t without range checking. Negative ports cause undefined behavior; ports exceeding 65535 silently wrap. Neither scenario is tested in the current suite, and the WinsockFake seam always returns well-formed responses, masking the gap.
Add bounds validation for port ∈ [0, 65535] and defensive checks for info, info->ai_addr, and info->ai_addrlen before use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Platform/Windows/Source/SolidSyslogWinsockResolver.c` around lines 67 - 72,
The code dereferences info->ai_addr and blindly casts resolver->getPort() to
uint16_t; add defensive checks after Winsock_getaddrinfo succeeds: ensure info
!= NULL, info->ai_addr != NULL and info->ai_addrlen is at least sizeof(struct
sockaddr_in) before casting/assigning to the sockaddr_in from
SolidSyslogAddress_AsSockaddrIn(result); validate resolver->getPort() is within
0..65535 before calling htons((uint16_t) resolver->getPort()) and handle
out-of-range or NULL cases by cleaning up with Winsock_freeaddrinfo(info) and
returning/propagating a failure code or logging an error instead of proceeding;
use the existing symbols Winsock_getaddrinfo, info, info->ai_addr,
info->ai_addrlen, SolidSyslogAddress_AsSockaddrIn, resolver->getPort and
Winsock_freeaddrinfo to locate where to add these checks.
Owner
Author
There was a problem hiding this comment.
Leaving as-is, intentionally. This resolver is a byte-for-byte mirror of Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.c — same *(struct sockaddr_in*) info->ai_addr dereference and same unchecked (uint16_t) getPort() cast. The resolver deliberately trusts its injected getHost / getPort callbacks (SRP); range validation belongs at the config layer. Fixing only the Windows side would break symmetry with the POSIX mirror. If we want to add port-range + ai_addr NULL checks, that's a separate, coordinated change across both resolvers — not scope for S13.4.
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — the symmetry with the POSIX resolver and the SRP argument are clear. I'll record this so the same observation isn't raised again on either resolver.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 149
File: Platform/Windows/Source/SolidSyslogWinsockResolver.c:67-72
Timestamp: 2026-04-17T20:07:36.721Z
Learning: In `Platform/Windows/Source/SolidSyslogWinsockResolver.c` (and its POSIX mirror `Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.c`), the dereference of `info->ai_addr` without NULL/addrlen validation, and the unchecked cast of `getPort()` to `uint16_t` before `htons()`, are intentional. The resolver trusts its injected `getHost`/`getPort` callbacks by design (SRP); port-range and `ai_addr` defensive checks belong at the config/caller layer. Any future hardening must be a coordinated change across both resolvers together. Do not flag these as bugs or missing validation in future reviews of either resolver file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 137
File: Interface/SolidSyslogResolverDefinition.h:12-12
Timestamp: 2026-04-15T21:12:31.145Z
Learning: In `Interface/SolidSyslogResolverDefinition.h` (and the corresponding `Source/SolidSyslogResolver.c`), the `Resolve` function pointer returns `void` intentionally. This is a direct extraction of the pre-existing behaviour in `SolidSyslogUdpSender` and `SolidSyslogTcpSender`, neither of which checked resolution success before PR `#137`. Adding a boolean/error return to `Resolve` and propagating failures through sender create/connect paths is deferred to the error-handling epic (Epic `#31`). Do not flag the `void` return on `SolidSyslogResolver.Resolve` as a contract gap in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 137
File: Source/SolidSyslogUdpSender.c:0-0
Timestamp: 2026-04-15T21:12:56.960Z
Learning: In `Source/SolidSyslogUdpSender.c`, NULL checks for `config` and `config->resolver` in `SolidSyslogUdpSender_Create` are intentionally deferred to the error-handling epic (Epic `#31`). The function currently calls `SolidSyslogResolver_Resolve()` dispatch (not the vtable directly) and includes `SolidSyslogResolver.h`. Do not flag missing NULL guards on config/resolver in this function in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-17T19:45:13.648Z
Learning: In `DavidCozens/solid-syslog`, the test helper methods `Resolve()` and `Result()` in `Tests/SolidSyslogWinsockResolverTest.cpp` intentionally use PascalCase to mirror the convention in the POSIX counterpart `Tests/SolidSyslogGetAddrInfoResolverTest.cpp`. Do not flag PascalCase test helper methods in Winsock or POSIX resolver tests in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-17T19:45:13.648Z
Learning: In `DavidCozens/solid-syslog`, the extern function-pointer seam variables in `Platform/Windows/Source/SolidSyslogWinsockDatagramInternal.h` and `Platform/Windows/Source/SolidSyslogWinsockResolverInternal.h` (`Winsock_socket`, `Winsock_sendto`, `Winsock_closesocket`, `Winsock_getaddrinfo`, `Winsock_freeaddrinfo`) intentionally use underscore-based naming (mirroring the real Winsock API names) to preserve visual clarity at `UT_PTR_SET` call sites. Do not flag these names as non-camelCase in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 109
File: Example/Common/ExampleCommandLine.c:83-88
Timestamp: 2026-04-13T09:52:59.823Z
Learning: In `Example/Common/ExampleCommandLine.c`, the `strtol`-without-endptr/sign-validation pattern for `OPT_MAX_FILES` and `OPT_MAX_FILE_SIZE` (around lines 83–88) is a known pre-existing deferral. The values are clamped downstream in `SolidSyslogFileStore_Create`, so there is no runtime safety issue. Parse-time validation (endptr + non-negative check, returning 1 on bad input) is backlogged for a follow-up PR after PR `#109` merges. Do not re-flag this as a new issue in future reviews of these two `case` branches.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 140
File: Source/SolidSyslogPosixTcpStream.c:31-36
Timestamp: 2026-04-17T07:50:51.607Z
Learning: In `Source/SolidSyslogPosixTcpStream.c`, `SolidSyslogPosixTcpStream_Destroy()` intentionally does not close the socket fd before nulling the vtable pointers. Through the only supported API path (TcpSender → Stream), every successful `Open` is guaranteed to be matched by a `Close` — either via `TcpSender_Destroy` (when `connected == true`), `SendData` failure recovery, or `Open`'s own connect-failure self-recovery (`OpenClosesSocketOnConnectFailure` test). Standalone "Destroy-without-Close" usage is not a supported scenario; adding defensive cleanup without a driving test would violate the project's "no untested production code" discipline. Formalising this contract (with tests) is deferred to Epic `#31` (error-handling / robustness). Do not flag missing fd cleanup in `SolidSyslogPosixTcpStream_Destroy` in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 131
File: Tests/Support/SafeStringWindows.c:5-8
Timestamp: 2026-04-15T15:53:38.455Z
Learning: In `Tests/Support/SafeStringWindows.c` of `DavidCozens/solid-syslog`, the `SafeString_Copy` Windows implementation intentionally omits a `destSize == 0` guard (unlike `SafeStringStandard.c`). The design goal is to let `strncpy_s` invoke the MSVC invalid parameter handler on misuse, acting as a fail-fast mechanism in test code. Do not flag this asymmetry as a defect in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 103
File: Tests/FileFake.c:336-339
Timestamp: 2026-04-11T21:59:17.489Z
Learning: In `Tests/FileFake.c`, the bounds checks `(fake->position + count) <= fake->active->fileSize` and `(fake->position + count) <= FILEFAKE_MAX_SIZE` do not risk `size_t` overflow in practice: `position` is bounded by `FILEFAKE_MAX_SIZE` (4096) and `count` is bounded by record overhead plus `SOLIDSYSLOG_MAX_MESSAGE_SIZE` (517). Do not flag these additions as overflow risks.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 91
File: Source/SolidSyslogTcpSender.c:57-71
Timestamp: 2026-04-09T20:59:09.391Z
Learning: In `Source/SolidSyslogTcpSender.c`, error handling for `connect()` failure and `send()` failure (including partial writes, SIGPIPE mitigation, and reconnection logic) is intentionally deferred to story S15.2. In the current implementation (S15.1), `tcp->connected` is set to `true` unconditionally after `connect()`, and `send()` return values are ignored. S15.2 will test-drive: connect failure → return false (and do NOT set `tcp->connected = true`), send failure → close socket and reconnect, and a `SendAll()` helper that retries on partial writes.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 103
File: Source/SolidSyslogFileStore.c:124-128
Timestamp: 2026-04-11T22:49:26.725Z
Learning: In `Source/SolidSyslogFileStore.c`, the return values of `OpenWriteFile()` and `OpenReadFile()` are intentionally unchecked in `SolidSyslogFileStore_Create` (around lines 124–128) and `RotateToNextFile` (around lines 394–400). Guarding against open failures and aborting the create/rotation flow is deferred to the error-handling epic, tracked in `project_robustness_backlog.md` under "FileStore error handling (PR `#103`)". Do not flag missing open-failure guards in these functions in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 91
File: Source/SolidSyslogTcpSender.c:42-50
Timestamp: 2026-04-09T20:59:05.891Z
Learning: In `Source/SolidSyslogTcpSender.c` (and the matching `SolidSyslogUdpSender`), the singleton `Create()` function does not close/clean up a previously opened file descriptor on a repeated call. This is intentional: double-`Create()` is considered a misuse of the singleton pattern, and proper guard/cleanup is deferred to a dedicated error-handling epic. Do not flag this as a bug in future reviews of these files.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 113
File: Interface/SolidSyslogFormatter.h:11-11
Timestamp: 2026-04-13T22:11:33.428Z
Learning: In `Interface/SolidSyslogFormatter.h`, `SolidSyslogFormatterStorage` is intentionally typedef'd as `size_t` (not `unsigned char`) to guarantee alignment matching `struct SolidSyslogFormatter`'s leading `size_t` fields followed by a flexible `char[]` array. The cast from `SolidSyslogFormatterStorage*` to `struct SolidSyslogFormatter*` is the standard C opaque-handle/caller-owned-storage pattern; the `_Static_assert` in `Source/SolidSyslogFormatter.c` guards layout compatibility. Do not flag this typedef or the associated cast as a strict-aliasing violation or suggest switching to `unsigned char` in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the `_CRT_SECURE_NO_WARNINGS` compile definition is a known temporary stopgap for MSVC CRT deprecation warnings in test fakes. Issue `#126` (S13.3) will introduce a `SafeString` abstraction (Windows impl wraps `_s` variants; POSIX falls through to standard functions); once test fakes migrate, this suppression is removed. Production code is already SDL-clean. Do not flag this definition as an unjustified suppression in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the MSVC suppression `/wd4200` (flexible array member) is a known stopgap. Issue `#125` (S13.2) will replace `char buffer[]` with the `char buffer[1]` pre-C99 idiom and remove this suppression. Do not flag `/wd4200` in future reviews of this file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the MSVC suppression `/wd4297` (exception thrown from `extern "C"` function) is intentional and permanent test-infrastructure design: C fakes require C linkage, and `TestAssert_Fail` throws to propagate failures through CppUTest. Issue `#125` will add an inline comment documenting this justification. Do not flag `/wd4297` as an unjustified suppression in future reviews.
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 531 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Mirror the sendto guard pattern from 55c7469: return EAI_FAIL when node or res is NULL, and when inet_pton cannot parse the node as an IPv4 literal, instead of dereferencing unconditionally. Matches the real getaddrinfo / inet_pton contract and keeps the fake honest against future tests that exercise error paths. Addresses CodeRabbit review comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 531 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
3 tasks
This was referenced Apr 26, 2026
This was referenced May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SolidSyslogResolver+SolidSyslogDatagramimplementations, parallel to the POSIXGetAddrInfoResolver/PosixDatagram. Brings up UDP syslog delivery on MSVC and validates the platform-abstraction layer put in place by S13.6 – S13.11.WinsockFaketest support library +SolidSyslogWinsockResolverTest/SolidSyslogWinsockDatagramTeston themsvc-debugpreset, using CppUTestUT_PTR_SETto swap Winsock function-pointer seams at setup (MSVC does not support the POSIX SocketFake's symbol-override approach).Platform/Windows/Interface/SolidSyslogAddress.hcompanion to the POSIX opaque-address header. Layout-compatiblesockaddr_inmeans the S13.11 static-assert still holds.Closes #128.
Design notes
#ifdef _WIN32— platform selection lives entirely in CMake.SOLIDSYSLOG_WINSOCKoption auto-detects fromcheck_symbol_exists(sendto "winsock2.h" ...). Link tows2_32and install Windows-side Interface headers when enabled.*Internal.hinPlatform/Windows/Source/:WinsockResolverownsWinsock_getaddrinfo,Winsock_freeaddrinfoWinsockDatagramownsWinsock_socket,Winsock_sendto,Winsock_closesocketWSAStartup/WSACleanup, consistent with how POSIX callers don't needsocket_init().WSAGetLastError/ TCP (S13.9) deliberately out of scope.Test plan
debug(gcc): 534 tests ✓clang-debug: 534 tests ✓sanitize(ASan + UBSan): 534 tests ✓coverage: 99.9% lines, 100% functions ✓tidy: clean ✓cppcheck: clean ✓format: clean ✓msvc-debug(windows-build-and-test): validated by CI — 13 WinsockResolver + 18 WinsockDatagram + 11 WinsockFake tests run on MSVC for the first time here🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores