Open ghost opened 3 years ago
hefloryd
commented
3 years ago I'm uploading results from clang scan-build and seeing this also when no issues are detected. As a workaround the report is only uploaded if issues were found however I suspect this would mean that fixed issues may never get flagged as closed if there are no more issues.
robertbrignull
commented
3 years ago You're right that not uploading when there are no alerts will lead to alerts being left open erroneously. Code scanning decides that an alert is closed when a SARIF file is uploaded for that tool and ref that doesn't contain the alert.
I would consider what does it mean to upload an empty SARIF file. It means that you ran zero code scanning tools. If you ran a tool and it found zero alerts then there should be an entry in the runs list for that tool and then that run has an empty list of alerts. So even if uploading an empty SARIF file was allowed, then code scanning wouldn't know that clang was run but found no issues. Essentially it would tell you zero information.
I think the real problem here lies with clang or the other tools that's generating a SARIF file with an empty runs list. In the case of @microsoft/sarif-multitool I expect the reports directory doesn't contain any SARIF files so it would be good to investigate why that is.
hefloryd
commented
3 years ago Thanks, that makes sense. Note that I am also using sarif-multitool to convert clang individual files to a merged report. Clang produces one report per analyzed file that looks like this if there are no alerts:
{
"$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
"runs": [
{
"artifacts": [],
"columnKind": "unicodeCodePoints",
"results": [],
"tool": {
"driver": {
"fullName": "clang static analyzer",
"language": "en-US",
"name": "clang",
"rules": [],
"version": "clang version 10.0.0-4ubuntu1 "
}
}
}
],
"version": "2.1.0"
}Running sarif-multitool merge on those files produces:
{
"$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json",
"version": "2.1.0",
"runs": []
}
so it appears information is lost. Perhaps this is an issue with the sarif-mutiltool, there are some bug reports that could be related.
The reason I am merging clang reports is that when these were uploaded using codeql-action/upload-sarif@v1, unfixed alerts would get randomly closed and reappear on the Github Security tab between runs. Merging the reports made them appear consistently. Is there a problem with the sarif output from clang? Here's an example where an alert was found:
{
"$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
"runs": [
{
"artifacts": [
{
"length": 35831,
"location": {
"uri": "file:///home/runner/work/p-net/p-net/src/device/pf_cmrdr.c"
},
"mimeType": "text/plain",
"roles": [
"resultFile"
]
}
],
"columnKind": "unicodeCodePoints",
"results": [
{
"codeFlows": [
{
"threadFlows": [
{
"locations": [
{
"importance": "essential",
"location": {
"message": {
"text": "Value stored to 'ret' is never read"
},
"physicalLocation": {
"artifactLocation": {
"index": 0,
"uri": "file:///home/runner/work/p-net/p-net/src/device/pf_cmrdr.c"
},
"region": {
"endColumn": 7,
"startColumn": 7,
"startLine": 108
}
}
}
}
]
}
]
}
],
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"index": 0,
"uri": "file:///home/runner/work/p-net/p-net/src/device/pf_cmrdr.c"
},
"region": {
"endColumn": 7,
"startColumn": 7,
"startLine": 108
}
}
}
],
"message": {
"text": "Value stored to 'ret' is never read"
},
"ruleId": "deadcode.DeadStores",
"ruleIndex": 0
}
],
"tool": {
"driver": {
"fullName": "clang static analyzer",
"language": "en-US",
"name": "clang",
"rules": [
{
"fullDescription": {
"text": "Check for values stored to variables that are never read afterwards"
},
"helpUri": "https://clang-analyzer.llvm.org/available_checks.html#deadcode.DeadStores",
"id": "deadcode.DeadStores",
"name": "deadcode.DeadStores"
}
],
"version": "clang version 10.0.0-4ubuntu1 "
}
}
}
],
"version": "2.1.0"
}I'll open a new issue for this once it's clear where the problem is.
jsoref
commented
1 year ago I've opened the above bug. It's very clear that the merge agent is buggy. Whether there are other bugs is subject to discussion.
From my perspective, there's a bug in github/codeql as well.
1 item required; only 0 were supplied.Is not remotely human friendly. If you (=codeql team) require that there be at least one runs item, then the error message should clearly mention runs. That it doesn't is a bug.
Also note that while the codeql backend should provide a friendly error akin to the above, this codeql-action should be able to preprocess, detect the same error and report it without hitting the codeql endpoint (saving everyone X api calls from their github api call quota).
It's ok for rare edge cases not to be handled in wrappers like this, but this is not going to be a rare edge case. Especially when a major vendor (👋 Microsoft) is generating bogus outputs of this form and you're actively encouraging people to use your tool.
Expected behaviour: No error
Actual behaviour: codeql-action/upload-sarif@v1 doesn't accept empty sarif
Exemple:
Result:
The sarif file was generated by github/codeql-action/analyze@v1 and sarif-multitool.