Add GitHub Actions CI and SECURITY.md - #6
Open
austek wants to merge 6 commits into
Open
Conversation
Fix downSampleBuffer overflow when numChannels != 1
Allow gcc to be overridden by passed in CC
Change CC variable assignment to conditional
Adds a 4-job CI workflow (.github/workflows/ci.yml): - build-and-test: matrix across ubuntu-latest/macos-latest, make && make test. - strict-and-sanitized: make test with -Werror and ASan+UBSan (clang). - fuzz-smoke: 2-minute bounded run of the existing tests/fuzz_main.c harness via make fuzz, uploading crash-* artifacts on failure. - coverage: make coverage, uploading the resulting .gcov files. Two drive-by fixes were needed to make the sanitized job meaningfully green, both found while building this out: - sonic.c's findSincCoefficient left-shifted a value that can be negative (sincTable has negative entries) -- undefined behavior in C. Replaced with the behaviorally-identical *2. - tests/sonic_api_test.c had an unused-variable warning under -Wall. Also fixes a stack-buffer-overflow in tests/fuzz_main.c's read loop: outBuffer was sized 1024 shorts but sonicReadShortFromStream's maxSamples argument is a per-channel count, so a fuzzer-generated numChannels=2 stream could ask the library to write up to 2048 shorts into it. Confirmed via ASan (stack-buffer-overflow, WRITE of size 2144 in sonicReadShortFromStream). This is a harness bug, not a sonic.c bug -- fix caps the read request by dividing the buffer size by numChannels. Extends .gitignore to cover the build artifacts (test binaries, .gcda/.gcno/.gcov, libsonic*) these CI jobs' make invocations produce. Adds SECURITY.md pointing reporters at GitHub's private security advisory flow, since there's no published security contact email. Ported from waywardgeek/sonic#67, adapted for this fork's build: verified all four jobs' exact commands here directly (make test, make CC=clang CFLAGS="-Wall -Werror -g -fsanitize=address,undefined -fno-omit-frame-pointer" test, make fuzz + a 2-minute run [669653 runs, 0 crashes], make coverage). The sonic_unit_test/coverage source lists and tests/fuzz_main.c/tests/sonic_api_test.c this depends on came from waywardgeek/sonic's "Basic unit tests and fuzzing" commit, already staged for this repo in the separate, still-open PR espeak-ng#2 ("Sync 5 commits from waywardgeek/sonic") -- this branch includes those same prerequisite commits directly so the CI setup is self-contained and doesn't depend on espeak-ng#2 merging first.
2 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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a 4-job CI workflow (
.github/workflows/ci.yml), ported from waywardgeek/sonic#67 and adapted/verified against this fork's actual build:ubuntu-latest/macos-latest,make && make test.make testwith-Werrorand ASan+UBSan (clang).tests/fuzz_main.charness viamake fuzz, uploadingcrash-*artifacts on failure.make coverage, uploading the resulting.gcovfiles.Two drive-by fixes were needed for the sanitized job to be meaningfully green:
findSincCoefficientleft-shifted a value that can be negative (sincTablehas negative entries) — UB in C. Replaced with the behaviorally-identical*2.tests/sonic_api_test.chad an unused-variable warning under-Wall.Also fixes a stack-buffer-overflow in
tests/fuzz_main.c's read loop:outBufferwas sized 1024 shorts butsonicReadShortFromStream'smaxSamplesis a per-channel count, so a fuzzer-generatednumChannels=2stream could ask the library to write up to 2048 shorts into it (confirmed via ASan). Fix caps the read request by dividing the buffer size bynumChannels.Extends
.gitignorefor the build artifacts these jobs produce, and addsSECURITY.mdpointing reporters at GitHub's private security advisory flow.Important: overlaps with #2
This fork's
masterdoesn't yet have the roottest/fuzz/coverageMakefile targets ortests/fuzz_main.c/tests/sonic_api_test.c— those exist here only via the 3 commits already staged in #2 ("Sync 5 commits from waywardgeek/sonic"). This branch includes those same 3 commits directly, so the CI setup is self-contained and its jobs actually run rather than immediately failing on missing targets. That means this PR's diff currently overlaps with #2's.If #2 lands first, this branch can be rebased down to just the CI-specific diff — happy to do that once you've decided the order.
Test plan
All four jobs' exact commands verified locally on this branch:
make && make test— passes, no warningsmake CC=clang CFLAGS="-Wall -Werror -g -fsanitize=address,undefined -fno-omit-frame-pointer" test— passes cleanmake fuzz+./fuzz_sonic -max_total_time=120— 669,653 runs, 0 crashes, exit 0make coverage— 74.21% line coverage,.gcovgenerated