Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MultithreadedAnalyzeCommandBase artifacts generation and enforcing JSON properties ordering #555

Merged
merged 2 commits into from
Feb 1, 2022

Conversation

eddynaka
Copy link
Contributor

@eddynaka eddynaka commented Feb 1, 2022

Description

The previous version of the sarif-sdk had a bug that was generating an artifact for each file that was analyzed. This update will guarantee that we will only generate an artifact if we have a result.

The sarif-sdk submodule change is also enforcing a specific ordering of the name/helpuri in the reportingDescriptor. This is expected since we want the name to be near the id.

Test

Checked all rebaselined files and only found two unexpected changes related to the artifacts. The reason we have those is that we are not producing any results, so the artifacts are getting dropped from the SARIF log.

@@ -38,18 +38,6 @@
"executionSuccessful": false
}
],
"artifacts": [
Copy link
Contributor Author

@eddynaka eddynaka Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

artifacts

we will only emit an artifact if we have a result. This was part of the sarif-sdk change to improve the file size generated when utilizing the MultithreadedAnalyzeCommandBase class:
microsoft/sarif-sdk#2433

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be an artifact entry for every artifact that is explicitly referenced. There's not a good reason to arbitrarily restrict this to results. We've lost important information here.

@eddynaka
Copy link
Contributor Author

eddynaka commented Feb 1, 2022

  ],

we will only emit an artifact if we have a result. This was part of the sarif-sdk change to improve the file size generated when utilizing the MultithreadedAnalyzeCommandBase class:
microsoft/sarif-sdk#2433


Refers to: src/Test.FunctionalTests.BinSkim.Driver/BaselineTestsData/Expected/gcc.stack_protector.a.sarif:52 in 64f53be. [](commit_id = 64f53be, deletion_comment = True)

@eddynaka eddynaka changed the title Updating SARIF-SDK submodules Fix MultithreadedAnalyzeCommandBase artifacts generation and enforcing JSON properties ordering Feb 1, 2022
@eddynaka eddynaka changed the title Fix MultithreadedAnalyzeCommandBase artifacts generation and enforcing JSON properties ordering Fix MultithreadedAnalyzeCommandBase artifacts generation and enforcing JSON properties ordering Feb 1, 2022
@@ -772,10 +772,10 @@
"rules": [
{
"id": "BA2001",
"name": "LoadImageAboveFourGigabyteAddress",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name

with the sarif-sdk change, we are enforcing a specific order where name will be close to id and helpuri will be near the end.

Reviewed all files and only two has unexpected changes that are commented as well.

@eddynaka eddynaka marked this pull request as ready for review February 1, 2022 02:46
@@ -1,5 +1,9 @@
# BinSkim Release History

## Unreleased

* BUGFIX: Fix `MultithreadedAnalyzeCommandBase` artifacts generation and enforcing JSON properties ordering. [#555](https://github.com/microsoft/binskim/pull/555)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MultithreadedAnalyzeCommandBase

I'd document this isn't a BinSkim fix, it's an SDK update.

Update SARIF-SDK in order to fix `blah blah

@michaelcfanning
Copy link
Member

  "artifacts": [

this is a bug, it shouldn't have gone away. Instead, the change should be that we see the addition of the artifact index.


Refers to: src/Test.FunctionalTests.BinSkim.Driver/BaselineTestsData/Expected/clang.stack_protector.a.sarif:41 in 3fa6d69. [](commit_id = 3fa6d69, deletion_comment = True)

Copy link
Member

@michaelcfanning michaelcfanning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@michaelcfanning michaelcfanning merged commit 99ac09e into main Feb 1, 2022
@michaelcfanning michaelcfanning deleted the users/ednakamu/updating-sarif-sdk-submodules branch February 1, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants