feat: S3.7 walking skeleton — SolidSyslogTlsStream (OpenSSL) + TLS BDD#170
Merged
Conversation
Add syslog-ng TLS listener on port 6514 alongside existing UDP/TCP, mount the test CA/server-cert material into the syslog-ng container, scaffold a @wip tls_transport.feature for later phases to light up. - Bdd/syslog-ng/tls/ — self-signed 10-year CA + server cert (SANs include syslog-ng / localhost / 127.0.0.1), generate.sh for regen, README marking material test-only - syslog-ng-full.conf and syslog-ng.conf: +tls source on 6514 with peer-verify(optional-untrusted), +d_file_tls destination, +log route - syslog-ng-udp-only.conf: unchanged (remains the streams-offline config) - ci/docker-compose.bdd.yml and .devcontainer/docker-compose.yml: mount Bdd/syslog-ng/tls read-only into syslog-ng container; devcontainer exposes 6514 on host - Bdd/features/tls_transport.feature: placeholder @tls @Buffered @wip Existing 15 BDD features / 32 scenarios / 168 steps still green; new @wip feature correctly skipped.
Adds a Read operation to the SolidSyslogStream vtable and
implementation in PosixTcpStream. Needed so the upcoming TLS Stream
(Platform/OpenSsl/...) can delegate BIO reads to an injected
transport Stream — decoupling OpenSSL from POSIX-specific I/O.
TDD cycles (red → green → refactor):
- A1: SocketFake gains recv() link-time intercept + call counter.
First test in a new SocketFakeTest.cpp (previously SocketFake
had no unit test file — now follows the project convention).
- A2: SolidSyslogStream_Read public fn + vtable slot + dispatcher.
PosixTcpStream implements Read via recv(). Test-order fix:
new ReadCallsRecvOnce closes the stream to avoid leaking state
into the next test (pre-existing Destroy-doesn't-reset-fd
latent bug exposed; not in scope for this cycle).
- A3: Drive the Read return value — add SocketFake_SetRecvReturn
and assert PosixTcpStream's Read propagates the recv return.
Public surface:
ssize_t SolidSyslogStream_Read(stream, buffer, size);
Return value contract: bytes read, 0 on EOF, negative on error.
Matches POSIX recv() and OpenSSL's custom BIO read callback.
Only one existing Stream implementation (PosixTcpStream) — nothing
else needed wiring. No callers use Read yet; it's infrastructure.
Tests: 589 (was 586). All passing. Full build clean (library,
examples, tests, example tests).
Strengthens Series A along three dimensions raised in review: 1. SocketFake.recv now captures args (fd, buf, len, flags) not just a call counter. 4 new arg-capture accessors + 4 new SocketFake tests exercising each. Brings recv into line with the existing send/sendto arg-capture pattern. 2. PosixTcpStream Read tests extended — one assertion per claim: ReadCallsRecvOnce, ReadPassesSocketFdToRecv, ReadPassesBufferToRecv, ReadPassesLengthToRecv, ReadPassesZeroFlagsToRecv, ReadReturnsRecvReturnValue. Matches the existing Send test pattern. 3. Micro-cycle: PosixTcpStream.Destroy now resets fd to INVALID_FD. Previously Destroy left stale fd state, which only surfaced as a test-order bug when a test left the stream open (my Read tests originally did). Driven by DestroyResetsFdSoRecreatedStreamIsNotOpen. Refactor: removed the defensive Close calls from Read tests — no longer needed with proper state reset. Tests: 598 total (was 589). All green. Full build clean. Noting: Windows TCP stream (S13.9, future) will need to provide Read in its vtable. Tracked in memory; compile will catch it.
Plugs the fd leak surfaced in review: Destroy previously just reset fd to INVALID_FD, leaving the underlying socket open if the caller forgot to Close. Now Destroy calls the internal Close helper first, mirroring the UdpSender pattern where Destroy invokes Disconnect before resetting. Red driven by two new tests: - DestroyClosesOpenSocket: asserts close() called once - DestroyClosesWithSocketFd: asserts close()'s arg is the socket fd Refactor after green: removed DestroyResetsFdSoRecreatedStreamIsNotOpen (A4's test). Its intent — "recreate starts fresh" — now follows transitively from DestroyClosesOpenSocket (Destroy cleans up fd) + the existing CloseIsNoOpWhenNotOpen (fresh stream's Close is noop). Also removed the now-redundant explicit `instance.fd = INVALID_FD` from Destroy — Close() already handles it. Tests: 599 total (was 598: +2 A5 tests, -1 A4 test subsumed). All green. Full build clean.
New state-ful fake implementing SolidSyslogStream. Matches the
existing pattern of state-ful fakes (SenderFake, StoreFake,
BufferFake, FileFake — calloc-backed handle, per-instance state,
accessor functions taking the handle).
TDD cycles:
- B1: CreateSucceeds (Create/Destroy skeleton)
- B2: OpenIncrementsCount + OpenCapturesAddr (vtable Open wiring)
- B3: SendIncrementsCount + SendCapturesBuffer + SendCapturesSize
(vtable Send wiring, per-claim assertion)
- B4: ReadIncrementsCount + ReadCapturesBuffer + ReadCapturesSize +
ReadReturnsConfiguredValue (vtable Read wiring, canned return
via StreamFake_SetReadReturn)
- B5: CloseIncrementsCount (vtable Close wiring)
API:
struct SolidSyslogStream* StreamFake_Create(void);
void StreamFake_Destroy(struct SolidSyslogStream*);
int StreamFake_{Open,Send,Read,Close}CallCount(stream);
... StreamFake_Last*(stream);
void StreamFake_SetReadReturn(stream, ssize_t);
Tests: 610 total (+11 for StreamFake). All green. Full build clean.
Next (series C): new Platform/OpenSsl/SolidSyslogTlsStream that
takes a StreamFake (or any SolidSyslogStream) as its injected
transport and wires OpenSSL I/O through a custom BIO.
New Platform/OpenSsl/ tier holding the OpenSSL-based TLS stream.
Decoupled from POSIX — takes any SolidSyslogStream* as its injected
byte transport, wires OpenSSL on top.
File layout:
- Platform/OpenSsl/Interface/SolidSyslogTlsStream.h
- Platform/OpenSsl/Source/SolidSyslogTlsStream.c
- Platform/OpenSsl/CMakeLists.txt — find_package(OpenSSL REQUIRED),
compiles into libSolidSyslog.a. Headers known at compile time;
libssl linked by real consumers or replaced by OpenSslFake in tests.
- Top-level CMakeLists adds the subdirectory under SOLIDSYSLOG_POSIX
for now (will generalise when/if we want Windows OpenSSL).
TDD cycles:
- C1 CreateSucceeds — scaffold
- C2 OpenOpensTransport + OpenPassesAddressToTransport — delegate to
injected transport (StreamFake verifies via Open call count + arg)
- C3 OpenCreatesSslContext — first OpenSSL fake detour
- C4 OpenLoadsCaBundleFromConfig — caBundlePath flows from config
- C5 OpenRequiresPeerVerification — SSL_VERIFY_PEER
- C6 OpenSetsTls12Floor — SSL_CTX_ctrl via SET_MIN_PROTO_VERSION cmd
- C7 OpenCreatesSslSession + OpenPassesCtxFromCtxNewToSslNew — the
pointer-chain verification David called out: fake captures
args AND return values, tests assert plumbing is consistent.
Added OpenSslFake_LastCtxReturned + OpenSslFake_LastSslNewCtxArg.
Refactor after green: extracted CreateSslContext() helper from Open.
Tests: 629 total (was 610; +19 new). All green. Full build clean.
Next: BIO setup (SSL_set_bio replacing SSL_set_fd), SSL_connect,
hostname/SNI, then Send/Close/Destroy.
Decoupled Open now wires OpenSSL via a custom BIO (not SSL_set_fd),
setting up the seam where BIO read/write callbacks will delegate to
the injected SolidSyslogStream transport.
Cycles:
- C8 OpenCreatesBio — BIO_meth_new + BIO_new. Fake gains BIO surface:
BIO_meth_new, BIO_new, LastBioReturned accessor. Opaque bio_st /
BIO_METHOD types get static-char storage like SSL_CTX/SSL.
- C9 OpenSetsBioOnSsl + OpenPassesSslFromNewToSetBio +
OpenPassesBioFromNewToSetBio — SSL_set_bio; fake captures both
SSL* and BIO* args so pointer-chain plumbing is verifiable.
- C10 OpenPerformsHandshake + OpenPassesSslToConnect — SSL_connect;
fake captures SSL arg, default-returns success.
Refactor after C9: extracted CreateTransportBio() helper from Open.
Open now reads: delegate transport.Open → CreateSslContext →
SSL_new → CreateTransportBio → SSL_set_bio → SSL_connect.
Each OpenSSL handoff has a pointer-chain test now (CtxNew→SslNew,
SslNew→SetBio, BioNew→SetBio, SslNew→Connect).
Tests: 644 total (+15). All green. Full build clean.
Next: hostname/SNI (SSL_set1_host / SSL_set_tlsext_host_name), then
Send/Close/Destroy.
Completes the Open sequence of the decoupled TLS stream.
- C11 OpenSetsSniHostnameFromConfig — SSL_set_tlsext_host_name
(routed via SSL_ctrl + SSL_CTRL_SET_TLSEXT_HOSTNAME)
- C12 OpenSetsExpectedCertHostname +
OpenSkipsHostnameSetupWhenServerNameIsNull — SSL_set1_host,
wrapped in a null-check so "no hostname verification" is the
documented NULL-serverName mode.
- C13 OpenAttachesTransportAsBioData — BIO_set_data stores the
injected transport pointer on the BIO so callbacks can retrieve
it. Fake BIO_get_data returns last-set data.
- C14 BioReadCallbackDelegatesToTransportRead — wires
BIO_meth_set_read with TransportBioRead; test captures the
callback and invokes it directly, asserting StreamFake.Read was
called on the injected transport. This is the key proof that
OpenSSL I/O will run through our transport when libssl is real.
- C15 BioWriteCallbackDelegatesToTransportSend — mirror for write.
Also: tests verify hostname setup is skipped entirely when
config.serverName is NULL (trust-anchor-only mode).
Tests: 656 total (was 644; +12). All green. Full build clean.
Next: Send (SSL_write), Close (SSL_shutdown + SSL_free), Destroy
(SSL_CTX_free + BIO_meth_free), all with the fake-assisted pattern.
Completes the vtable: TlsStream now implements full Open/Send/Read
lifecycle. (Read is inherited from Stream interface via the injected
transport — we don't need TLS-level Read at Stream API until someone
reads through StreamSender.)
- C16 Send: SSL_write. Four tests — count, SSL arg plumbing, buffer
pointer, size. Instance now holds the SSL* from Open so Send
can reuse it.
- C17 Close: SSL_shutdown + SSL_free + transport.Close propagation.
Verifies Stream_Close on injected transport fires too.
- C18 Destroy: SSL_CTX_free. Instance stores SSL_CTX from Open;
Destroy frees with a null-guard so double-Destroy is safe.
OpenSSL fake gains: SSL_write (echoes size), SSL_shutdown, SSL_free,
SSL_CTX_free. Each with counter and per-arg captures where useful.
Tests: 672 total (was 656; +16). All green. Full build clean.
Status: SolidSyslogTlsStream is now complete at the unit level —
full lifecycle through the decoupled architecture, OpenSSL API
calls all verified via fake, BIO callbacks verifiably delegate to
the injected transport.
Next to close out S3.7:
- Wire up Example/Threaded to use PosixTcpStream as transport under
a new TLS StreamSender in addition to existing UDP/TCP
- Link real libssl into the example binary
- Remove @wip tag on tls_transport.feature and green the BDD scenario
- Final preset gauntlet + PR
Every discarded arg in the OpenSslFake has been promoted to a
captured value with an accessor; every TlsStream claim that used
to be "API X was called N times" now has a sibling test proving the
args were the handles returned by the preceding call.
Fake additions (15 new captures):
- SSL_CTX_new: method arg
- SSL_CTX_load_verify_locations: ctx arg (kept CAfile)
- SSL_CTX_set_verify: ctx arg (kept mode)
- SSL_CTX_ctrl: ctx arg (kept cmd+larg for SET_MIN_PROTO_VERSION)
- BIO_meth_set_read: method arg (kept callback)
- BIO_meth_set_write: method arg (kept callback)
- BIO_new: method arg
- BIO_set_data: bio arg (kept data)
- BIO_get_data: bio arg (proves TransportBioRead resolves the right BIO)
- SSL_set_bio: wbio arg (kept ssl + rbio)
- SSL_ctrl: ssl arg (kept SNI hostname)
- SSL_set1_host: ssl arg (kept hostname)
- SSL_shutdown: ssl arg
- SSL_free: ssl arg
- SSL_CTX_free: ctx arg
Plus BIO_meth_new return-value accessor for chain assertions.
Restructured OpenSslFake.{h,c} with per-function groupings for
captures, reset, accessors, and link-interposed functions, so
adding the next API call has a clean location.
Pointer-chain tests in SolidSyslogTlsStreamTest.cpp (15 new):
- CTX chain: ClientMethod -> CtxNew, CtxNew -> LoadVerify/SetVerify/
SetMinProto/SslNew/CtxFree
- SSL chain: SslNew -> SetBio/SslCtrl(SNI)/Set1Host/Connect/Write/
Shutdown/Free
- BIO method chain: BioMethNew -> SetRead/SetWrite/BioNew
- BIO chain: BioNew -> SetData, SetBio uses same BIO for read+write,
BIO_get_data called with same BIO from inside TransportBioRead
Arg capture tests in OpenSslFakeTest.cpp (16 new) — one per new
capture, proving the fake itself works.
Tests: 703 total (was 672; +31). All green. Full build clean.
Three small TDD cycles adding the most impactful error paths. Each
with a failure-mode switch on the relevant fake.
- E1 OpenReturnsFalseWhenHandshakeFails — SSL_connect returning
negative should propagate false from Open. Fake gains
OpenSslFake_SetConnectFails. Production Open now returns
`SSL_connect(...) > 0` instead of unconditional true.
- E2 SendReturnsFalseWhenWriteFails — SSL_write returning negative
should yield false from Send. Production already had the
correct propagation (SSL_write > 0); this cycle just added
the OpenSslFake_SetWriteFails switch + test.
- E3 OpenReturnsFalseWhenTransportOpenFails +
OpenSkipsSslSetupWhenTransportOpenFails — if the injected
transport's Open fails, bail out before any OpenSSL setup.
StreamFake gains StreamFake_SetOpenFails. Production Open
short-circuits on transport-Open failure.
Added happy-path sibling tests (OpenReturnsTrueOnHappyPath,
SendReturnsTrueOnHappyPath) so the positive assertion is explicit
next to each failure case.
Not covered at this scope — deferred to E3 hardening stories:
- SSL_CTX_new / SSL_new allocation failures (rare; handled
transitively via later OpenSSL calls that would then fail)
- SSL_CTX_load_verify_locations returning 0 (handled transitively
via handshake failure)
- Resource cleanup on mid-Open failure (ctx/ssl left dangling;
picked up by Destroy — documented as a known limitation)
Tests: 713 total (was 703; +10). All green. Full build clean.
Ends the unit-test-only phase — the Threaded example now exercises the TLS stream against real OpenSSL, and a manual smoke test against the BDD syslog-ng container verifies end-to-end delivery (message arrives in received_tls.log). Changes: - Example/Common/ExampleSwitchConfig gains EXAMPLE_SWITCH_TLS and a "tls" case in SetByName. Tests updated. - New Example/Common/ExampleTlsConfig — host "syslog-ng", port 6514 (RFC 5425), CA bundle Bdd/syslog-ng/tls/ca.pem, server name "syslog-ng". Mirrors ExampleTcpConfig shape. No dedicated test file, matching TCP/UDP config pattern (exercised via BDD). - Example/Threaded/main.c — launch-time choice between plain TCP and TLS. SolidSyslogStreamSender and SolidSyslogPosixTcpStream are singletons in this build, so we can't wire both. "--transport tls" wires UDP + TLS; anything else keeps the existing UDP + TCP. Runtime switching to the non-enabled reliable transport falls back to UDP. Documented in comment. - ExampleCommandLine accepts "tls" alongside "udp"/"tcp" (was silently rejected by strict validation). - Example/CMakeLists.txt — find_package(OpenSSL REQUIRED), link SolidSyslogThreadedExample against OpenSSL::SSL + OpenSSL::Crypto. - BIO method wiring gap caught during smoke test: real libssl's SSL_connect calls BIO_ctrl, so our custom BIO needed a ctrl callback (returns 1 for FLUSH/PUSH/POP/DUP, 0 otherwise) and a create callback (calls BIO_set_init(bio, 1)). Added via cycle P6 — TransportBioCreate + TransportBioCtrl in production, plus OpenSslFake support for BIO_meth_set_ctrl / _create / BIO_set_init and accessors for the new callback slots. Tests: 717 total (was 703; +14 for TLS CLI, switch, BIO ctrl/create). All green. Full build clean. Known limitation flagged in comment: TCP↔TLS runtime switching isn't supported because both use singleton StreamSender + singleton PosixTcpStream. Multi-instance refactor is future E3 work. Remaining to close out S3.7: - Un-@wip tls_transport.feature and green the BDD scenario - Full preset gauntlet (debug, clang-debug, sanitize, coverage, tidy, cppcheck, format, bdd) - PR
Addresses a testing-as-documentation gap David flagged: the tests captured "a ctrl callback was set" and "a create callback was set" but never invoked them, so a reader of the tests couldn't tell what the callbacks actually do. Now each callback is retrieved from the fake and invoked directly, with assertions on the behaviour. Four new tests: - BioCtrlCallbackReturnsSuccessForFlush — the critical one; real libssl calls BIO_ctrl(BIO_CTRL_FLUSH) during SSL_connect, and our missing-ctrl-handler was the bug P6 fixed. - BioCtrlCallbackReturnsSuccessForPushPopDup — the other lifecycle commands our handler recognises. - BioCtrlCallbackReturnsFailureForUnknownCommand — documents the "return 0 for anything we don't know" behaviour. - BioCreateCallbackMarksBioInitialised — invokes the create callback, verifies it calls BIO_set_init(bio, 1). Fake gains LastSetInitArg so the assertion has something to check. This mirrors the existing pattern for Read/Write callbacks (BioReadCallbackDelegatesToTransportRead, etc.) — tests that actually exercise the registered callback. Tests: 721 total (was 717; +4). All green. Full build clean. BDD smoke test — TLS end-to-end still green.
- tls_transport.feature un-@wip; PER_TRANSPORT_LOG gains tls; RECEIVED_TLS_LOG exported from environment. - clang-tidy clean-ups on the OpenSslFake (parameter names aligned with OpenSSL's declarations: biom / type / a / ptr / init); NOLINT for fixed-signature OpenSSL functions where adjacent- swappable-params rule fires; NOLINT on recv for the same reason. - SocketFake recv gets the POSIX-API NOLINT pattern already used by getaddrinfo. - TransportBioCtrl gets bugprone-easily-swappable-parameters NOLINT (OpenSSL BIO_ctrl_fn contract). - Test-only DummyRead/DummyWrite/DummyCtrl/DummyCreate helpers renamed + NOLINT where relevant; comparisons switched to FUNCTIONPOINTERS_EQUAL (avoids C-style / reinterpret_cast). - cppcheck suppressions on StreamFakeTest and TlsStreamTest fixtures for the unreadVariable false-positive (CppUTest macros not modelled). - clang-analyzer: guarded BIO callback invocations with an explicit null-check early return so CHECK_TRUE doesn't leave analyser with possible-null deref. - clang-format: full-tree auto-format applied; added missing off/on markers around OpenSslFakeTest's TEST_GROUP. Gauntlet result: - debug: 721 tests pass - clang-debug: 721 tests pass - sanitize: 721 tests pass (ASan + UBSan clean) - coverage: 99.9% overall (1177/1178), TlsStream 100% - tidy: clean (with documented NOLINTs on fixed-signature APIs) - cppcheck: clean - format: clean - bdd: 16 features / 33 scenarios / 171 steps green (was 15/32/168 before S3.7; +1 TLS feature, 1 TLS scenario, 3 TLS steps)
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 16 minutes and 41 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 (11)
📝 WalkthroughWalkthroughAdds OpenSSL-backed TLS transport support: new TLS stream implementation, stream Read API, example/CLI wiring for Changes
Sequence DiagramsequenceDiagram
participant App as Threaded\nExample
participant Sender as SyslogStream\nSender
participant TLS as SolidSyslog\nTlsStream
participant SSL as OpenSSL\n(API)
participant BIO as Custom\nBIO
participant Transport as TCP\nStream
participant Server as syslog-ng\nTLS Endpoint
App->>Sender: Send message (transport=tls)
Sender->>TLS: Create(config: CA, serverName)
TLS->>SSL: SSL_CTX_new(TLS_client_method)
TLS->>SSL: SSL_CTX_load_verify_locations(CA)
TLS->>SSL: SSL_new(ctx)
TLS->>BIO: Create custom BIO (read/write/ctrl callbacks)
TLS->>SSL: SSL_set_bio(ssl, bio, bio)
TLS->>SSL: SSL_set1_host(serverName)
TLS->>Transport: Open(address)
Transport->>Server: TCP connect
TLS->>SSL: SSL_connect(handshake)
SSL->>BIO: Read/Write via BIO callbacks
BIO->>Transport: recv/send to socket
SSL-->>TLS: Handshake success
Sender->>TLS: Send(data)
TLS->>SSL: SSL_write(data)
SSL->>BIO: Write -> Transport -> Server
Sender->>TLS: Close
TLS->>SSL: SSL_shutdown
TLS->>Transport: Close socket
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 726 passed, 🙈 3 skipped) 🚧 Error MessagesCreated 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: 5
🧹 Nitpick comments (5)
Bdd/syslog-ng/tls/ca.key (1)
1-27: Remove the CA private key from committed test fixtures.The
ca.keyfile is only used bygenerate.shto sign the server certificate during fixture creation. The syslog-ng runtime requires onlyserver.key,server.pem, andca.pem—the private CA key is never loaded. Keeping it in the repository adds unnecessary secret-scanner noise and security exposure. Consider either generating it on-demand ingenerate.shor storing it outside the fixture directory that gets mounted into the runtime environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Bdd/syslog-ng/tls/ca.key` around lines 1 - 27, Remove the committed CA private key file (ca.key) from the fixtures and stop checking it into the repo: delete ca.key from version control, add it to .gitignore, and update the fixture generation flow so generate.sh creates the CA key on-demand (or writes it to a non-mounted/private location) and then produces server.key, server.pem, and ca.pem for runtime; adjust any references in generate.sh and test harness code that previously expected ca.key so they use the generated ephemeral CA key path or the external/private storage location instead.Example/Common/ExampleTlsConfig.h (1)
11-16: Minor:GetPortreturnsintwhile TLS default port and on-wire port are 16-bit.
ExampleTcpConfig/ExampleUdpConfiglikely follow the same pattern, so this is consistency-with-existing, not a defect. If you're aligning the example config surface in a follow-up,uint16_twould more precisely express the port domain. Deferrable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Example/Common/ExampleTlsConfig.h` around lines 11 - 16, The ExampleTlsConfig_GetPort signature returns int but port values are 16-bit; change the declaration and corresponding implementation to return uint16_t instead of int (update ExampleTlsConfig_GetPort in ExampleTlsConfig.h and its implementation), include <stdint.h> if not already present, and update any callers to accept/compare uint16_t; consider applying the same change to ExampleTcpConfig_GetPort and ExampleUdpConfig_GetPort for consistency.Platform/OpenSsl/Source/SolidSyslogTlsStream.c (1)
78-86: Optional: track BIO_METHOD cleanup for S3.8.Each
CreateTransportBio()allocates a freshBIO_METHODviaBIO_meth_newwhich is neverBIO_meth_free'd — neither inClosenor inDestroy. In the walking skeleton with a single Open this is bounded, but please ensureBIO_meth_free(plus SSL nulling inCloseand SSL/BIO_METHOD cleanup inDestroy) are on the S3.8 tightening list alongside the already-acknowledged mid-Open leaks. A BIO method is method-shape data and could be hoisted to a module-scope one-time init if reuse is cheaper than rebuilding each Open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Platform/OpenSsl/Source/SolidSyslogTlsStream.c` around lines 78 - 86, CreateTransportBio currently allocates a BIO_METHOD with BIO_meth_new and never frees it; update the module to either (a) hoist the BIO_METHOD into a static module-scope variable and initialize it once (reusing the same method for every CreateTransportBio) and free it on module shutdown, or (b) call BIO_meth_free for the method you allocate when the enclosing SSL object is torn down; additionally ensure Close nulls any SSL pointer and Destroy performs full cleanup of the SSL and associated BIO_METHOD (BIO_meth_free) to eliminate the leak — refer to CreateTransportBio, BIO_meth_new, BIO_meth_free, Close and Destroy when making the changes.Tests/SolidSyslogTlsStreamTest.cpp (1)
161-187: Minor: defensive null-check pattern is inconsistent across callback tests.
BioReadCallbackDelegatesToTransportReadandBioWriteCallbackDelegatesToTransportSendbothCHECK_TRUEthen early-return on null, whereas the later callback tests (e.g.,BioReadCallbackLooksUpDataOnTheCorrectBioat lines 327-334,BioCtrlCallbackReturns*at lines 412-434,BioCreateCallbackMarksBioInitialisedat 436-441) call the function pointer directly without the guard. Since the suite already has separateOpenWiresBio*Callbacktests that assert non-null, dropping theCHECK_TRUE/ifpair from the two delegation tests would make the suite uniform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/SolidSyslogTlsStreamTest.cpp` around lines 161 - 187, Remove the redundant null-check/early-return pattern from the two tests BioReadCallbackDelegatesToTransportRead and BioWriteCallbackDelegatesToTransportSend: instead of CHECK_TRUE(...) followed by if (.. == nullptr) return, call the retrieved function pointer directly (from OpenSslFake_LastBioReadCallback and OpenSslFake_LastBioWriteCallback) with OpenSslFake_LastBioReturned() and the test buffers/messages and then assert StreamFake_ReadCallCount(transport) / StreamFake_SendCallCount(transport); this makes these tests consistent with the other callback tests that assume the OpenWires/OpenSsl fake callback presence.Tests/Support/OpenSslFake.c (1)
489-499: SharedlastSetDataArgmeans multi-BIO tests will alias.
BIO_set_dataandBIO_get_datashare a singlelastSetDataArgglobal rather than keying off theBIO*argument. For the current walking-skeleton (one read BIO, one write BIO, both fed the same transport pointer) this is fine, but any future test that creates two BIOs with different transport payloads and round-trips them throughBIO_get_datawill observe whichever pointer was set last. Worth tracking if S3.8 introduces per-BIO state — otherwise the read/write callback dispatch tests could quietly pass against the wrong transport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Support/OpenSslFake.c` around lines 489 - 499, The test fake currently stores a single global lastSetDataArg shared across all BIOs, causing different BIO instances to alias; change BIO_set_data and BIO_get_data to store/retrieve the data keyed by the BIO* (e.g., add a static mapping from BIO* to void* or a small array of pairs) so each BIO retains its own payload; update BIO_set_data to set map[a]=ptr and BIO_get_data to return map[a] (you can still update lastSetDataBioArg for debugging but remove reliance on lastSetDataArg as the single source of truth); ensure any allocation or entries are cleaned up when BIOs are freed if a lifecycle function exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Bdd/features/tls_transport.feature`:
- Around line 6-9: The scenario "Message delivered over TLS" currently uses the
generic step Then syslog-ng receives a message with priority "134"; replace that
assertion with the TLS-specific receive step that checks the transport-scoped
log (e.g. assert against received_tls.log) so the test verifies the TLS
listener/output mapping rather than the aggregate log. Locate the scenario by
its title and swap the final step to the transport-scoped receive assertion
referencing received_tls.log or the TLS-specific step name used elsewhere in the
feature suite.
In `@CMakeLists.txt`:
- Around line 87-90: The CMakeLists currently always adds Platform/OpenSsl when
SOLIDSYSLOG_POSIX is set; change this to be conditional on an explicit
TLS/OpenSSL option so OpenSSL is optional. Add a CMake option (e.g.
SOLIDSYSLOG_USE_OPENSSL or SOLIDSYSLOG_TLS) defaulting to OFF, and change the
add_subdirectory call for Platform/OpenSsl to run only when that option is ON
(while still keeping Platform/Posix unconditionally added when SOLIDSYSLOG_POSIX
is true); update any references/targets that depend on OpenSsl to check the new
option before linking or enabling TLS-specific code.
In `@Interface/SolidSyslogStream.h`:
- Around line 8-16: The header declares SolidSyslogStream_Read returning ssize_t
which is POSIX-only and breaks MSVC; update the header to make the interface
portable by adding platform conditional handling: either wrap the ssize_t-using
declarations (e.g., SolidSyslogStream_Read) in an `#ifdef` POSIX guard, or add a
portable signed-size typedef when compiling on Windows (include <BaseTsd.h> and
typedef SSIZE_T ssize_t or define a project-specific portable type like
SolidSyslog_ssize_t and use it in SolidSyslogStream_Read), and ensure the change
is applied to the declarations for SolidSyslogStream_Read (and any other uses of
ssize_t) while keeping SolidSyslogStream_Open and SolidSyslogStream_Send
unchanged.
In `@Interface/SolidSyslogStreamDefinition.h`:
- Line 12: Add a TLS-specific Read implementation and wire it into the TLS
stream vtable: implement a function like SolidSyslogTlsStream_Read(struct
SolidSyslogStream* self, void* buffer, size_t size) that delegates to the TLS
socket recv/read routine used by the TLS stream, then set instance.base.Read =
SolidSyslogTlsStream_Read inside SolidSyslogTlsStream_Create() (following the
same pattern as SolidSyslogPosixTcpStream and other stream types) so
SolidSyslogStream_Read() will not dereference NULL.
In `@Tests/CMakeLists.txt`:
- Around line 63-65: TEST_SOURCES is missing the OpenSslFake implementation file
which causes linker errors for tests that call OpenSslFake_*; add
Support/OpenSslFake.c to the TEST_SOURCES list alongside
SolidSyslogTlsStreamTest.cpp and OpenSslFakeTest.cpp (following the same pattern
used for FileFake.c and StreamFake.c) so the OpenSslFake.h declarations have
their corresponding definitions at link time.
---
Nitpick comments:
In `@Bdd/syslog-ng/tls/ca.key`:
- Around line 1-27: Remove the committed CA private key file (ca.key) from the
fixtures and stop checking it into the repo: delete ca.key from version control,
add it to .gitignore, and update the fixture generation flow so generate.sh
creates the CA key on-demand (or writes it to a non-mounted/private location)
and then produces server.key, server.pem, and ca.pem for runtime; adjust any
references in generate.sh and test harness code that previously expected ca.key
so they use the generated ephemeral CA key path or the external/private storage
location instead.
In `@Example/Common/ExampleTlsConfig.h`:
- Around line 11-16: The ExampleTlsConfig_GetPort signature returns int but port
values are 16-bit; change the declaration and corresponding implementation to
return uint16_t instead of int (update ExampleTlsConfig_GetPort in
ExampleTlsConfig.h and its implementation), include <stdint.h> if not already
present, and update any callers to accept/compare uint16_t; consider applying
the same change to ExampleTcpConfig_GetPort and ExampleUdpConfig_GetPort for
consistency.
In `@Platform/OpenSsl/Source/SolidSyslogTlsStream.c`:
- Around line 78-86: CreateTransportBio currently allocates a BIO_METHOD with
BIO_meth_new and never frees it; update the module to either (a) hoist the
BIO_METHOD into a static module-scope variable and initialize it once (reusing
the same method for every CreateTransportBio) and free it on module shutdown, or
(b) call BIO_meth_free for the method you allocate when the enclosing SSL object
is torn down; additionally ensure Close nulls any SSL pointer and Destroy
performs full cleanup of the SSL and associated BIO_METHOD (BIO_meth_free) to
eliminate the leak — refer to CreateTransportBio, BIO_meth_new, BIO_meth_free,
Close and Destroy when making the changes.
In `@Tests/SolidSyslogTlsStreamTest.cpp`:
- Around line 161-187: Remove the redundant null-check/early-return pattern from
the two tests BioReadCallbackDelegatesToTransportRead and
BioWriteCallbackDelegatesToTransportSend: instead of CHECK_TRUE(...) followed by
if (.. == nullptr) return, call the retrieved function pointer directly (from
OpenSslFake_LastBioReadCallback and OpenSslFake_LastBioWriteCallback) with
OpenSslFake_LastBioReturned() and the test buffers/messages and then assert
StreamFake_ReadCallCount(transport) / StreamFake_SendCallCount(transport); this
makes these tests consistent with the other callback tests that assume the
OpenWires/OpenSsl fake callback presence.
In `@Tests/Support/OpenSslFake.c`:
- Around line 489-499: The test fake currently stores a single global
lastSetDataArg shared across all BIOs, causing different BIO instances to alias;
change BIO_set_data and BIO_get_data to store/retrieve the data keyed by the
BIO* (e.g., add a static mapping from BIO* to void* or a small array of pairs)
so each BIO retains its own payload; update BIO_set_data to set map[a]=ptr and
BIO_get_data to return map[a] (you can still update lastSetDataBioArg for
debugging but remove reliance on lastSetDataArg as the single source of truth);
ensure any allocation or entries are cleaned up when BIOs are freed if a
lifecycle function exists.
🪄 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: 6e87e028-527a-49d0-88d5-dcf22d061bdf
⛔ Files ignored due to path filters (2)
Bdd/syslog-ng/tls/ca.pemis excluded by!**/*.pemBdd/syslog-ng/tls/server.pemis excluded by!**/*.pem
📒 Files selected for processing (42)
.devcontainer/docker-compose.ymlBdd/features/environment.pyBdd/features/steps/syslog_steps.pyBdd/features/tls_transport.featureBdd/syslog-ng/syslog-ng-full.confBdd/syslog-ng/syslog-ng.confBdd/syslog-ng/tls/README.mdBdd/syslog-ng/tls/ca.keyBdd/syslog-ng/tls/generate.shBdd/syslog-ng/tls/server.keyCMakeLists.txtExample/CMakeLists.txtExample/Common/ExampleCommandLine.cExample/Common/ExampleSwitchConfig.cExample/Common/ExampleSwitchConfig.hExample/Common/ExampleTlsConfig.cExample/Common/ExampleTlsConfig.hExample/Threaded/main.cInterface/SolidSyslogStream.hInterface/SolidSyslogStreamDefinition.hPlatform/OpenSsl/CMakeLists.txtPlatform/OpenSsl/Interface/SolidSyslogTlsStream.hPlatform/OpenSsl/Source/SolidSyslogTlsStream.cPlatform/Posix/Source/SolidSyslogAddressInternal.hPlatform/Posix/Source/SolidSyslogPosixTcpStream.cSource/SolidSyslogStream.cTests/CMakeLists.txtTests/Example/ExampleCommandLineTest.cppTests/Example/ExampleSwitchConfigTest.cppTests/OpenSslFakeTest.cppTests/SocketFakeTest.cppTests/SolidSyslogPosixTcpStreamTest.cppTests/SolidSyslogTlsStreamTest.cppTests/StreamFake.cTests/StreamFake.hTests/StreamFakeTest.cppTests/Support/CMakeLists.txtTests/Support/OpenSslFake.cTests/Support/OpenSslFake.hTests/Support/SocketFake.cTests/Support/SocketFake.hci/docker-compose.bdd.yml
Resolves 4 of the 5 CodeRabbit issues on PR #170 (the 5th is a false positive — OpenSslFake.c is already compiled via the PosixFakes static lib, replied on-thread). 1. SolidSyslogSsize typedef (portable replacement for ssize_t in public API). intptr_t-backed, MISRA-friendly stdint type, compiles on MSVC where POSIX ssize_t isn't available. Stream interface + StreamFake fake API + call sites updated. Internal Platform/Posix code still uses ssize_t and casts at the API boundary. 2. SOLIDSYSLOG_OPENSSL CMake option gating the OpenSSL-based TLS subsystem. Default ON when OpenSSL is discoverable; override -DSOLIDSYSLOG_OPENSSL=OFF for a UDP/TCP-only build. Platform/OpenSsl, OpenSslFake, TLS unit tests, ExampleTlsConfig, and example's libssl linkage are all gated on this option. Verified both configurations build and pass: OpenSSL on = 725 tests, OpenSSL off = 612 tests (TLS tests correctly dropped, no libssl in example's ldd). Known short-term sharp edge: main.c retains three #ifdef SOLIDSYSLOG_HAVE_OPENSSL sites guarding TLS wiring; follow-up S3.13 (#171) replaces these with per-backend CMake-selected factory files (ExampleTlsSender_OpenSsl.c / _WolfSsl.c / _Unavailable.c). 3. Wire Read in SolidSyslogTlsStream vtable. Critical bug: vtable slot was uninitialised, any future caller of SolidSyslogStream_Read on a TLS stream would have crashed. Read delegates to SSL_read. OpenSslFake gains SSL_read stub with count + arg captures. 4 new TlsStream tests covering the call, pointer-chain (ssl from new), buffer pass-through, size pass-through. 4. Tighten tls_transport.feature scenario to assert the transport-specific log (received_tls.log) via the existing "over tls" step, not just the aggregate log. Scenario now proves both TLS listener routing AND the priority. Local gauntlet all green: - debug + clang-debug + sanitize: 725 tests pass (OpenSSL on) - debug (OpenSSL off): 612 tests pass, no libssl linkage - coverage: 99.9% overall (1181/1182 lines), 100% functions - tidy: clean - cppcheck: clean - format: clean - bdd: 16 features / 33 scenarios / 172 steps green
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 730 passed, 🙈 3 skipped) 🚧 Error MessagesCreated by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Windows CI failure root-cause: vcpkg provides OpenSSL on windows-latest runners, so find_package(OpenSSL QUIET) sets OpenSSL_FOUND=TRUE and SOLIDSYSLOG_OPENSSL defaults ON. TlsStream.c, OpenSslFake.c, and the two TLS-flavoured test files all then need the OpenSSL headers. On POSIX this works implicitly because /usr/include is on the compiler search path. On Windows+vcpkg, OpenSSL headers live under the vcpkg install tree and must be declared explicitly for each target that includes <openssl/ssl.h>. Fix: in Tests/Support/CMakeLists.txt, add OPENSSL_INCLUDE_DIR as a PUBLIC include on the PosixFakes target when SOLIDSYSLOG_OPENSSL is ON. OpenSslFake.c (which lives in PosixFakes) gets it directly; any test that links PosixFakes inherits it transitively, so SolidSyslogTlsStream Test.cpp and OpenSslFakeTest.cpp also find the headers. Linux side: 725 tests still green, example still links real libssl.
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 730 passed, 🙈 3 skipped) 🚧 Error MessagesCreated by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Refactoring the previous Windows fix: OpenSslFake was stacked inside
PosixFakes, and the OpenSSL include-path was added under the POSIX
block. That's the wrong layering — OpenSSL is orthogonal to platform,
and the OpenSSL fake has no POSIX dependencies.
Split into a dedicated OpenSslFakes library gated on SOLIDSYSLOG_OPENSSL:
Tests/Support/CMakeLists.txt:
if(SOLIDSYSLOG_POSIX) -> PosixFakes (Clock/Safe/Socket)
if(SOLIDSYSLOG_WINSOCK) -> WinsockFakes
if(SOLIDSYSLOG_OPENSSL) -> OpenSslFakes (OpenSslFake, carries OpenSSL
headers via PUBLIC include)
Tests/CMakeLists.txt:
if(SOLIDSYSLOG_OPENSSL) target_link_libraries(... OpenSslFakes)
Each fake library is now scoped to its concern. Consumers link only what
they need. Verified:
- OpenSSL on: 725 tests green
- OpenSSL off: 612 tests green (no OpenSslFakes target created)
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 730 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)
CMakeLists.txt (1)
89-100:⚠️ Potential issue | 🟡 MinorAdd fail-fast guard when TLS is explicitly enabled without OpenSSL.
If
-DSOLIDSYSLOG_OPENSSL=ONis passed butfind_package(OpenSSL QUIET)fails, the build entersPlatform/OpenSsland later fails on${OPENSSL_INCLUDE_DIR}or missingOpenSSL::SSLtargets with a confusing error.Suggested fix
find_package(OpenSSL QUIET) option(SOLIDSYSLOG_OPENSSL "Include OpenSSL-based TLS support" ${OpenSSL_FOUND}) +if(SOLIDSYSLOG_OPENSSL AND NOT OpenSSL_FOUND) + message(FATAL_ERROR "SOLIDSYSLOG_OPENSSL=ON requires OpenSSL; install OpenSSL or pass -DSOLIDSYSLOG_OPENSSL=OFF") +endif() add_subdirectory(Source)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 89 - 100, The CMake logic allows add_subdirectory(Platform/OpenSsl) when SOLIDSYSLOG_OPENSSL is ON even if find_package(OpenSSL QUIET) failed, causing confusing missing-variable/target errors; add a fail-fast guard after find_package(OpenSSL QUIET) that checks if SOLIDSYSLOG_OPENSSL is enabled but OpenSSL_FOUND is false (i.e., if(SOLIDSYSLOG_OPENSSL AND NOT OpenSSL_FOUND)) and emit a clear CMake error (message(FATAL_ERROR ...)) explaining OpenSSL was requested but not found so the build stops before entering Platform/OpenSsl.
🤖 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/OpenSsl/Source/SolidSyslogTlsStream.c`:
- Around line 58-62: Check the return values of
SSL_set_tlsext_host_name(stream->ssl, stream->config.serverName) and
SSL_set1_host(stream->ssl, stream->config.serverName) and fail the
open/connection if either returns an error; do not proceed to SSL_connect() when
either call fails. In the function that prepares/opens the TLS stream (where
stream->config.serverName is used with stream->ssl), detect non-success returns
from SSL_set_tlsext_host_name and SSL_set1_host, log/propagate an Open failure,
clean up any allocated/Open resources for the stream, and return an error so the
handshake cannot proceed without hostname/SNI verification.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 89-100: The CMake logic allows add_subdirectory(Platform/OpenSsl)
when SOLIDSYSLOG_OPENSSL is ON even if find_package(OpenSSL QUIET) failed,
causing confusing missing-variable/target errors; add a fail-fast guard after
find_package(OpenSSL QUIET) that checks if SOLIDSYSLOG_OPENSSL is enabled but
OpenSSL_FOUND is false (i.e., if(SOLIDSYSLOG_OPENSSL AND NOT OpenSSL_FOUND)) and
emit a clear CMake error (message(FATAL_ERROR ...)) explaining OpenSSL was
requested but not found so the build stops before entering Platform/OpenSsl.
🪄 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: 5b1dc334-4cc5-4e0c-93c1-be2b9b3a4215
📒 Files selected for processing (20)
Bdd/features/tls_transport.featureCMakeLists.txtExample/CMakeLists.txtExample/Threaded/main.cInterface/SolidSyslogStream.hInterface/SolidSyslogStreamDefinition.hPlatform/OpenSsl/CMakeLists.txtPlatform/OpenSsl/Source/SolidSyslogTlsStream.cPlatform/Posix/Source/SolidSyslogPosixTcpStream.cSource/SolidSyslogStream.cTests/CMakeLists.txtTests/SolidSyslogPosixTcpStreamTest.cppTests/SolidSyslogTlsStreamTest.cppTests/StreamFake.cTests/StreamFake.hTests/StreamFakeTest.cppTests/Support/CMakeLists.txtTests/Support/OpenSslFake.cTests/Support/OpenSslFake.hTests/Support/SocketFake.c
✅ Files skipped from review due to trivial changes (4)
- Tests/SolidSyslogPosixTcpStreamTest.cpp
- Bdd/features/tls_transport.feature
- Tests/SolidSyslogTlsStreamTest.cpp
- Tests/StreamFake.c
🚧 Files skipped from review as they are similar to previous changes (12)
- Platform/Posix/Source/SolidSyslogPosixTcpStream.c
- Interface/SolidSyslogStreamDefinition.h
- Tests/Support/CMakeLists.txt
- Example/CMakeLists.txt
- Platform/OpenSsl/CMakeLists.txt
- Tests/CMakeLists.txt
- Source/SolidSyslogStream.c
- Tests/StreamFakeTest.cpp
- Example/Threaded/main.c
- Interface/SolidSyslogStream.h
- Tests/Support/OpenSslFake.c
- Tests/Support/OpenSslFake.h
Windows CI failure: on windows-latest, SOLIDSYSLOG_POSIX=OFF but SOLIDSYSLOG_OPENSSL=ON (vcpkg provides OpenSSL). TlsStream.c is compiled into the library and references SolidSyslogStream_Open/Send/ Read/Close from the Stream dispatcher. The dispatcher was gated on SOLIDSYSLOG_POSIX only, so on Windows the library had unresolved externals. SolidSyslogStream.c is platform-agnostic (pure vtable dispatch). Needed by any target that implements or consumes the Stream interface — today that's PosixTcpStream (POSIX), TlsStream (OPENSSL), and the StreamFake test fake. Gate: POSIX OR OPENSSL. StreamSender.c kept under POSIX-only for this PR — it isn't triggered by the Windows+OpenSSL build (no Winsock StreamSender consumers yet, that's S13.9/S13.10). Linux: 725 tests still green. Candidate fix for Windows — needs confirmation on a Windows build.
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 730 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
- CMake: FATAL_ERROR when SOLIDSYSLOG_OPENSSL=ON but OpenSSL not found
(was: silent fall-through to confusing downstream errors).
- Example{Tls,Tcp,Udp}Config_GetPort now returns uint16_t instead of int,
matching SolidSyslogEndpoint::port domain; redundant uint16_t casts at
call sites removed.
Verified locally: debug (725 tests), debug -DSOLIDSYSLOG_OPENSSL=OFF
(612 tests), tidy, cppcheck, format all green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai — follow-ups on the five nitpicks from your first review and the duplicate CMake comment from your later review:
Also addressed in 0e3f306:
Verified locally: debug (725 tests), debug |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
CMake fail-fast guard and hostname verification return values deferral both align with the discussed approach. Verified test counts (725 / 612) look solid. ✏️ Learnings added
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 100% successful (✔️ 730 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
DavidCozens
added a commit
that referenced
this pull request
Apr 22, 2026
…le cleanup (#183) * test: S03.08 in-process TLS integration harness with real libssl Adds a new OpenSslIntegrationTests executable that links real libssl (instead of OpenSslFake) and exercises SolidSyslogTlsStream end-to-end via an in-process BIO pair. This sits between unit tests (fast, fake-backed) and BDD (end-to-end via syslog-ng) in the testing pyramid, and will carry the SL4 failure-mode coverage in subsequent commits (expired cert, wrong CN/SAN, wrong CA, unsupported cipher). Harness components: - BioPairStream: SolidSyslogStream adapter over one side of a BIO pair with a cooperative pump callback, so SolidSyslogTlsStream's synchronous SSL_connect path works unchanged over non-blocking in-memory BIOs. - TlsTestCert: programmatic X509 cert generation (OpenSSL 3.0 EVP_RSA_gen), parameterised by CN, SAN DNS names, validity window, and optional issuer for self-signed or chain scenarios. - TlsTestServer: server-side SSL_CTX + SSL + BIO pair; Pump() drives SSL_accept one step at a time as the client-side handshake progresses. Smoke test HandshakeSucceedsAgainstTrustedServerCert verifies all four pieces wire together against a self-signed localhost cert. CI: new openssl-integration job runs in parallel with build-and-test, self-contained (own configure + target build), quality-monitor entry added. Clang and sanitize presets deliberately skip the integration suite — unit tests remain the coverage engine; integration is supplementary evidence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: S03.08 integration tests for TLS handshake rejection paths Three harness-based characterisation tests confirming the library's existing SSL_VERIFY_PEER plus SSL_set1_host wiring rejects the three SL4 failure modes named in the ticket acceptance: - HandshakeRejectedWhenServerCertIsExpired — cert notAfter in the past - HandshakeRejectedWhenServerCertHostnameDoesNotMatch — CN/SAN mismatch - HandshakeRejectedWhenClientDoesNotTrustServerCert — CA not in trust All three go green on first write — no production change was driven. The tests still earn their keep as regression protection: a future change that disabled SSL_VERIFY_PEER or dropped SSL_set1_host would turn them red. Setup refactor alongside: extracted buildScenario() helper from setup() so each scenario parameterises the cert config (commonName, SANs, validity window) without reconstructing the whole harness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 block handshake when SSL_set1_host fails SSL_set1_host configures the expected-hostname check that makes SSL_VERIFY_PEER reject a cert whose subject doesn't match the serverName. If set1_host fails silently (returns 0), the client still runs SSL_connect but without the hostname check — a MITM risk, since any trust-chain-valid cert from any host in the trust store would then be accepted. Return-value check now aborts Open instead. Driven by OpenReturnsFalseWhenSet1HostFails in the fake-based unit suite; OpenSslFake gains a SetSet1HostFails switch following the same pattern as SetConnectFails / SetWriteFails. Flagged in PR #170 review — first of the return-value-checking bullets in the S03.08 acceptance list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 block handshake when SNI hostname setup fails Parallel fix to the SSL_set1_host return-value check: SSL_set_tlsext_host_name (the SNI extension setter) can also fail silently. If it does, the client proceeds with SSL_connect but without advertising the intended hostname to the server — the server may then present a default certificate that the client's SSL_set1_host check accepts for a different tenant. Return-value check now aborts Open instead. Driven by OpenReturnsFalseWhenSniHostnameSetupFails; OpenSslFake gains a SetSniHostnameFails switch on the SSL_ctrl SET_TLSEXT_HOSTNAME path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 abort Open when SSL_CTX_new returns NULL Previously Open silently proceeded with a NULL SSL_CTX, which would crash on the next libssl call (SSL_CTX_load_verify_locations or SSL_CTX_set_verify deref the CTX). Under fakes this looked like a successful handshake. Either way the library was lying about the Open result. CreateSslContext now returns NULL immediately if SSL_CTX_new fails, and Open treats a NULL CTX as Open failure. Driven by OpenReturnsFalseWhenCtxNewFails; OpenSslFake gains SetCtxNewFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 abort Open when SSL_new returns NULL Mirror of the SSL_CTX_new guard: if SSL_new returns NULL, the subsequent SSL_set_bio / SSL_ctrl / SSL_set1_host / SSL_connect all deref the NULL pointer. Check the return and bail early. The SSL_CTX allocated on the line above is released by Destroy — the caller is expected to call Destroy after a failed Open, which Destroy already handles. No new cleanup needed here; a broader lifecycle sweep is scheduled for Phase 4. Driven by OpenReturnsFalseWhenSslNewFails; OpenSslFake gains SetSslNewFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 abort Open when SSL_CTX_load_verify_locations fails If the caller's CA bundle file is missing, unreadable, or malformed, libssl refuses to load any trust anchors. Continuing would make the handshake fail anyway, but failing fast surfaces the misconfiguration cleanly. First of the return-value checks that needs in-function cleanup — CreateSslContext frees the CTX it just allocated before returning NULL, so the caller doesn't have to know about the partial state. Driven by two tests: one for the false return (Open fails), one for the CTX_free call count (no leak). OpenSslFake gains SetLoadVerifyLocationsFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 abort Open when TLS 1.2 floor cannot be set SSL_CTX_set_min_proto_version is a ctrl macro that can fail if libssl rejects the version constant (build-time configuration mismatch or future-version rejection). Treating it as non-failing means the library could silently fall through to a lower protocol floor, breaking SL4 assumptions about TLS 1.2 minimum. CreateSslContext now returns NULL and frees the CTX it allocated on SET_MIN_PROTO_VERSION failure, matching the load_verify_locations path. Last of the CreateSslContext return value checks. Driven by OpenReturnsFalseWhenMinProtoVersionFails and MinProtoVersionFailureFreesCtx; OpenSslFake gains SetMinProtoVersionFails on the SSL_CTX_ctrl SET_MIN_PROTO_VERSION command. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 free BIO_METHOD on Close CreateTransportBio allocated a fresh BIO_METHOD per Open and lost the pointer after returning — a pure leak. Now the stream remembers the method pointer on the instance, and Close pairs the allocation with BIO_meth_free. Per-Open lifecycle matches the symmetry with SSL_CTX_new/SSL_free and SSL_new/SSL_free — one Open allocates a full set, one Close releases it. Driven by CloseFreesBioMethod; OpenSslFake gains a counted BIO_meth_free stub that was previously implicit (the method was never freed, so the fake didn't model the call). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 BIO_METHOD freed exactly once across Close/Destroy Destroy now covers the case where Close wasn't called (e.g. Open partially failed, or the caller's lifecycle skipped Close), releasing the BIO_METHOD the way Close would have. Close in turn nulls the stored bioMethod pointer so a subsequent Destroy won't double-free. Two tests pin the contract: DestroyFreesBioMethodWhenCloseNotCalled and DestroyAfterCloseDoesNotDoubleFreeBioMethod. Together they say: BIO_METHOD is freed once, whether the lifecycle ended at Close, ended at Destroy, or went through both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 SSL freed exactly once across Close/Destroy Mirror of the BIO_METHOD idempotence commit for SSL: Destroy now releases the SSL handle if Close wasn't called (e.g. Open failed after SSL_new but before a successful connect). Close nulls the stored pointer so a subsequent Destroy doesn't double-free. Previously Destroy only handled SSL_CTX — any caller that hit an Open failure after SSL_new was leaking an SSL handle until process exit. Not caught under fakes (SSL_free is a stub), real libssl under valgrind would flag it. Driven by DestroyFreesSslWhenCloseNotCalled and DestroyAfterCloseDoesNotDoubleFreeSsl. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 abort Open when BIO_meth_new returns NULL Under memory pressure BIO_meth_new can return NULL. Continuing with a NULL BIO_METHOD would crash the subsequent BIO_meth_set_* calls in real libssl. CreateTransportBio now returns NULL on failure, and Open checks the result. The partially-initialised SSL_CTX and SSL are released by Destroy — the test covers return-value behaviour; the lifecycle invariants set in the Destroy-idempotence commits already guarantee no leak. Driven by OpenReturnsFalseWhenBioMethNewFails; OpenSslFake gains SetBioMethNewFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 abort Open when BIO_new returns NULL and free BIO_METHOD Final return-value check in Open. BIO_new can fail under memory pressure; when it does, the BIO_METHOD we just allocated is freed inline rather than leaving it for Destroy, so callers that skip Destroy still don't leak. Two tests drive this: - OpenReturnsFalseWhenBioNewFails (return propagates via the CreateTransportBio-returns-NULL path added earlier) - BioNewFailureFreesBioMethodInline (cleanup co-located with the failure so it happens even without Destroy) Completes the "every OpenSSL call in Open is return-value-checked" and "partial-init failures release all resources" acceptance items. OpenSslFake gains SetBioNewFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat: S03.08 forward SolidSyslogTlsStreamConfig.cipherList to libssl Adds an optional cipherList field to SolidSyslogTlsStreamConfig. Non- NULL values are forwarded to SSL_CTX_set_cipher_list; NULL leaves OpenSSL's default cipher selection in place. The library stays policy-free — no baked-in SL4 cipher list. Callers that need SL4-compliant ciphers set the list at their level, where the security profile lives. docs/iec62443.md will document a recommended example. Driven by OpenPassesCipherListToSslCtx (list forwarded verbatim) and OpenSkipsCipherListSetupWhenNotConfigured (NULL means no call). Failure-handling and cleanup come in the next commit. OpenSslFake gains SSL_CTX_set_cipher_list stub + accessors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 abort Open when cipher list is rejected by libssl SSL_CTX_set_cipher_list returns 0 when every cipher in the list is unrecognised or unavailable in the linked libssl build. Continuing with a CTX that has no cipher suites to offer produces a handshake failure with no meaningful client-side error message — so fail fast at config time and surface the misconfiguration. CreateSslContext frees the CTX it allocated before returning NULL, matching the pattern of the other return-value checks. Driven by OpenReturnsFalseWhenCipherListRejected and CipherListFailureFreesCtx; OpenSslFake gains SetCipherListFails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: S03.08 integration test for cipher list rejection Pairs with the fake-based OpenReturnsFalseWhenCipherListRejected: real libssl rejects a cipher name it doesn't recognise, and the library surfaces that as Open failure. Covers the "unsupported cipher" item in the S03.08 acceptance list. Exercises the real SSL_CTX_set_cipher_list validation path, which the fake can only approximate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: S03.08 document SL4 TLS hardening in iec62443.md Captures the SL4 controls landed by S03.08 as part of the SL4 "what's in place" narrative, without promoting the TLS sender from Planned to Available (that's S03.11's scope): - Hostname verification (SNI + cert check) with return-checked setup calls. - Pinned SSL_VERIFY_PEER on the CTX. - Return-checked TLS 1.2 floor. - cipherList config field for caller-supplied SL4 cipher pinning, with a starting-point example (ECDHE+AESGCM:ECDHE+CHACHA20). - Idempotent Close/Destroy lifecycle. Traceability matrix cross-references the S03.08 story for CR 3.9. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: update DEVLOG for S03.08 TLS hardening session Covers the phased planning pass, what worked (test-support trio, pump-on-read, upfront surprises list), what didn't (BDD-first reversal, Phase 2/4 coupling, commit count), the key decisions (cipher policy at caller, per-Open BIO lifecycle, coverage stays unit-only), and deferred items (per-BIO fake refactor, TLS 1.3 cipher suites, max_proto_version control, S03.11 doc promotion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: S03.08 satisfy clang-format and clang-tidy on new TLS code CI checks flagged: - Format: the new `Tests/OpenSslIntegration/` sources and the `Tests/Support/OpenSslFake*` additions didn't match the project's clang-format aggressive alignment rules. Auto-formatted via `clang-format -i`. - Tidy: * CreateSslContext had two adjacent `const char*` params (bugprone-easily-swappable-parameters). Refactored to take the `SolidSyslogTlsStreamConfig*` directly — same logic, one pointer, future-proof for further config fields. * BioPairStream::Read had two `done = true` arms with identical bodies (bugprone-branch-clone). Combined under one `||` condition. * TlsTestCert had `#define` macros for integral constants (modernize-macro-to-enum). Promoted to an anonymous enum. * SetValidity had two adjacent `time_t` params. Refactored to take the TlsTestCertConfig* directly, same pattern as CreateSslContext. * TlsTestCert_WritePemToFile didn't check fopen. Added a NULL-guard early return. No behaviour change; 747 unit tests + 5 integration tests remain green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: S03.08 address CodeRabbit review on PR #183 Accepted: - Integration test now fails loudly on mkstemp failure instead of closing -1 and writing to the unexpanded template path. - CMakeLists.txt enforces OpenSSL >= 3.0 when SOLIDSYSLOG_OPENSSL is ON — the integration harness uses EVP_RSA_gen which is 3.0-only. Configure fails with a clear message on older OpenSSL rather than failing to link with a confusing missing-symbol error. - quality-summary job now depends on openssl-integration so the new JUnit artifact is guaranteed downloaded before the summary runs. - TlsTestServer nulls serverBio after SSL_set_bio to document the ownership transfer (the BIO pair's server side is now owned by SSL, freed via SSL_free). Deferred to S03.14 (#182): - TLS 1.3 ciphersuite control (SSL_CTX_set_ciphersuites) in production and in OpenSslFake. Library stays policy-free on cipher choice per the S03.08 design decision; TLS 1.3 plumbing plus a BDD proof is a coherent separate story. Declined: - Full return-value checks on every OpenSSL and libc call inside TlsTestServer_Create. This is test-harness code running in a controlled CI container where allocations are deterministic; defensive branches create untested paths with no realistic failure mode to cover. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: extract ConfigureExpectedHostname from Open Open was mixing orchestration with OpenSSL hostname setup detail. The extracted helper collapses eleven lines of nested ifs and five return- points into a single named step whose short-circuit `&&` preserves the original "skip set1_host if SNI setup failed" semantics. Single-exit, positive-logic (== 1 rather than != 1) to match the MISRA style we're moving toward. Open itself still has eight early returns — addressed in subsequent refactorings as the remaining stages (transport BIO wiring, handshake) are extracted. No behaviour change; all seven hostname-related unit tests and the integration suite remain green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: extract AttachTransportBio from Open Wraps the BIO-over-transport wiring (create the custom BIO, attach the transport as its backing data, hand it to the SSL handle) in a single named step. Open loses four lines of OpenSSL-vocabulary detail. Single-exit, positive-logic (`bio != NULL`) form. Eight-line helper replaces four inline lines in Open, for net clarity at the call site where the named operation is doing the explaining. No behaviour change — same BIO_new / BIO_set_data / SSL_set_bio calls in the same order; pointer-chain tests (LastBio, LastSetData*, LastSetBio*) stay green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: extract ReleaseHandshakeState from Close and Destroy Pulls the SSL/BIO_METHOD free-and-null pair out of both lifecycle endpoints. Close reads as three steps (shutdown, release, close transport). Destroy reads as two (release handshake state, free context). The null-guarding makes ReleaseHandshakeState safe to call from either path regardless of which subset of resources is live — already covered by the existing idempotence tests. No behaviour change; lifecycle-count tests (Close/Destroy variants for SSL_free and BIO_meth_free) stay green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: single-exit Open via && chain of named steps Open is now a narrative of six named handshake stages combined with short-circuit &&, returning once. One return, all positive logic, one level of abstraction throughout. Introduces three small stream-side wrappers so every step in the chain has the uniform bool(*)(stream) signature: - InitSslContext — stores stream->ctx, returns bool - InitSslSession — stores stream->ssl, returns bool - PerformHandshake — wraps SSL_connect > 0 No behaviour change. Short-circuit evaluation preserves the original bail-out semantics: a failed step skips all subsequent ones exactly as the prior early-return chain did. All 20+ Open-path tests stay green — identical OpenSSL call sequence, unchanged call counts, unchanged pointer-chain assertions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: single-exit CreateSslContext with named configurers CreateSslContext now reads as allocate, configure, clean up on failure — three concepts, single exit. The per-concern configure step becomes: ConfigureTrustAnchors(ctx, path) — load CA bundle + pin SSL_VERIFY_PEER ConfigureProtocolFloor(ctx) — TLS 1.2 floor ConfigureCipherList(ctx, list) — optional TLS 1.2 cipher pinning composed by ConfigureSslContext via && chain. Each helper does one thing at one level of abstraction, named for what it does. Four duplicated SSL_CTX_free + return NULL branches collapse to a single cleanup site in CreateSslContext. Minor ordering shift: SSL_CTX_set_verify now runs after (rather than between) the fallible configuration steps — grouped with the trust-chain concern it belongs to. Semantically equivalent; no test pins the ordering. All 12+ CreateSslContext-facing tests stay green: identical OpenSSL call set with unchanged arg captures, call counts, and CTX-free counts on failure paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: split CreateTransportBioMethod out of CreateTransportBio The two concerns — building the BIO_METHOD vtable and using it to allocate a BIO — are now separate functions, each with a single exit and one clear job. CreateTransportBioMethod handles BIO_meth_new + BIO_meth_set_*; returns NULL if allocation fails (guarding the setters). CreateTransportBio reads as: get the method, use it to make a BIO, clean up inline if BIO allocation fails so the BioNewFailureFreesBioMethodInline contract still holds. Three returns drop to one per function. No behaviour change — OpenSSL call sequence, arg captures, and failure-path cleanup counts all match the existing BIO-pathway tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: single-exit TransportBioCtrl Switch/case with two returns becomes a single return over a result accumulator that defaults to 0 (failure) and is set to 1 for the recognised commands. Same behaviour — the three BIO_CTRL tests stay green. Last multi-return function in SolidSyslogTlsStream.c. The file is now fully single-exit, positive-logic, with every function doing one thing at one level of abstraction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: order functions top-down, depth-first from callers Function bodies are now in the "helpers directly beneath first caller" order that the rest of the codebase follows: Create Destroy ReleaseHandshakeState (Destroy's helper, also shared with Close) Open InitSslContext (1st step) CreateSslContext ConfigureSslContext ConfigureTrustAnchors ConfigureProtocolFloor ConfigureCipherList InitSslSession (2nd step) AttachTransportBio (3rd step) CreateTransportBio CreateTransportBioMethod TransportBioCreate (registered callbacks) TransportBioCtrl TransportBioRead TransportBioWrite ConfigureExpectedHostname (4th step) PerformHandshake (5th step) Send Read Close Reading top-to-bottom takes the reader from public API down through the handshake pipeline into the raw OpenSSL wiring at the bottom. Each scroll-down goes one level deeper. Forward declarations at the top are now alphabetical — a predictable table of contents that doesn't need manual re-sorting when helpers are added. No behaviour change; full test / tidy / format run green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: extract ReleaseSsl, ReleaseBioMethod, ReleaseSslContext Every "if X != NULL, free X, null X" block in the file now lives in a one-job helper named for the resource it releases: ReleaseSsl — frees and nulls stream->ssl ReleaseBioMethod — frees and nulls stream->bioMethod ReleaseSslContext — frees and nulls stream->ctx Destroy becomes two lines of named operations: ReleaseHandshakeState(&instance); ReleaseSslContext(&instance); ReleaseHandshakeState becomes: ReleaseSsl(stream); ReleaseBioMethod(stream); CreateTransportBio's inline BIO_meth_free cleanup collapses to ReleaseBioMethod(stream) — same operation, same name. No behaviour change; all lifecycle-count tests and the integration suite stay green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: vtable is a static const, not per-instance assignment The four function-pointer slots on the base struct are constant for the lifetime of the module — they never change after Create and never vary between instances (there is only one instance). Lifting them into a file-scope const VTABLE and assigning it as a struct copy in Create communicates that invariant and collapses four per-call writes into one. Create now reads: instance.config = *config; instance.base = VTABLE; No behaviour change; test/tidy/format all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: prefix all TU-local functions with TlsStream_ Every static function in the file gets the TlsStream_ prefix, so the 21 internal-linkage identifiers are unique across the codebase and stack traces / debuggers name the module explicitly — e.g. TlsStream_Open instead of bare Open (which collides with Open in PosixTcpStream.c). Addresses MISRA C:2012 Rule 5.9 (internal-linkage identifier uniqueness) for this file. Consistent with the SolidSyslogTlsStream_ prefix on the public API: public gets the full project name, private gets the module-name shorthand. Rest of the codebase (PosixTcpStream.c, PosixUdpSender.c, etc.) will follow the same pattern in a separate chore; this commit establishes the convention on the file we've been refactoring. No behaviour change; test/tidy/format all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: mark all TU-local static functions static inline Uniform static inline on every private function in the file. The compiler was free to inline these at -O2 anyway; the explicit keyword: - makes intent visible at the declaration - gives the same signal at -O0 (debug builds) - does not break vtable / callback address-taking — static inline still generates an addressable definition when needed MISRA C:2012 has no rule against static inline. Coverage remains 100% (gcov attributes source lines regardless of inlining). No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: S03.08 OpenSslFake stores BIO data per-instance BIO_set_data and BIO_get_data shared a single global storage slot, so multiple BIOs aliased — bio2's data overwrote bio1's. BIO_new also returned a singleton pointer, hiding the aliasing further (every BIO was the same pointer). Replaced with a small fixed-size pool. Each pool slot is independent; the address of slot.slot is the BIO handle, slot.data is the per-BIO storage that backs BIO_set_data / BIO_get_data. BIO_new hands out the next free slot, Reset zeros the pool. Pool size 4 is plenty for current tests (production uses one BIO per Open); easy to bump if a future test needs more. The existing global lastSetDataArg is preserved so the existing OpenSslFake_LastSetDataArg accessor (which reports "the most recent data passed to BIO_set_data") continues to work. Driven by BioDataIsStoredPerInstance in OpenSslFakeTest.cpp: - BIO_new returns distinct pointers per call - BIO_get_data(bio1) returns the data set on bio1, not on bio2 Closes the last outstanding S03.08 acceptance bullet ("OpenSslFake stores BIO data per-instance"). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 22, 2026
Closed
Merged
3 tasks
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.
Purpose
Closes #165. Walking skeleton for E3 (TLS Transport, #5).
Adds OpenSSL-backed TLS as a new `SolidSyslogStream` implementation at `Platform/OpenSsl/`, proves end-to-end via a new `@tls` BDD scenario, and demonstrates the library's Stream abstraction is correctly shaped: the core `SolidSyslogStreamSender` (RFC 6587 framing) transmits over TLS without changes — TLS is a Stream, not a Sender.
Change Description
Architecture
Tests
Infrastructure from supporting PRs (already merged to main)
S3.7-specific BIO wiring (what made end-to-end work)
Example wiring
Test Evidence
Full preset gauntlet run locally against the gcc / clang / behave / syslog-ng devcontainer services:
Windows preset is CI-only (TLS test is tagged `@buffered` and excluded on Windows, as agreed in the story).
Areas Affected
Known limitations (explicitly out of scope; tracked)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Testing
Documentation