Repository navigation
feat(sarif): describe the rules and rate each finding's confidence - #31
Merged
Merged
Conversation
The SARIF log has no tool.driver.rules: a consumer such as GitHub code scanning or CoreTrace's merged log can only show the bare rule id. Each of the eleven rules the analyzer reports must be listed once, with a title other than its id and a description. Refs #29
A fault observed while the program ran is certain (high); a vtable type mismatch may be a legitimate cast (medium); an unrecognized vtable report says little (low). The SARIF result must carry it as the confidence property, as CoreTrace's merged log spells it. Refs #30
DescribeRule gives a title and a description for each of the eleven rules ParseFindings reports, next to where they are defined, for a SARIF consumer to explain their findings. Refs #29
The driver had no rules, so a consumer could only show a finding's bare rule id. tool.driver.rules now lists each rule the run reports once, in the order first reported, with its title as shortDescription and its description as fullDescription. The exact logs the tests compare against list those rules, and an empty run an empty rules array. Refs #29
A fault observed while the program ran is certain (high); a vtable type mismatch may be a legitimate cast (medium); an unrecognized vtable report says little (low). Refs #30
Each result carries its rule's confidence as the property confidence, low, medium or high, as CoreTrace's merged log spells it. The exact logs the tests compare against carry it next to the CWE. Refs #30
SizzleUnrlsd
added
enhancement
A program built with -fno-rtti has no typeinfo in its vtables, so the runtime logs "missing typeinfo" for valid virtual calls. The parser mapped it to vtable-corrupted, an error rated high confidence. Move it last in the vtable rules as a vtable-diagnostic warning, so any other warning of the same box still decides the rule, and drop "type information" from the vtable-corrupted description.
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.
Closes #29. Closes #30.
The SARIF log said nothing about its rules and nothing about how sure a finding is. A consumer such as GitHub code scanning, an IDE or CoreTrace's merged log could only show a finding's bare rule id.
Changes
DescribeRulegives a title and a description for each of the eleven rulesParseFindingsreports. The texts sit next to where the rules are defined, insrc/findings.cpp.tool.driver.ruleslists each rule a run reports once, in the order first reported, withshortDescription(the title) andfullDescription(the description). An empty run has an emptyrulesarray.properties.confidence, spelled as CoreTrace's merged log spells it:highfor a fault observed while the program ran (bounds, use after free, double free, leak, and the vtable faults);mediumforvtable-type-mismatch, which may be a legitimate cast;lowforvtable-diagnostic, the fallback for a vtable report without a more specific rule.CoreTrace reads this log. With CoreTrace/coretrace#208, it already keeps a rule's description, so nothing else is needed on its side for #29. Reading the confidence and the allocation site is CoreTrace/coretrace#209.
Tests
Each test was committed before its implementation and failed for the expected reason:
sarif_unit: each of the eleven rules is listed once, with a title other than its id and a description. The confidence is checked for each level.sarif_unitalready compared against now list their rule and carry the confidence: they changed with the format.Results:
sweep_runstimed out once under a parallelctest -j 8(156 s) and passed alone (17 s): it runs every fixture with a 10 s limit.scripts/format-check.shis clean.