feat: S28.06 SolidSyslogLwipRaw_SetMarshal — marshal injection for NO_SYS=0#470
Merged
Conversation
…default New TU under Platform/LwipRaw/. SolidSyslogLwipRaw_SetMarshal installs a process-global marshal (single slot, same shape as SolidSyslog_SetErrorHandler); SolidSyslogLwipRaw_Marshal is the library-internal dispatch call site. Default is a direct-call null-object; SetMarshal(NULL) restores it. No wrapper call sites yet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…shal Every lwIP Raw API call each wrapper makes now batches into one SolidSyslogLwipRaw_Marshal hop per public operation: - Datagram: Open (udp_new), Close (udp_remove), SendTo (pbuf_alloc + udp_sendto + pbuf_free) — one hop each. - TcpStream: Open marshals setup+tcp_connect in one hop and the bounded connect spin stays on the caller's thread (sleeping the lwIP thread mid-connect would starve RX/timers); a tcp_abort-on-failure is a second hop. Send / Read / Close each take one hop; the pure would-block Read and the no-pcb Close take none. Driven by a new invariant rail: LwipFakeMarshalGuard arms every faked lwIP function (udp_*, tcp_*, pbuf_*) to record a breach if called outside an installed marshal, surfaced at fixture teardown with the offending call site. Fixtures install a tracking marshal; default direct-call semantics mean every existing scenario passes byte-identically. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ipaddr_aton is a pure string parser touching no lwIP core state, so Resolve is thread-safe as-is. The forward-looking comment marks where the S28.08 DnsResolver's dns_gethostbyname hop will belong. No behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Replace the wrong "marshal at the SolidSyslog API boundary" advice with the SolidSyslogLwipRaw_SetMarshal seam: default direct call (NO_SYS=1), plus two worked NO_SYS=0 marshals (tcpip_callback_with_block, LOCK_TCPIP_CORE), with the synchronous-callback contract and the on-tcpip-thread re-entry guard. - Add the LWIP_TCPIP_CORE_LOCKING row to the lwipopts table (marshal-dependent). - Add the SolidSyslogLwipRawMarshal.h public-header row to CLAUDE.md. - DEVLOG entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Match the SelfFromArg house style — explicit (struct X*) casts from the marshal's void* context rather than implicit conversion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 5 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces a process-global marshal seam through which all lwIP Raw API calls are routed, enabling RTOS integrators to synchronize lwIP work onto a dedicated thread. Datagram and TCP stream wrappers refactored to dispatch lwIP operations through marshalled callbacks; test fakes enforce marshal-boundary invariants; documentation updated with concrete threading integration patterns for ChangeslwIP Raw API marshal seam
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 366: Update the documentation line in SolidSyslogLwipRawMarshal.h to
narrow the “every lwIP Raw API call” claim by explicitly excluding the resolver
no-op path: mention that SolidSyslogLwipRawMarshal (symbols:
SolidSyslogLwipRawCallback, SolidSyslogLwipRawMarshalFunction,
SolidSyslogLwipRaw_SetMarshal) marshals Datagram/TcpStream operations but does
not marshal the resolver parsing path (e.g. ipaddr_aton); keep note that the
marshal must call its callback synchronously and that setting marshal = NULL
restores the default.
In `@docs/integrating-lwip.md`:
- Around line 199-201: Pick a single canonical story ID and replace all
mismatched IDs so they are identical across docs and source; specifically update
the references that appear alongside the phrases "FreeRtosLwip BDD target
(S28.07)", "lwIP BDD/DNS follow-up" and the resolver source comment to the
chosen canonical ID, ensuring every occurrence in the docs/integrating-lwip.md
and related source comments is changed to that one ID.
In `@Platform/LwipRaw/Source/SolidSyslogLwipRawDatagram.c`:
- Around line 142-143: SolidSyslogLwipRawDatagram_DoSendTo currently casts
call->Size to u16_t for pbuf_alloc which can wrap if call->Size > UINT16_MAX;
add a guard before calling pbuf_alloc that checks if call->Size > UINT16_MAX,
set call->Result to FAILED (or appropriate failure constant) and return early to
avoid truncation, then proceed to call pbuf_alloc with the safe cast when the
size is within bounds.
In `@Tests/Lwip/SolidSyslogLwipRawMarshalTest.cpp`:
- Around line 92-93: The test is comparing function pointers by casting them to
void* with POINTERS_EQUAL which is non-portable; replace the
POINTERS_EQUAL((void*) Increment, (void*) lastMarshalledCallback) check with a
function-pointer equality assertion using FUNCTIONPOINTERS_EQUAL (or the repo's
equivalent) to compare Increment and lastMarshalledCallback as
SolidSyslogLwipRawCallback types, leaving the context check
POINTERS_EQUAL(&calls, lastMarshalledContext) unchanged.
In `@Tests/Support/LwipFakes/Source/LwipFakeMarshalGuard.cpp`:
- Around line 43-47: The tracking marshal is not re-entrant-safe because
LwipFakeMarshalGuard_Active is set false by inner callbacks; modify
LwipFakeMarshalGuard_TrackingMarshal so it increments a guard counter (e.g.
LwipFakeMarshalGuard_Depth) or uses a local nesting increment before invoking
the callback and decrements after, setting LwipFakeMarshalGuard_Active to true
when depth>0 and only clearing it when the depth returns to zero; update any
declarations for LwipFakeMarshalGuard_Depth (initialize to 0) and replace the
simple boolean toggles in LwipFakeMarshalGuard_TrackingMarshal with depth-based
increment/decrement logic so nested calls don't prematurely clear the active
flag.
🪄 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: b7cb1796-b9f8-4351-a396-ba4e31e80cc8
📒 Files selected for processing (18)
CLAUDE.mdDEVLOG.mdPlatform/LwipRaw/Interface/SolidSyslogLwipRawMarshal.hPlatform/LwipRaw/Source/SolidSyslogLwipRawDatagram.cPlatform/LwipRaw/Source/SolidSyslogLwipRawMarshal.cPlatform/LwipRaw/Source/SolidSyslogLwipRawMarshalPrivate.hPlatform/LwipRaw/Source/SolidSyslogLwipRawResolver.cPlatform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.cTests/Lwip/CMakeLists.txtTests/Lwip/SolidSyslogLwipRawDatagramTest.cppTests/Lwip/SolidSyslogLwipRawMarshalTest.cppTests/Lwip/SolidSyslogLwipRawTcpStreamTest.cppTests/Support/LwipFakes/Interface/LwipFakeMarshalGuard.hTests/Support/LwipFakes/Source/LwipFakeMarshalGuard.cppTests/Support/LwipFakes/Source/LwipPbufFake.cTests/Support/LwipFakes/Source/LwipTcpFake.cTests/Support/LwipFakes/Source/LwipUdpFake.cdocs/integrating-lwip.md
Comment on lines
+142
to
+143
| struct pbuf* p = pbuf_alloc(PBUF_TRANSPORT, (u16_t) call->Size, PBUF_REF); | ||
| if (p != NULL) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Platform/LwipRaw/Source/SolidSyslogLwipRawDatagram.c"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "=== File: $FILE ==="
wc -l "$FILE"
echo "--- Surrounding lines 120-170 ---"
nl -ba "$FILE" | sed -n '120,170p'
echo "--- Find pbuf_alloc usage(s) ---"
rg -n "pbuf_alloc\(" "$FILE" || true
echo "--- Find LwipRawDatagram_DoSendTo ---"
rg -n "LwipRawDatagram_DoSendTo" "$FILE" || true
echo "--- Find Size type/assignments for SendToCall ---"
rg -n "struct LwipRawDatagram_SendToCall|SendToCall|call->Size" "$FILE" || true
echo "--- Find SOLIDSYSLOG_UDP_IPV6_SAFE_PAYLOAD definition/usages ---"
rg -n "SOLIDSYSLOG_UDP_IPV6_SAFE_PAYLOAD" -S . || trueRepository: DavidCozens/solid-syslog
Length of output: 288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Platform/LwipRaw/Source/SolidSyslogLwipRawDatagram.c"
test -f "$FILE" || { echo "Missing file: $FILE" >&2; exit 1; }
echo "=== File: $FILE ==="
wc -l "$FILE"
echo "--- Surrounding lines 110-170 ---"
cat -n "$FILE" | sed -n '110,170p'
echo "--- Find pbuf_alloc usage(s) ---"
rg -n "pbuf_alloc\(" "$FILE" || true
echo "--- Find LwipRawDatagram_DoSendTo ---"
rg -n "LwipRawDatagram_DoSendTo" "$FILE" || true
echo "--- Find SendToCall struct and call->Size usage ---"
rg -n "LwipRawDatagram_SendToCall" "$FILE" || true
rg -n "call->Size" "$FILE" || true
echo "--- Find SOLIDSYSLOG_UDP_IPV6_SAFE_PAYLOAD definition/usages ---"
rg -n "SOLIDSYSLOG_UDP_IPV6_SAFE_PAYLOAD" -S "$FILE" || true
rg -n "SOLIDSYSLOG_UDP_IPV6_SAFE_PAYLOAD" -S . || trueRepository: DavidCozens/solid-syslog
Length of output: 6328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the SendToCall struct + types in the target file
FILE="Platform/LwipRaw/Source/SolidSyslogLwipRawDatagram.c"
echo "--- SendToCall struct (around line ~1-60) ---"
cat -n "$FILE" | sed -n '1,80p'
echo "--- SolidSyslogDatagram/UDP payload interface references in Core ---"
rg -n "SolidSyslogDatagram_MaxPayload|MaxPayload\(" Core Platform Tests -S || true
echo "--- Find call sites of datagram SendTo ---"
rg -n "SolidSyslogDatagram_SendTo\(" -S . || true
rg -n "SendTo\(" Platform/ Core/ -S || true
echo "--- Find where size is produced/trimmed for UDP payload ---"
rg -n "Trim|FromMtu|MaxPayload|UDP_IPV6_SAFE_PAYLOAD" Core Platform Tests -S || trueRepository: DavidCozens/solid-syslog
Length of output: 35125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed_range () {
local file="$1"
local start="$2"
local end="$3"
echo "--- $file:$start-$end ---"
cat -n "$file" | sed -n "${start},${end}p"
}
sed_range "Core/Source/SolidSyslogUdpSender.c" 150 260
sed_range "Core/Source/SolidSyslogUdpPayload.c" 1 120
sed_range "Tests/Lwip/SolidSyslogLwipRawDatagramTest.cpp" 1 120
sed_range "Tests/Lwip/SolidSyslogLwipRawDatagramTest.cpp" 130 220
sed_range "Tests/Lwip/SolidSyslogLwipRawDatagramTest.cpp" 360 450Repository: DavidCozens/solid-syslog
Length of output: 18309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE_TEST="Tests/Lwip/SolidSyslogLwipRawDatagramTest.cpp"
echo "--- Search for pbuf alloc length assertions in Lwip test ---"
rg -n "LastAllocLength|PbufFake_LastAllocLength|PbufAlloc|pbuf_alloc|sendBytes\(" "$FILE_TEST" || true
echo "--- Show sections around any LastAllocLength usage ---"
rg -n "LastAllocLength|PbufFake_LastAllocLength" "$FILE_TEST" -n || true
# Print a few likely ranges
cat -n "$FILE_TEST" | sed -n '240,340p'
cat -n "$FILE_TEST" | sed -n '80,140p'
FILE_FAKE="Tests/Lwip/SolidSyslogLwipRawDatagramTest.cpp"
# also inspect lwip pbuf fake header/impl for how it stores last alloc len
FILE_PBUF_FAKE_DIR="Tests/Support/LwipFakes"
echo "--- Search in Lwip fakes for PbufFake_LastAllocLength and pbuf_alloc length ---"
rg -n "LastAllocLength|pbuf_alloc|PbufAlloc" "$FILE_PBUF_FAKE_DIR" || true
# print lwip pbuf fake implementation sections
ls -R "$FILE_PBUF_FAKE_DIR" | head -n 50
fd -i "PbufFake" "$FILE_PBUF_FAKE_DIR" || true
fd -i "lwip.*pbuf" "$FILE_PBUF_FAKE_DIR" || trueRepository: DavidCozens/solid-syslog
Length of output: 8528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show enum SolidSyslogDatagramSendResult to understand semantics
rg -n "enum SolidSyslogDatagramSendResult" -S Core/Interface Core/Source Platform/ -g'*.h' -g'*.c' || true
sed -n '1,120p' Core/Interface/SolidSyslogDatagram.h || true
# Inspect Posix + Winsock implementations for OVERSIZE handling
for f in \
Platform/Posix/Source/SolidSyslogPosixDatagram.c \
Platform/Windows/Source/SolidSyslogWinsockDatagram.c \
Platform/PlusTcp/Source/SolidSyslogPlusTcpDatagram.c \
Core/Source/SolidSyslogNullDatagram.c
do
echo "=== $f: sendto logic (rough range) ==="
test -f "$f" || continue
rg -n "OVERSIZE|EMSGSIZE|WSAEMSGSIZE|sendto|sendBytes|SendTo" "$f" -S || true
done
# Also inspect the SolidSyslogDatagram_SendTo wrapper
sed -n '1,120p' Core/Source/SolidSyslogDatagram.c || trueRepository: DavidCozens/solid-syslog
Length of output: 6935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "SolidSyslogDatagram_SendTo|Datagram_SendTo|MaxPayload\(|MaxPayload.*SendTo|SendTo.*MaxPayload|oversize" -S . \
--glob='*.md' --glob='*.h' --glob='*.c' --glob='*.cpp' --glob='*.hpp' --glob='*.hpp.in' || true
rg -n "SOLIDSYSLOG_UDP_IPV6_SAFE_PAYLOAD|UINT16_MAX|u16_t" -S Platform/LwipRaw/Source/SolidSyslogLwipRawDatagram.c Core/Interface/Core/Source --glob='*.c' --glob='*.h' || trueRepository: DavidCozens/solid-syslog
Length of output: 17536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant docs sections
sed -n '260,360p' docs/integrating-lwip.md
sed -n '400,520p' docs/misra-deviations.md
sed -n '280,370p' DEVLOG.md
sed -n '300,340p' docs/misra-deviations.md
sed -n '120,190p' docs/bdd.mdRepository: DavidCozens/solid-syslog
Length of output: 19640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect SolidSyslogUdpSender around where it computes/uses `size` for SendTo
FILE="Core/Source/SolidSyslogUdpSender.c"
echo "--- $FILE: show earlier transmit call site context ---"
cat -n "$FILE" | sed -n '1,280p'
echo "--- $FILE: search for SOLIDSYSLOG_MAX_MESSAGE_SIZE and max payload usage ---"
rg -n "SOLIDSYSLOG_MAX_MESSAGE_SIZE|MaxPayload|trim|oversize|clipLimit|trimmed" "$FILE" || true
# 2) Find and inspect SOLIDSYSLOG_MAX_MESSAGE_SIZE definition
echo "--- Search for SOLIDSYSLOG_MAX_MESSAGE_SIZE definition ---"
rg -n "SOLIDSYSLOG_MAX_MESSAGE_SIZE" -S . || true
# Show the definition file(s) near first match
for f in $(rg -n "SOLIDSYSLOG_MAX_MESSAGE_SIZE" -S . | cut -d: -f1 | sort -u); do
echo "=== $f ==="
sed -n '1,120p' "$f" | cat -n | head -n 120
done
# 3) Search for any extremely large literals close to u16_t range
echo "--- Search for large numeric constants near 65535/u16 max ---"
rg -n "65535|UINT16_MAX|u16_t|cast.*u16_t|\\(u16_t\\)" -S Core Platform Tests --glob='*.c' --glob='*.h' || true
# 4) Inspect oversize-related UDP sender tests for what message sizes they use
FILE_TEST="Tests/SolidSyslogUdpSenderTest.cpp"
echo "--- $FILE_TEST: inspect oversize tests ---"
rg -n "Oversize|EMSGSIZE|MaxPayload|Retry" "$FILE_TEST" || true
# Print ranges around likely oversize tests
cat -n "$FILE_TEST" | sed -n '560,780p'
# 5) Inspect docs statements about the contract: whether SendTo is called with size<=MaxPayload
echo "--- docs/integrating-lwip.md around mentioned guarantee line ---"
rg -n "guarantee.*SolidSyslogDatagram_SendTo" docs/integrating-lwip.md || true
# show around line ~311 if exists
cat -n docs/integrating-lwip.md | sed -n '280,340p'
echo "--- DEVLOG.md around the note about honours MaxPayload before reaching SendTo ---"
rg -n "honours `MaxPayload\\(\\)`|MaxPayload\\(\\) before reaching SendTo|before reaching SendTo" DEVLOG.md || true
cat -n DEVLOG.md | sed -n '280,360p'Repository: DavidCozens/solid-syslog
Length of output: 50453
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Search Core for MaxPayload usage outside datagram backends ---"
rg -n "MaxPayload\(" Core/Source -S || true
echo "--- Inspect SolidSyslog_Service path (search for Sender->Send call site) ---"
rg -n "SolidSyslog_Service\\(|\\.Send\\(|Sender_.*Send\\(|Send\\(" Core/Source/SolidSyslog.c Core/Source -S || true
echo "--- Inspect SolidSyslogUdpSender integration (where buffer size passed into Sender) ---"
rg -n "SolidSyslogUdpSender|UdpSender_Send\\(" Core/Source -S || true
echo "--- Check tunable constraints for SOLIDSYSLOG_MAX_MESSAGE_SIZE vs UINT16 ---"
# show defaults
sed -n '1,120p' Core/Interface/SolidSyslogTunablesDefaults.h
# also look for other tunables headers that `#define` SOLIDSYSLOG_MAX_MESSAGE_SIZE
rg -n "SOLIDSYSLOG_MAX_MESSAGE_SIZE" Core/Interface -S || true
echo "--- Search for UINT16_MAX / u16 constraints anywhere ---"
rg -n "UINT16_MAX|65535|u16_t" Core/Interface Core/Source -S --glob='*.h' --glob='*.c' || true
echo "--- Locate docs statement about Service honoring MaxPayload before SendTo (integrating-lwip / misra-deviations / devlog) ---"
for f in docs/integrating-lwip.md docs/misra-deviations.md DEVLOG.md; do
echo "=== $f ==="
rg -n "honours .*MaxPayload|MaxPayload\\(\\).*before reaching SendTo|before reaching SendTo|already honours" "$f" -S || true
doneRepository: DavidCozens/solid-syslog
Length of output: 15914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect SolidSyslog_Service logic around where it computes len and calls Sender_Send
FILE="Core/Source/SolidSyslog.c"
echo "--- $FILE: SolidSyslog_Service around send call sites ---"
rg -n "SolidSyslog_Service\\(|Sender_Send\\(" "$FILE"
sed -n '180,330p' "$FILE"
# 2) Look for any use of MaxPayload() in SolidSyslog.c (or call stack)
echo "--- $FILE: search for MaxPayload in SolidSyslog.c ---"
rg -n "MaxPayload\\(" "$FILE" || true
# 3) Check SOLIDSYSLOG_MAX_MESSAGE_SIZE compile-time bounds relative to u16_t/UINT16_MAX (repo-wide)
echo "--- Search for SOLIDSYSLOG_MAX_MESSAGE_SIZE upper bound / UINT16_MAX checks ---"
rg -n "SOLIDSYSLOG_MAX_MESSAGE_SIZE.*(65535|UINT16_MAX|u16_t)|((65535|UINT16_MAX).*(SOLIDSYSLOG_MAX_MESSAGE_SIZE))" -S . --glob='*.h' --glob='*.c' --glob='*.cpp' --glob='*.hpp' || true
# 4) Search for other casts from size_t to u16_t (to see established pattern/assumptions)
echo "--- Search for casts to u16_t in C code ---"
rg -n "\\(u16_t\\)" -S Core Platform Tests --glob='*.c' --glob='*.h' || true
# 5) Confirm integrating-lwip claims about Service honoring MaxPayload
echo "--- integrating-lwip.md: search for honours MaxPayload / before reaching SendTo ---"
rg -n "honours .*MaxPayload|MaxPayload\\(\\) before reaching SendTo|before reaching SendTo" docs/integrating-lwip.md DEVLOG.md docs -S || trueRepository: DavidCozens/solid-syslog
Length of output: 5871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Global search for u16 bounds / truncation risk
rg -n "65535|UINT16_MAX|UINT16|u16_t.*MAX|MAX_MESSAGE_SIZE.*655|MAX_MESSAGE_SIZE.*UINT16" -S . --glob='*.h' --glob='*.c' --glob='*.cpp' --glob='*.hpp' || true
echo "--- Inspect SOLIDSYSLOG_MAX_MESSAGE_SIZE compile-time checks ---"
# show the whole tunables defaults header around the MAX_MESSAGE_SIZE definition area
sed -n '1,120p' Core/Interface/SolidSyslogTunablesDefaults.h
echo "--- Find all occurrences of SOLIDSYSLOG_MAX_MESSAGE_SIZE in Core/Interface (for any additional checks) ---"
rg -n "SOLIDSYSLOG_MAX_MESSAGE_SIZE" Core/Interface -S || true
echo "--- Inspect SolidSyslogTunables.h umbrella (if it adds extra checks) ---"
if [ -f Core/Interface/SolidSyslogTunables.h ]; then
sed -n '1,160p' Core/Interface/SolidSyslogTunables.h
else
echo "Missing Core/Interface/SolidSyslogTunables.h"
fi
echo "--- Inspect any message-length type conversions (size_t->uint16_t/u16_t) besides lwIP/TCP stream ---"
rg -n "\\(u16_t\\)|\\(uint16_t\\)" -S Core Platform --glob='*.c' --glob='*.h' || true
echo "--- Inspect CMake validation for SOLIDSYSLOG_MAX_MESSAGE_SIZE ---"
rg -n "SOLIDSYSLOG_MAX_MESSAGE_SIZE" CMakeLists.txt -S || true
sed -n '160,260p' CMakeLists.txtRepository: DavidCozens/solid-syslog
Length of output: 12499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- DEVLOG.md around line 300-330 ---"
sed -n '280,340p' DEVLOG.md | cat -n
echo "--- docs/integrating-lwip.md around the 'guarantee SolidSyslogDatagram_SendTo already provides' line (~311) ---"
rg -n "guarantee .*SolidSyslogDatagram_SendTo" docs/integrating-lwip.md
LINE="$(rg -n "guarantee .*SolidSyslogDatagram_SendTo" docs/integrating-lwip.md | head -n1 | cut -d: -f1)"
START=$((LINE-60))
END=$((LINE+60))
sed -n "${START},${END}p" docs/integrating-lwip.md | cat -n
echo "--- docs/integrating-lwip.md around 'SOLIDSYSLOG_UDP_IPV6_SAFE_PAYLOAD' unconditionally (~416) ---"
rg -n "SOLIDSYSLOG_UDP_IPV6_SAFE_PAYLOAD" docs/integrating-lwip.md
LINE2="$(rg -n "SOLIDSYSLOG_UDP_IPV6_SAFE_PAYLOAD" docs/integrating-lwip.md | head -n1 | cut -d: -f1)"
START2=$((LINE2-40))
END2=$((LINE2+80))
sed -n "${START2},${END2}p" docs/integrating-lwip.md | cat -nRepository: DavidCozens/solid-syslog
Length of output: 12545
Prevent silent truncation of the pbuf_alloc length cast to u16_t.
SolidSyslogLwipRawDatagram_DoSendTo casts call->Size to u16_t in pbuf_alloc(..., (u16_t) call->Size, ...) (line 142). If call->Size > UINT16_MAX, the value can wrap and lwIP may allocate/send the wrong length. Add a bound check against UINT16_MAX (and keep call->Result as FAILED/return early) before the cast.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Platform/LwipRaw/Source/SolidSyslogLwipRawDatagram.c` around lines 142 - 143,
SolidSyslogLwipRawDatagram_DoSendTo currently casts call->Size to u16_t for
pbuf_alloc which can wrap if call->Size > UINT16_MAX; add a guard before calling
pbuf_alloc that checks if call->Size > UINT16_MAX, set call->Result to FAILED
(or appropriate failure constant) and return early to avoid truncation, then
proceed to call pbuf_alloc with the safe cast when the size is within bounds.
Owner
Author
There was a problem hiding this comment.
Declined as out-of-scope. This (u16_t) cast is pre-existing (S28.04 baseline), not introduced here — the redesign just moved the line. It's lwIP-specific (pbuf_alloc takes u16_t; the Posix/PlusTcp datagram siblings pass size_t to their socket APIs so have no such cast), and size is contract-bounded by Datagram_MaxPayload (~1232 B IPv6-safe) with UdpSender running an oversize-retry path above it, so a size > UINT16_MAX datagram cannot reach here. Not changing it in this PR to keep scope to the marshal seam.
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — the contract layering (caller-side MaxPayload → UdpSender oversize-retry → SendTo) means an out-of-range value cannot reach the cast, and it's a pre-existing S28.04 baseline pattern rather than anything introduced here. I'll note this for future reviews.
Would you like me to resolve this comment?
(>_<) 🐇
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 470
File: Platform/LwipRaw/Source/SolidSyslogLwipRawDatagram.c:152-153
Timestamp: 2026-05-28T22:43:31.454Z
Learning: In DavidCozens/solid-syslog, `LwipRawDatagram_DoSendTo` (Platform/LwipRaw/Source/SolidSyslogLwipRawDatagram.c) casts `call->Size` to `u16_t` for `pbuf_alloc(PBUF_TRANSPORT, (u16_t) call->Size, PBUF_REF)`. This cast is safe and intentional: `size` is contract-bounded by `Datagram_MaxPayload()` (~1232 B, IPv6-safe), and `UdpSender` enforces an oversize-retry path above `SendTo`, so a value exceeding `UINT16_MAX` cannot reach this code path. The cast is a pre-existing S28.04 baseline pattern (not introduced by the marshal-seam refactor) required by the lwIP `pbuf_alloc` API signature. Do not flag this cast as a truncation risk in future reviews.
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: 283
File: Platform/OpenSsl/Source/SolidSyslogTlsStream.c:363-363
Timestamp: 2026-05-07T08:22:59.856Z
Learning: In this solid-syslog codebase, follow the documented contract: do not add NULL/defensive checks inside library/internal code paths (e.g., `_Create` functions) when the NULL case cannot occur under normal internal usage and is covered by framework or internal wiring guarantees. Only add validation at true system boundaries such as user input and external APIs. Where setup-time wiring contracts between the integrator and the library require specific function-pointer fields (e.g., `SolidSyslogTlsStreamConfig.sleep`), assume those required fields are present and intentionally omit NULL guards in internal creation functions.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 291
File: Example/FreeRtos/HelloWorld/CmsdkUart.c:17-42
Timestamp: 2026-05-08T17:19:38.382Z
Learning: In this codebase (DavidCozens/solid-syslog), internal driver/config/access structs may intentionally store raw pointers without NULL checks, `isInitialized` flags, or defensive struct copies. Treat this as an explicit project policy: do not flag store-by-pointer usage or missing NULL/initialization guards for these internal config/access structs when the pointer is provided by production callers and is guaranteed to point to `static const` objects with program lifetime (per CLAUDE.md). Only flag NULL/initialization issues when the pointer originates from potentially invalid/external/untrusted sources or when the code does not establish lifetime/guarantees.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 303
File: Example/Common/ExampleInteractive.h:14-17
Timestamp: 2026-05-09T15:56:13.853Z
Learning: During code review, ensure solid-syslog Tier 1/2 library callbacks in Core/ and Platform/ follow the CLAUDE.md “Callback Conventions”: the callback API must take a `void*` context parameter and the implementation must use the paired configuration field associated with that callback context. Do not apply this requirement to Tier 3 example handlers (e.g., Example/Common/, Example/FreeRtos/, such as `ExampleInteractiveSwitchHandler`/`ExampleInteractiveSetHandler`), which are allowed to use file-static globals per the documented example-tier pattern. If a multi-instance use-case arises, migrate Switch and Set together in a single cohesive change rather than piecemeal updates.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 372
File: Platform/FreeRtos/Source/SolidSyslogFreeRtosDatagram.c:0-0
Timestamp: 2026-05-15T13:09:11.811Z
Learning: When reviewing PRs that bulk-rename identifiers (especially “static function rename” changes) in this repo, check for unintended renames inside FreeRTOS macro usages. A too-broad regex can incorrectly prefix macro tokens used in `static const` initializers (e.g., `pdMS_TO_TICKS` from FreeRTOS). Look for signs like a renamed macro name being incorrectly prefixed (e.g., `FreeRtosDatagram_pdMS_TO_TICKS`), and if present, treat it as evidence the identifier-extraction regex was overly broad and requires correction/revert of the affected rename changes.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 400
File: Core/Source/SolidSyslogOriginSdStatic.c:11-11
Timestamp: 2026-05-18T17:07:19.499Z
Learning: In this repo (DavidCozens/solid-syslog), an IWYU CI gate enforces strict direct-header inclusion: for every symbol (types, enums, functions, etc.) that is directly referenced in a translation unit’s `.c` or `.h` file, include the specific header that declares that symbol. Do not rely on transitive includes from other headers.
Example: if a `.c`/`.h` file uses `SolidSyslogSeverity` enums, it must directly `#include` `SolidSyslogPrival.h` even if another included header (e.g., `SolidSyslogError.h`) pulls it in transitively.
When reviewing, do not flag or recommend removing includes solely because they are “transitive” in this codebase—removing them may still compile locally but can fail the IWYU CI check.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 411
File: Platform/FreeRtos/Source/SolidSyslogFreeRtosDatagramStatic.c:42-50
Timestamp: 2026-05-20T12:23:42.200Z
Learning: In the solid-syslog (DavidCozens) codebase, review pool-backed `_Destroy` implementations to ensure they *uniformly* warn on any handle not present in the relevant pool (the established E11 pattern). This intentionally covers: (1) stale/already-destroyed handles, (2) never-issued handles, and (3) the shared-null-sentinel returned on pool exhaustion. Do not ask for a special-case early-exit/silent no-op when the handle equals the shared null sentinel—integrators already receive an error at `_Create` time on pool exhaustion, and the `_Destroy` warning is the deliberate secondary safety net. If a change to this behavior is proposed (e.g., suppressing warnings / changing null-sentinel semantics), it must be applied epic-wide and consistently across all migrated classes/platform variants, not as a per-class or per-platform deviation.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 416
File: DEVLOG.md:155-155
Timestamp: 2026-05-20T17:56:08.543Z
Learning: When a source file needs the public facility/severity enums used by `SolidSyslog_Error` (or other public APIs), include the public header `Core/Interface/SolidSyslogPrival.h` rather than `Core/Source/SolidSyslogPrivate.h`. Reserve `Core/Source/SolidSyslogPrivate.h` (TU-internal structs like `SolidSyslog`) for the translation unit that defines/uses them (e.g., `SolidSyslogStatic.c`).
…very CI's cppcheck-misra (the --addon=misra step, which the local cppcheck preset does not run) flagged the marshal callbacks' void*-context recoveries under MISRA 11.5. Rather than suppress all six, collapse them: each wrapper now uses a single per-class call struct recovered through one CallFromContext helper — the marshal-seam analogue of the existing SelfFromArg, matching D.002's "one suppression site per class, not one per callback". Net new suppressions drop from 6 to 2 (LwipRawDatagram + LwipRawTcpStream CallFromContext). The four pre-existing LwipRaw suppressions (SelfFromBase 11.3 ×2, SelfFromArg 11.5, payload 11.8) are renumbered for the shifted lines. Full cppcheck-misra passes locally (exit 0); all Lwip tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…idy) analyze-tidy-freertos-plustcp (the lane that compiles the Lwip tests — the plain analyze-tidy image has no LWIP_PATH and skips them) failed on readability-identifier-naming: file-scope struct tags must be bare PascalCase with no underscore. Rename LwipRawDatagram_Call → LwipRawDatagramCall and LwipRawTcpStream_Call → LwipRawTcpStreamCall, and add both to the StructIgnoredRegexp Tier-2 allow-list per the convention in .clang-tidy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… idioms) - CLAUDE.md: narrow the marshal row to Datagram/TcpStream operations and note the Resolver's ipaddr_aton parse is intentionally not marshalled. - integrating-lwip.md: standardize follow-up story IDs — BDD target = S28.07, DnsResolver = S28.08 — fixing the stale S28.07 DNS refs and the S28.06 BDD ref. - SolidSyslogLwipRawMarshalTest: FUNCTIONPOINTERS_EQUAL for the callback-pointer assertion (function-pointer→void* cast is non-portable). - LwipFakeMarshalGuard: save/restore the active flag so a nested marshal can't raise a false breach. Declined the u16_t-truncation finding on LwipRawDatagram_DoSendTo: the cast is pre-existing (S28.04 baseline), lwIP-specific (pbuf_alloc takes u16_t; siblings pass size_t), and size is contract-bounded by Datagram_MaxPayload (~1232 B). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
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.
Closes #467 (E28 story 6).
What
Adds
SolidSyslogLwipRaw_SetMarshal— a process-global seam that routesevery lwIP Raw API call the LwipRaw adapters make through one marshal hop per
public operation. The default is a direct-call null object (correct for
NO_SYS=1);NO_SYS=0integrators install atcpip_callback_with_blockshim or a
LOCK_TCPIP_CORE/UNLOCK_TCPIP_COREpair to pin the calls to thelwIP-owning thread. Same single-global-slot shape as
SolidSyslog_SetErrorHandler.Why: the integrator guide S28.05 shipped advised "marshal at the
SolidSyslog API boundary" (wrap
SolidSyslog_Serviceintcpip_callback).That was wrong — it drags file I/O, mbedTLS crypto, the CircularBuffer mutex,
the StreamSender formatter work, and the Resolver parse onto the tcpip
thread, starving lwIP's timer/RX path. The correct boundary is the
individual lwIP call.
How (TDD, per-slice)
internal
SolidSyslogLwipRaw_Marshaldispatch lives in a private header.5 tests covering default direct-call / install / NULL-restore / one-hop-
per-dispatch / callback+context forwarding.
invariant rail (
LwipFakeMarshalGuard) that records any lwIP call madeoutside an installed marshal and fails at teardown with the offending
fake call site. Existing 36 + 55 scenarios pass byte-identically (direct-
call default).
ipaddr_atonis a pure parser; the DNShop belongs to the future DnsResolver).
docs/integrating-lwip.mdthreading section rewritten with thecorrected rule + three worked examples (NO_SYS=1, NO_SYS=0+tcpip_callback,
NO_SYS=0+core-locking); CLAUDE.md header row; DEVLOG.
Notes for review
("arm rail → red → green via tracking marshal, before routing") can't go
green, since the rail flag only flips once production routes through the
marshal, and the shared
LwipPbufFakecouples both wrappers into onecommit. Same end state, every commit green. (Agreed with maintainer.)
fakes are C and CppUTest aborts via throw/longjmp (UB through C frames).
The captured
__FILE__/__LINE__still points at the exact lwIP call.tcp_connectin one hop; the boundedconnect spin stays on the caller's thread (sleeping the tcpip thread
mid-connect would starve RX/timers);
tcp_abort-on-failure is a 2nd hop.Tests
Local debug: Address 12 / Resolver 16 / Datagram 36 / TcpStream 55 /
Marshal 5 — all green.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
SolidSyslogLwipRaw_SetMarshal().Documentation
NO_SYS=0configurations, includingtcpip_callbackand core-locking approaches.