Fix: Error BA2006: '...' was compiled with one or more modules which were not built using minimum required tool versions #533
Conversation
shaopeng-gh
requested review from
eddynaka and
michaelcfanning
as code owners
| { | ||
| // TODO: https://github.com/Microsoft/binskim/issues/114 | ||
| continue; | ||
| } |
There was a problem hiding this comment.
In changeset
#344
We have this check and skip in BA2011, I think we should have it for BA2006 as well.
BA2006 had this and were removed:
if (omDetails.WellKnownCompiler != WellKnownCompilers.MicrosoftNativeCompiler)
{
continue;
}
The reason is currently the only know compiler in the enum WellKnownCompilers are Microsoft compilers, and the version to compare are only Microsoft VS versions. We should remove this check after we also have the codes and determined min versions for all other compilers.
#Resolved
| @@ -772,13 +772,10 @@ | |||
| "rules": [ | |||
| { | |||
| "id": "BA2001", | |||
| "name": "LoadImageAboveFourGigabyteAddress", | |||
There was a problem hiding this comment.
thanks for the feedback, working on
- update SDK to have those re-ordering reverted.
- review
- change only one ordering so that name is right after id.
There was a problem hiding this comment.
I have created a fix in SDK
microsoft/sarif-sdk#2420 (please review together)
and then point binskim to that branch and run re-baseline to verify the fix.
Now this PR is clearer to review.
After all looks good to you, we should get the desired ordering from you and I will update both PR with that order.(you mentioned one name field)
skip those re-ordering changes, clang version 13 is latest version should be supported, after this fix now it will not compare {13.0.0.0} to VS Version(17, 0, 65501, 17013) and report it. In reply to: 996301745 In reply to: 996301745 Refers to: src/Test.FunctionalTests.BinSkim.Driver/BaselineTestsData/Expected/clangcl.pe.cpp.codeview.exe.sarif:431 in d845a11. [](commit_id = d845a11, deletion_comment = False) |
| if (omDetails.WellKnownCompiler != WellKnownCompilers.MicrosoftC | ||
| && omDetails.WellKnownCompiler != WellKnownCompilers.MicrosoftCxx) | ||
| { | ||
| // TODO: https://github.com/Microsoft/binskim/issues/114 |
There was a problem hiding this comment.
TODO: MikeFan (1/6/2022)
We need to take a step back and comprehensively review our compiler/language support.
#114 #Resolved
Comment this whole block, using C /* case Language.MASM */ In reply to: 1006978150 In reply to: 1006978150 In reply to: 1006978150 In reply to: 1006978150 Refers to: src/BinSkim.Rules/PERules/BA2006.BuildWithSecureTools.cs:142 in af6ff95. [](commit_id = af6ff95, deletion_comment = False) |
| @@ -50,6 +50,7 @@ | |||
| } | |||
| } | |||
| ], | |||
| "automationDetails": {}, | |||
There was a problem hiding this comment.
This is valid SARIF, automationDetails has no required members, but it's inefficient, we don't need to serialize an empty object. In general, we don't want our log file persistence to change version-over-version. #Resolved
There was a problem hiding this comment.
fixed together in the SDK PR, link in the other comment with json node ordering.
fixed In reply to: 1006978150 Refers to: src/BinSkim.Rules/PERules/BA2006.BuildWithSecureTools.cs:142 in af6ff95. [](commit_id = af6ff95, deletion_comment = False) |
… check if fix works 2. now we can review the actual change without noise
|
Is there supposed to be a sarif file for the clangcl.pe.c.codeview.exe in the expected folder? |
|
good question, I think this was the similar thing that confused Michael in the review :) the cpp one was already in the baseline, I added it also to the Functional folder because I think it benefit that if future regression we might see a very clear test failure with the rule id. related info: In reply to: 1007837347 |
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| * BUGFIX: Fix Error BA2006: '...' was compiled with one or more modules which were not built using minimum required tool versions [533](https://github.com/microsoft/binskim/pull/533) | |||
| @@ -116,6 +116,15 @@ public override void AnalyzePortableExecutableAndPdb(BinaryAnalyzerContext conte | |||
| Symbol om = omView.Value; | |||
| ObjectModuleDetails omDetails = om.GetObjectModuleDetails(); | |||
|
|
|||
| if (omDetails.WellKnownCompiler != WellKnownCompilers.MicrosoftC | |||
| && omDetails.WellKnownCompiler != WellKnownCompilers.MicrosoftCxx) | |||
There was a problem hiding this comment.
should this be:
omDetails.WellKnownCompiler != WellKnownCompilers.MicrosoftC &&
omDetails.WellKnownCompiler != WellKnownCompilers.MicrosoftCxx
``` #Resolved
|
Just remembering that we must review the sarif-sdk change before reviewing this. Because we cannot merge this without merging the other. In reply to: 1007848170 |
|
Re-open the PR and point the submodule to main. In reply to: 1007979314 |
Description:
reported issue with
error BA2006: '...' was compiled with one or more modules which were not built using minimum required tool versions
this error happens in new version 1.9.0
tested it passes in old version 1.7.2
Fix:
related to changeset #344
location: file BA2006.BuildWithSecureTools.cs line 104
we should add a skip if the compiler is not the ones we currently support those we can check min version.
After we widen the support we should remove this skip.