Build the native tests under address and UB sanitisers #53
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/native-sanitisers"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
CCS-UF-5 shipped an out-of-bounds read the committed tests could not fail.
env:nativenow builds with-fsanitize=address,undefined -fno-sanitize-recover=all -g, so that class of bug fails the run.The default, not a second environment. CI only invokes
pio test -e native, so anative_sanitisedenv would have needed a new workflow step and would have been a run nobody does locally. The cost is small enough not to justify the split: a clean build goes from about 7.7s to 9.7s, all of it compile time, and the run itself is unchanged (5.9s against 6.2s warm).Toolchain. Confirmed in CI rather than only on the desk: run #1368 on
37f57a0was green with the sanitisers already in place. Both the desk and the runner are ubuntu-24.04 on gcc 13.3.Leak suppression. Narrowed to
leak:ctrl::CanBus::beginvia__lsan_default_suppressions()intest_mcp2515_tx, the only test that reaches a backend'snew, instead ofASAN_OPTIONS=detect_leaks=0. Leak detection stays on for the other 28 test directories and for the rest of that binary — checked by planting an unrelatednew int[4]in the same file and confirming it still errors.CI test step is now
-v.pio testprints only the lines it recognises as test output, so without it an ASan or UBSan failure reaches the log as nothing butProgram received signal SIGHUP. A failing full suite is around 900 lines verbose.Tested by planting faults and confirming each fails: a read 8 bytes past the manifest buffer (ASan), and a shift overflow (UBSan).
-fno-sanitize-recover=allis load-bearing — without it the overflow printed a runtime error and the test still passed.The sweep found no memory error anywhere on the current tree, so the ticket's claim still holds after UF-6. UF-12 is not in this base.
pio runfor both nodes andpio run -e host_simstill build.One limitation worth a follow-up rather than this PR: the manifest tests feed
.rodatastring literals sized bystrlen, so ASan's redzone starts after the literal's NUL. A one-byte overshoot, or an over-read inside a deliberately shortened length as attest_firmware_manifest.cpp:166, still slips through. Feeding those cases from a heap buffer sized to the length would make the bound tight.CCS-UF-11.
pio testhides the sanitiser report, so as it stands a CI failure says onlyProgram received signal SIGHUP (Hangup)and nothing about the fault. Reproduced by restoring the pre-UF-5matchStringover-read: the run fails (good), but the ASan block appears only withpio test -e native -v, or by running.pio/build/native/programdirectly. Worth a sentence in CONTRIBUTING next to the new paragraph, since the report is the whole point of the change.Otherwise the decision holds: CI already runs
pio test -e native(build.yml:41) so the sanitisers are exercised with no workflow change, and the restored over-read does fire, so the claim about UF-5 is real.One thing I could not check: the pipeline status on this PR. The leak suppression matches on a symbolised frame name, so it needs
addr2lineorllvm-symbolizerin the runner image — withASAN_OPTIONS=symbolize=0the suppression stops matching and the suite fails with7 byte(s) leaked. Loud rather than silent, so fine, but confirm the run is green before merging.@ -82,0 +82,4 @@It builds with `-fsanitize=address,undefined`, so a read past a buffer or anoverflow fails the run rather than quietly returning the right answer, which ishow an out-of-bounds read in the manifest scanner once passed a green suite. Itadds nothing measurable to the nine seconds the suite takes. Leak detection isDrop "the nine seconds the suite takes". A wall-clock figure in the docs ages — the warm full suite is 6.2s on my machine already. "adds nothing measurable" carries the point on its own.
@ -82,0 +83,4 @@overflow fails the run rather than quietly returning the right answer, which ishow an out-of-bounds read in the manifest scanner once passed a green suite. Itadds nothing measurable to the nine seconds the suite takes. Leak detection ison too; the one deliberate never-freed allocation, the driver the MCP2515"the one deliberate never-freed allocation" is not accurate: all three backends do the same never-freed
newinbegin()(can_bus_socketcan.cpp:86,can_bus_twai.cpp:79). MCP2515's is only the one a host test reaches — the suppression patternctrl::CanBus::beginwould cover any of them.@ -58,1 +58,4 @@; Linux CAN headers with it.;; Sanitised rather than fast: ASan and UBSan cost nothing measurable on a suite; this small, and they catch the reads past a buffer that an assertion on theCut "and they catch the reads past a buffer that an assertion on the answer cannot" — that is what a sanitiser is. The cost measurement and the
-fno-sanitize-recoversentence are the parts that earn their place.@ -63,3 +68,3 @@; a library it cannot see is one it will not link.lib_deps = node-runtimebuild_flags = ${env.build_flags} -DCTRL_CAN_SOCKETCAN -Ilib/node-runtime/src -Isrc/host_sim -Isrc/node_bathroom -Isrc/node_lighting -Itoolsbuild_flags = ${env.build_flags} -DCTRL_CAN_SOCKETCAN -Ilib/node-runtime/src -Isrc/host_sim -Isrc/node_bathroom -Isrc/node_lighting -Itools -fsanitize=address,undefined -fno-sanitize-recover=allAdd
-g. Without debug info the frames carry no source location:#0 ... in matchString (program+0x1a42e). With-gthe same report readsmatchString lib/node-runtime/src/firmware_manifest.cpp:177, which is the difference between an actionable CI log and a hunt.@ -24,2 +24,4 @@} // namespace fake// The driver begin() news lives as long as the node does and is never freed,// which on a host is a leak. Scoped to this one call rather than turning leakThe second sentence repeats the CONTRIBUTING paragraph. The first sentence, why the allocation is deliberate, is the part that has to live here.
Thanks — all five acted on in
9984679.pio testprints only the lines it recognises as test output, solog_path=stdoutdoes not help either; the CI test step now runspio test -e native -v, which puts the report in the log. A failing full suite comes to 944 lines verbose, so the noise is affordable. Noted in CONTRIBUTING and CLAUDE.md.-gadded.parseFirmwareManifest ... firmware_manifest.cpp:266now, instead of an address.37f57a0was green with the sanitisers already in place, so CI's gcc 13 resolves the suppression. That also settles the toolchain question the ticket raised.begin()and the MCP2515 one is only the one a host test reaches. The nine-second figure is gone.All five earlier points are addressed and the mechanism holds: a leak, a UBSan overflow and a global over-read each ERROR the run with exit 1, and
-vputs the full report withfile:linein the log. Checked on PlatformIO 6.2.0 too, since CI installs the latest core rather than your 6.1.19 — same output, 828 lines green, no compile spam. Nothing blocking.Two prose nits below.
@ -82,0 +82,4 @@It builds with `-fsanitize=address,undefined`, so a read past a buffer or anoverflow fails the run rather than quietly returning the right answer, which ishow an out-of-bounds read in the manifest scanner once passed a green suite. Itadds nothing measurable to the suite's runtime. **`pio test` prints only what itSix sentences and three topics in one paragraph — why sanitised, what the runtime costs, how to read a failure, and the leak suppression. Worth splitting after "...passed a green suite." The runtime-cost sentence also says the same thing as the new platformio.ini comment; keep it in one place.
@ -82,0 +86,4 @@recognises as test output**, so a sanitiser failure looks like`Program received signal SIGHUP` and nothing else: rerun with `-v`, which CIalready does, to see the report. Leak detection is on too, and every `CanBus`backend news a driver in `begin()` that is never freed. Only the MCP2515 one is"news a driver" — "allocates a driver" reads better; same for "The driver begin() news lives..." in test_mcp2515_tx.cpp:26.