* FPS: BA2004.EnableSecureSourceCodeHashing now will no longer generate false positives for UWP App regarding dummy.obj.
#987
Conversation
…rate false positives for UWP App regarding `dummy.obj`.
shaopeng-gh
requested review from
michaelcfanning and
marmegh
as code owners
| @@ -17,6 +17,7 @@ | |||
| ## UNRELEASED | |||
| * DEP: Update `Sarif.Sdk` submodule from [bc8cb57 to fd6e615](https://github.com/microsoft/sarif-sdk/compare/bc8cb57...fd6e615). Reference [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/fd6e615/ReleaseHistory.md). | |||
| * NEW: Add `--disable-telemetry` argument to disable telemetry collection. | |||
| * FPS: `BA2004.EnableSecureSourceCodeHashing` will now no longer generate false positives for Universal Windows Platform (UWP) app regarding `dummy.obj`. [#976](https://github.com/microsoft/binskim/pull/976) | |||
There was a problem hiding this comment.
nit: FPS should come after BUG in the standard order.
There was a problem hiding this comment.
@stacywray I don't think that list is ordered. Alphabetical ordering makes sense, but why should BUG be prioritized over FPS otherwise?
| @@ -297,5 +302,8 @@ public IEnumerable<IOption> GetOptions() | |||
| // RequiredCompilerWarnings, | |||
| }.ToImmutableArray(); | |||
| } | |||
|
|
|||
| internal static bool IsLikelyUwpDummyObj(Language language, string library, string name) => | |||
| language == Language.MASM && library?.Equals(name) == true && library == @"c:\dummy.obj"; | |||
There was a problem hiding this comment.
string.Equals call should specify StringComparison, presumably .Ordinal.
Drop "== true"
Don't use == comparison with strings (language and library). Use .Equals with a comparison specified.
There was a problem hiding this comment.
fixed.
can not Drop "== true" because there is null check. But I moved it out now so more clear.
| @@ -297,5 +302,11 @@ public IEnumerable<IOption> GetOptions() | |||
| // RequiredCompilerWarnings, | |||
| }.ToImmutableArray(); | |||
| } | |||
|
|
|||
| internal static bool IsLikelyUwpDummyObj(Language language, string library, string name) => | |||
There was a problem hiding this comment.
Thanks, this is so much more readable with the improved wrapping.
If you want to change the method name to IsUwpDummyObj --well, I think it's more than 'likely' given all these checks. :) (Or 'Presumed' if you want to be super cautious.) Up to you though. If you decide to make that change let me know and I'll restamp quickly.
| @@ -17,6 +17,7 @@ | |||
| ## UNRELEASED | |||
| * DEP: Update `Sarif.Sdk` submodule from [bc8cb57 to fd6e615](https://github.com/microsoft/sarif-sdk/compare/bc8cb57...fd6e615). Reference [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/fd6e615/ReleaseHistory.md). | |||
| * NEW: Add `--disable-telemetry` argument to disable telemetry collection. | |||
| * FPS: `BA2004.EnableSecureSourceCodeHashing` will now no longer generate false positives for Universal Windows Platform (UWP) app regarding `dummy.obj`. [#976](https://github.com/microsoft/binskim/pull/976) | |||
There was a problem hiding this comment.
@stacywray I don't think that list is ordered. Alphabetical ordering makes sense, but why should BUG be prioritized over FPS otherwise?
| public void IsLikelyUwpDummyObjTests() | ||
| { | ||
| EnableSecureSourceCodeHashing.IsLikelyUwpDummyObj(Language.MASM, @"c:\dummy.obj", @"c:\dummy.obj").Should().BeTrue(); | ||
| EnableSecureSourceCodeHashing.IsLikelyUwpDummyObj(Language.MASM, @"d:\dummy.obj", @"d:\dummy.obj").Should().BeFalse(); |
There was a problem hiding this comment.
@shaopeng-gh To make sure I understand this correctly, d:\dummy.obj should result in false? The drive letter must be c?
There was a problem hiding this comment.
yes I believe the official one should have the path fixed as c:\dummy.obj
BA2004.EnableSecureSourceCodeHashingnow will no longer generate false positives for UWP App regardingdummy.obj.Please see details in #983