fix: saturate digit parsing, check gmtime_r, pin clang-tidy (0.4.2) - #17
Merged
Conversation
Chasing the intermittent clang-tidy warning turned up undefined behavior
in the parser.
The flake itself was not a race. run-clang-tidy resolves `clang-tidy` from
PATH, and this machine has two: LLVM 18 from the workspace environment and
22 from the distribution. Whether the warning appeared depended on whether
the shell had sourced the environment. Version 18 reported it; 22 did not.
The warning was a false positive. It flagged `currentScreenshot = {}` on
the line after `std::move(currentScreenshot)`, which is how a moved-from
object is restored. But 18 was also emitting twenty clang-diagnostic-errors
against this host's libstdc++ and stopping early, so it was reporting from
a half-parsed translation unit. Pinning to a version that parses cleanly
replaced that false positive with a real defect.
Undefined behavior, both confirmed with UBSan on the real code path:
AppStreamParser.cpp:57 runtime error: signed integer overflow:
999999999999999999 * 10 cannot be represented in type 'long long int'
AppStreamParser.cpp:37 runtime error: signed integer overflow:
999999999 * 10 cannot be represented in type 'int'
Reached from `<release timestamp="999...">` and `<icon width="999...">`.
Those are attribute values in a catalog downloaded over the network, and
the same helper also parses priority, image and video dimensions, and icon
scale. Accumulation now saturates. CI already runs UBSan, but no test fed
an oversized number, so nothing fired; three regression tests do now.
gmtime_r returns null for a time_t it cannot represent, which a saturated
epoch reaches. The return value was ignored, leaving the std::tm zero
initialized and silently yielding 1900-01-01T00:00:00Z. It now yields no
timestamp.
scripts/tidy.sh pins clang-tidy as scripts/format.sh pins clang-format, and
the CI job is no longer advisory. It invokes clang-tidy directly, because
the pip package ships no run-clang-tidy and run-clang-tidy would resolve
the binary from PATH — the exact drift being fixed. It passes
--warnings-as-errors: plain clang-tidy exits 0 even when it reports
diagnostics, so the first version of this gate passed with the gmtime_r
defect still present. Verified the gate now exits non-zero with that defect
reintroduced and zero once restored.
Verified: shellcheck clean; UBSan reports no errors on either hostile input
after the fix; 152/152 C++ (three new) and 45/45 Dart; clang-tidy clean at
the pinned version; clang-format run last; dart analyze --fatal-infos clean.
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
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.
Chasing the intermittent clang-tidy warning turned up undefined behavior in the parser. 0.4.2.
The flake was not a race
run-clang-tidyresolvesclang-tidyfromPATH, and this machine has two: LLVM 18 from the workspace environment and 22 from the distribution. Whether the warning appeared depended on whether the shell had sourcedsetup_env.sh. 18 reported it, 22 did not — perfect correlation across every run.The warning itself was a false positive: it flagged
currentScreenshot = {}on the line afterstd::move(currentScreenshot), which is precisely how a moved-from object is restored. But 18 was also emitting 20clang-diagnostic-errors against this host's libstdc++ and stopping with "too many errors emitted", so it was reporting from a half-parsed translation unit.Pinning to a version that parses cleanly replaced that false positive with a real defect.
Undefined behavior on untrusted input
Both confirmed with UBSan on the real code path, driving the actual parser over a crafted catalog:
Reached from
<release timestamp="999...">and<icon width="999...">— attribute values in a catalog downloaded over the network. The same helper also parsespriority, image/video dimensions, and icon scale.Accumulation now saturates, so a hostile value is clamped rather than wrapped. CI already runs UBSan across a Debug/Release x asan/ubsan matrix, but no test fed an oversized number, so nothing ever fired. Three regression tests do now.
gmtime_rreturns null for atime_tit cannot represent, which a saturated epoch reaches. The return was ignored, leaving thestd::tmzero-initialized and silently producing1900-01-01T00:00:00Z. It now yields no timestamp.Pinning clang-tidy
scripts/tidy.shpins clang-tidy asscripts/format.shpins clang-format, and the CI job is no longercontinue-on-error.Two things worth calling out in review, both found while testing the gate rather than assumed:
clang-tidydirectly instead of viarun-clang-tidy. The pip package ships norun-clang-tidy, andrun-clang-tidyresolves the binary fromPATH— the exact drift being fixed here.--warnings-as-errors. Plainclang-tidyexits 0 even when it reports diagnostics, so the first version of this gate passed with thegmtime_rdefect still present. A check that cannot fail is worse than no check, because it reads as coverage.Verified by reintroducing the defect: gate exits 123. Restored: exits 0.
use-after-movefalse positivegmtime_rdefectVerification
shellcheck clean; UBSan reports no errors on either hostile input after the fix; 152/152 C++ (three new) and 45/45 Dart; clang-tidy clean at the pinned version; clang-format run last;
dart analyze --fatal-infosclean;dart pub publish --dry-run0 warnings.