Repository navigation
feat(sarif): carry every tool's related locations and confidence to the merged log - #210
Merged
Merged
Conversation
…h the merged log The fake runtime analyzer now prints what the real one does for a heap overflow: the allocation as a related location, 'allocated here', and the confidence. The merged SARIF log must keep both, and link the message to the allocation. Refs #209
… model diagnosticsFromSarif read neither relatedLocations nor properties.confidence: the runtime analyzer's allocation site and the Python analyzer's sink were lost. They must reach the model, the confidence in any case (the concurrency analyzer writes HIGH) and another value ignored, a related location that is not an object skipped; the message must come without the embedded links to them. Refs #209
…ed locations GitHub code scanning links to a related location only when the message names it. Each related location must have its id and the message end with an embedded link to each, its brackets escaped; a message without related locations, and the fingerprints, must not change. Refs #209
diagnosticsFromSarif, which flawfinder and the Python and runtime analyzers go through, now reads each result's relatedLocations, their place and message, and its confidence property, low, medium or high in any case (the concurrency analyzer writes HIGH), another value ignored. The message is read without its embedded links: [text](id) becomes its text and escaped brackets and backslashes themselves, so that the text report reads cleanly; the merged log links it again. Refs #209
GitHub code scanning links a result to a related location only when the message names it. Each related location of the merged log now has its id, and the message ends with an embedded link [text](id) to each, its own brackets and backslashes escaped. A result without related locations keeps its message, and the coretrace/v1 fingerprints, computed from the model's message, do not change. Refs #209
…the changelog Refs #209
…ation is The Python bridge resolved a finding's main location from the project root and spelled it as its input, but kept a related location's uri as the analyzer wrote it, relative to that root: for a project outside the working directory, the merged log's link opened another file. A related location must be resolved from the project root and spelled as the main location; respellFindings, which cppcheck, tscancode and the runtime analyzer go through, must respell related locations too. Refs #209
respellFindings spelled a finding's file as its input but left its related locations as the tool wrote them, so that a runtime analyzer allocation site in ./-F.c could be spelled -F.c next to its finding. It now respells each related location's file the same way. Refs #209
The Python analyzer writes every location relative to the project root it was given (uriBaseId SRCROOT). The bridge resolved the main location from that root and spelled it as its input, but kept a related location's uri as written: for a project outside the working directory, the merged log's link to the sink opened another file. Each related location is now resolved from the root and spelled as InputPaths spells the main location. Refs #209
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.
Part of #209.
The merged SARIF log kept a finding's related locations and confidence only for the concurrency analyzer, which is read in memory. The SARIF reader, which flawfinder and the Python and runtime analyzers go through, dropped them. The runtime analyzer's allocation site ("allocated here") was lost. The Python analyzer's sink and confidence, now in its SARIF (CoreTrace/coretrace-python-analyzer#274), would have been lost too.
Changes
Reading:
diagnosticsFromSarifnow reads:relatedLocations, with their place and message;properties.confidence,low,mediumorhighin any case (the concurrency analyzer's own SARIF writesHIGH); another value is ignored.The message is read without its embedded links:
[text](id)becomes its text, and escaped brackets and backslashes become themselves, so that the text report reads cleanly.Rendering: each related location of the merged log has its
id, and the message ends with an embedded link[text](id)to each, with its own brackets escaped. GitHub code scanning then shows the places as links. This applies to every tool, the concurrency analyzer's data races included.Unchanged: a message without related locations, and the
coretrace/v1fingerprints, which are computed from the model's message.Not in this PR
The stack analyzer's confidence is a number in [0, 1], and its bridge still drops it. How it maps to a level is decided in CoreTrace/coretrace-stack-analyzer#244, so #209 stays open for that part.
Tests
Each test was committed before its implementation and failed for the expected reason:
tool_diagnostics_tests, reading:tool_diagnostics_tests, rendering:ctrace_sarif_keeps_the_runtime_allocation_site: the fake runtime analyzer now prints, as the real one does, the allocation of a heap overflow and its confidence. The merged log keeps both, and its message links to the allocation.Results: