CI: add sanitizer build job (ASAN/UBSAN) - #131
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The sanitizer flags are not reliably propagated to the make distcheck rebuild, so the new job may not actually run the distcheck build under ASAN/UBSAN as intended.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new GitHub Actions job intended to run the project’s build and make distcheck under AddressSanitizer/UndefinedBehaviorSanitizer to catch memory errors in CI.
Changes:
- Add a new
sanitizersjob onubuntu-latestusingclang. - Configure ASAN/UBSAN runtime options and run
make distcheckin CI.
File summaries
| File | Description |
|---|---|
| .github/workflows/build.yml | Adds a new CI job to build and run distcheck under sanitizers. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
99f7fdd to
f58dc0c
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The sanitizer job wiring and the new fixture parse harness align with existing dctool descriptor selection behavior and don’t introduce any verified functional issues.
Review details
- Files reviewed: 3/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
c6e7076 to
ac3f7d5
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 3 out of 6 changed files in this pull request and generated 2 comments.
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install libbluetooth-dev libusb-1.0-0-dev |
| - run: make | ||
| - run: make distcheck | ||
| - name: Run parse fixtures | ||
| run: test/run-parse.sh |
Add a 'sanitizers' job that builds with -fsanitize=address,undefined using clang on ubuntu-latest and runs make distcheck under the sanitizer. ASAN_OPTIONS=detect_leaks=1 and abort_on_error=1 ensure any detected leak or memory error fails the build immediately. Full parser coverage under the sanitizer requires sample dump fixtures and a parse harness to be wired into this job in a follow-up. Signed-off-by: Michael Keller <github@ike.ch>
Add raw per-dive binary dumps (captured via dctool download) for three device families, together with a manifest and a shell harness that runs dctool parse over each fixture and fails on any non-zero exit. test/fixtures/manifest.txt Maps each blob to the exact dctool descriptor string used at capture and required at replay. Format: filename|vendor|product. test/fixtures/shearwater_petrel2-0001.bin (21 KB) Shearwater Petrel 2, SHEARWATER_PETREL family, CCR dive, no GPS. Descriptor: 'Shearwater Petrel 2', transport: bluetooth (classic). test/fixtures/hw_ostc5-0001.bin (15 KB) Heinrichs Weikamp OSTC 5, HW_OSTC3 family, CCR dive, no GPS. Descriptor: 'Heinrichs Weikamp OSTC 5', transport: bluetooth (classic). test/fixtures/garmin_descent_mk2i-0001.bin (12 KB) Garmin Descent Mk2i, GARMIN family, no GPS. Descriptor: 'Garmin Descent Mk2(i)/Mk3(i)(S)/G1/G2/X50i', transport: USB storage (MTP). All three families use dc_field_add_string / dc_field_cache, which is the code path affected by the field-cache leak reported upstream. Parsing all three fixtures under an ASAN build with detect_leaks=1 reproduces the leak without the fix applied; with the fix the run is clean. test/run-parse.sh Locates the built examples/dctool, reads the manifest, and runs 'dctool -d "<descriptor>" parse -o /dev/null <blob>' for each entry. Exits non-zero if any invocation returns non-zero. Under the sanitizer build the ASAN abort_on_error flag turns any detected error into a non-zero exit, so a leak or memory error surfaces as a failed parse step. Wire the harness into the sanitizers CI job with: - run: test/run-parse.sh No autotools make-check infrastructure is introduced; the script is a standalone entry point deliberately kept small and reviewable. Dive data is the device owner's own recordings, cleared for redistribution under the library licence. No GPS or personal location data is present in any of the committed blobs. Signed-off-by: Michael Keller <github@ike.ch>
ac3f7d5 to
40e5383
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Fix the harness executable permissions and include parser assets in distribution packaging.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
.github/workflows/build.yml:203
- The PR description says that the sample fixtures and parse harness will be wired into this job in a follow-up, but this step now runs that harness and the same change adds its manifest and fixtures. Please update the description or defer/remove this step so the stated scope matches the CI behavior.
- name: Run parse fixtures
run: test/run-parse.sh
test/run-parse.sh:8
- The new harness and the
test/fixturesinputs are not listed in anyEXTRA_DIST(the rootMakefile.amonly enumerates a few supplemental paths), somake dist/distcheckwill omit this parser coverage from release tarballs. Add the script, manifest, and blobs to the distribution list so the sanitizer coverage can be reproduced from packaged sources.
# Reads test/fixtures/manifest.txt, runs 'dctool parse' over each raw blob
# using the exact model descriptor recorded in the manifest, and exits
# non-zero if any invocation fails.
- Files reviewed: 3/6 changed files
- Comments generated: 1
- Review effort level: Lite
| - run: make | ||
| - run: make distcheck | ||
| - name: Run parse fixtures | ||
| run: test/run-parse.sh |
Signed-off-by: Michael Keller <github@ike.ch>
|
Thanks. I checked the current PR head.
The distribution-packaging observation is correct: the harness, manifest, and fixture blobs were not listed in The PR description is also stale after the follow-up commit added the fixtures and harness. I will update it to describe the current scope. |
There was a problem hiding this comment.
🔵 Needs a closer look
The parser harness is invoked directly but is not executable, so the CI job will fail before parsing fixtures.
Review details
Suppressed comments (1)
.github/workflows/build.yml:203
- This step invokes the newly added shell script directly, but the file is not marked executable in the change, so GitHub Actions will fail with
Permission deniedbefore any fixture is parsed. Invoke it through the shell (or commit the script with executable mode 100755).
run: test/run-parse.sh
- Files reviewed: 4/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add a 'sanitizers' job that builds with -fsanitize=address,undefined
using clang on ubuntu-latest and runs make distcheck under the
sanitizer. ASAN_OPTIONS=detect_leaks=1 and abort_on_error=1 ensure any
detected leak or memory error fails the build immediately.
Full parser coverage under the sanitizer requires sample dump fixtures
and a parse harness to be wired into this job in a follow-up.