ci: scope CodeQL to first-party code - #413
Open
xanimo wants to merge 1 commit into
Open
Conversation
Fifteen of the forty-eight open CodeQL alerts are in vendored code -- 7 in secp256k1 and 8 in lodepng. Nobody is going to act on the secp256k1 ones: a finding there is fixed by taking a newer upstream, not by editing our copy. They dilute the list that does need attention, which is how a security tab stops getting read. Worth being precise about why this needs a config file at all. ql.yml already has a paths-ignore, but that is under `on:` -- it decides which file changes trigger a run. Restricting what CodeQL analyses is a different setting and lives in a config file passed to github/codeql-action/init. The exclusions mirror the -i list in cppcheck.yml deliberately rather than by coincidence. Someone already reasoned about which of this tree's vendored code is ours to maintain; agreeing with that decision keeps the two analysers describing the same codebase, and means a finding either matters in both or neither. Note what is not excluded. src/png.c (lodepng) and src/qr.c (qrcodegen) are single-file libraries vendored into the tree and, in practice, maintained here: qr.c has already needed a local fix for table indexing guarded only by an assert, which compiles out under NDEBUG. Excluding those would have hidden a real bug, so they stay in scope. cppcheck excludes neither, and this matches. Expected effect: 48 alerts to 41, all of them in code this project can actually change. The 8 lodepng alerts remain, and have been triaged -- the invalid-pointer-deref at png.c:908 is an OOM path present verbatim in current upstream lodepng, on an encoder we drive with our own data, and libdogecoin never calls the PNG decoder at all. One caveat for review: paths-ignore is supported for compiled languages but does not behave identically to interpreted ones, since CodeQL sees what the build compiles. If secp256k1 alerts survive the next run, the exclusion needs to happen at build time instead, and that is worth watching on the first analysis after merge rather than assuming.
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.
15 of the 48 open CodeQL alerts are in vendored code — 7 in secp256k1, 8 in lodepng.
Nobody is going to act on the secp256k1 ones. A finding there is fixed by taking a newer upstream, not by editing our copy. They dilute the list that does need attention, which is how a security tab stops getting read.
Why this needs a config file
ql.ymlalready has apaths-ignore— but it's underon:, where it decides which file changes trigger a run. Restricting what CodeQL analyses is a different setting entirely, and lives in a config file passed togithub/codeql-action/init. Easy to conflate; they're unrelated.Why these paths
The exclusions mirror the
-ilist incppcheck.ymldeliberately, not coincidentally. Someone already reasoned about which vendored code in this tree is ours to maintain. Agreeing with that decision keeps both analysers describing the same codebase — so a finding either matters in both or neither.What is deliberately not excluded
src/png.c(lodepng) andsrc/qr.c(qrcodegen) are single-file libraries vendored into the tree and, in practice, maintained here.qr.chas already needed a local fix — table indexing guarded only by anassert, which compiles out underNDEBUG(#401).Excluding those would have hidden a real bug. They stay in scope, matching cppcheck, which excludes neither.
Expected effect
48 alerts → 41, all in code this project can actually change.
The 8 lodepng alerts remain and have been triaged: the
invalid-pointer-derefatpng.c:908is an OOM path present verbatim in current upstream lodepng (20260119), on an encoder we drive with our own data — and libdogecoin never calls the PNG decoder at all, onlylodepng_encode24/lodepng_save_file/lodepng_error_text.One caveat for review
paths-ignoreis supported for compiled languages but doesn't behave identically to interpreted ones, since CodeQL sees what the build compiles. If secp256k1 alerts survive the next run, the exclusion needs to happen at build time instead. Worth watching on the first analysis after merge rather than assuming it worked.