Add new PE CV_CFL_LANG language code for ALIASOBJ and Rust #530

shaopeng-gh · 2021-12-15T04:13:00Z

Description:

Add new PE CV_CFL_LANG language code for ALIASOBJ and RUST.

L"Objective-C",                    // CV_CFL_OBJC
L"Objective-C++",                  // CV_CFL_OBJCXX
L"Swift",                          // CV_CFL_SWIFT
L"ALIASOBJ",                       // CV_CFL_ALIASOBJ
L"Rust",                           // CV_CFL_RUST

…/microsoft/binskim into users/shaopeng-gh/addnewlangcode

eddynaka · 2021-12-15T09:41:55Z

...Test.FunctionalTests.BinSkim.Driver/BaselineTestsData/Expected/Binskim.win-x64.RTR.dll.sarif

-              "helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2002DoNotIncorporateVulnerableDependencies",
-              "help": {
-                "text": "Binaries should not take dependencies on code with known security vulnerabilities."
-              },


can you verify what happened?
this should not happen. #Resolved

I ran the re-baseline, will see why there is this change. The order of the json node changed.

found the reason,
not related to our change,
was the result of changing the AnalyzeCommand to use MultithreadedAnalyzeCommandBase. I did verified if change it back this will disappear.
f87857a

we have fixed the sdk, got the latest from main and re-ran the baseline, now these order changes are gone.

eddynaka

marmegh · 2022-01-25T17:26:17Z

src/Test.UnitTests.BinaryParsers/PEBinary/PEBinaryTests.cs

@@ -92,5 +109,27 @@ public void PEBinary_TryLoadPdbFromSymbolFolder()
                peBinary.PdbParseException.Should().BeNull();
            }
        }
+
+        private static bool IfContainsLanguagueCode(string fileName, Language language)


IfContainsLanguagueCode

This name is confusing. I would expect either ContainsLanguageCode or IfContainsLanguageCode for a method name. #Resolved

In general, you can simply use 'Contains' for these, we generally don't burn 'If' into this sort of boolean test.

Look at the way the code reads, do you see the duplication of 'if'?

if (IfContainsLanguageCode...

if (IfContainsLanguageCode...
---- thanks for the example. fixed.

marmegh · 2022-01-25T17:34:53Z

src/BinaryParsers/PEBinary/ProgramDatabase/ObjectModuleDetails.cs

        HLSL = 0x10, // High Level Shader Language
        ObjectiveC = 0x11,
        ObjectiveCxx = 0x12,
        Swift = 0x13,
+        ALIASOBJ = 0x14, // module generated by the aliasobj tool
+        Rust = 0x15,


What does adding these accomplish? Does it prevent an error if we come across one of these files while trying to analyze a repo? Does it unblock future work? What does the customer get out of this addition? #Resolved

the only thing it is for is when the compilers Clang/GCC starts to emit the 0x15 value when the program is Rust program,
our tool will be able to parse it as "Rust" and show correctly.

michaelcfanning · 2022-01-25T17:43:21Z

src/ReleaseHistory.md

@@ -1,5 +1,9 @@
 # BinSkim Release History

+## Unreleased
+
+* FEATURE: Add new PE CV_CFL_LANG language code for ALIASOBJ and Rust. [530](https://github.com/microsoft/binskim/pull/530)


FEATURE

I don't think FEATURE is buying us much in this document. This release note item doesn't describe the additional value the change is providing. #Resolved

agree, changed to the other one BUGFIX

michaelcfanning · 2022-01-25T17:43:48Z

src/ReleaseHistory.md

@@ -1,5 +1,9 @@
 # BinSkim Release History

+## Unreleased
+
+* FEATURE: Add new PE CV_CFL_LANG language code for ALIASOBJ and Rust. [530](https://github.com/microsoft/binskim/pull/530)


CV_CFL_LANG

This file does not do a good job of using markdown conventions of properly denoting code items, for example, by encapsulating them in backticks. #Resolved

added backticks.

michaelcfanning · 2022-01-25T17:44:40Z

src/BinaryParsers/PEBinary/ProgramDatabase/ObjectModuleDetails.cs

-        CVTRES = 0x08,
-        CVTPGD = 0x09,
+        LINK = 0x07, // linker-generated module
+        CVTRES = 0x08, // resource module converted with CVTRES tool


resource

All comments need to start with an upper cased word.

Please prefer adding periods to the end of sentences. #Resolved

michaelcfanning · 2022-01-25T17:46:02Z

src/BinaryParsers/PEBinary/ProgramDatabase/ObjectModuleDetails.cs

        Java = 0x0D,
        JScript = 0x0E,
-        MSIL = 0x0F,
+        MSIL = 0x0F, // Microsoft Intermediate Language (MSIL), possibly a result of using the /LTCG (Link-time Code Generation) switch


LTCG

Huh? The native whole code optimization feature produces MSIL? Can you show me the source of your information? #Resolved

https://github.com/MicrosoftDocs/visualstudio-docs/blob/main/docs/debugger/debug-interface-access/cv-cfl-lang.md

michaelcfanning · 2022-01-25T17:46:45Z

src/BinaryParsers/PEBinary/ProgramDatabase/ObjectModuleDetails.cs

-        LINK = 0x07,
-        CVTRES = 0x08,
-        CVTPGD = 0x09,
+        LINK = 0x07, // linker-generated module


linker

What's the additional value of this comment? #Resolved

agree some of the comments does not add value at all, removed all to be consistent

michaelcfanning · 2022-01-25T17:46:55Z

src/BinaryParsers/PEBinary/ProgramDatabase/ObjectModuleDetails.cs

        HLSL = 0x10, // High Level Shader Language
        ObjectiveC = 0x11,
        ObjectiveCxx = 0x12,
        Swift = 0x13,
+        ALIASOBJ = 0x14, // module generated by the aliasobj tool


module

What's the additional value of your comment? #Resolved

agree some of the comments does not add value at all, removed all to be consistent

michaelcfanning · 2022-01-25T17:48:13Z

src/BinaryParsers/PEBinary/ProgramDatabase/ObjectModuleDetails.cs

        HLSL = 0x10, // High Level Shader Language
        ObjectiveC = 0x11,
        ObjectiveCxx = 0x12,
        Swift = 0x13,
+        ALIASOBJ = 0x14, // module generated by the aliasobj tool
+        Rust = 0x15,


Rust

Note that for this language, you didn't say 'Module generated by Rust.' So, in general, please be consistent in your approach. I don't think this comment would provide much value, by the way. If your comment doesn't add useful additional information, we are just cluttering the code. #Resolved

OK, so I think what you've done is to partly port the documentation for enum values over. My feedback to you is: don't do that in an inconsistent way.

You have two choices:

Author useful comments in places where you think a comment provides additional information over the language name. Is anyone confused by what 'Pascal' means here, for example?

OR: port all the comments from the documentation, so that every member is consistently documented precisely as it is online.

i.e., stay out of the middle, which is to inconsistently apply exact language in some cases, partial documentation in others, and to ignore others entirely.

V_CFL_C Application language is C.

CV_CFL_CXX Application language is C++.

CV_CFL_FORTRAN Application language is FORTRAN.

CV_CFL_MASM Application language is Microsoft Macro Assembler.

CV_CFL_PASCAL Application language is Pascal.

CV_CFL_BASIC Application language is BASIC.

CV_CFL_COBOL Application language is COBOL.

CV_CFL_LINK Application is a linker-generated module.

CV_CFL_CVTRES Application is a resource module converted with CVTRES tool.

CV_CFL_CVTPGD Application is a POGO optimized module generated with CVTPGD tool.

CV_CFL_CSHARP Application language is C#.

CV_CFL_VB Application language is Visual Basic.

CV_CFL_ILASM Application language is intermediate language assembly (that is, Common Language Runtime (CLR) assembly).

CV_CFL_JAVA Application language is Java.

CV_CFL_JSCRIPT Application language is Jscript.

CV_CFL_MSIL Application language is an unknown Microsoft Intermediate Language (MSIL), possibly a result of using the /LTCG (Link-time Code Generation) switch.

CV_CFL_HLSL Application language is High Level Shader Language.

CV_CFL_OBJC Application language is Objective-C.

CV_CFL_OBJCXX Application language is Objective-C++.

CV_CFL_SWIFT Application language is Swift.

CV_CFL_ALIASOBJ Application is a module generated by the aliasobj tool.

CV_CFL_RUST Application language is Rust.

stay out of the middle --- agree

and also agree some of the comments does not add value at all, so removed all to be consistent.

michaelcfanning · 2022-01-25T17:49:12Z

"$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json",

How did you generate your test binaries? Why aren't we checking in build scripts?

It is not clear to me what compiler settings you are using. If someone needs to update your test binaries in the future, they will not be able to leverage your approach.

In general, please try to help engineers in the future who will update your changes.

In reply to: 1021451914

Refers to: src/Test.FunctionalTests.BinSkim.Driver/BaselineTestsData/Expected/clangcl.pe.c.codeview.exe.sarif:2 in 9b9d3a6. [](commit_id = 9b9d3a6, deletion_comment = False)

michaelcfanning · 2022-01-25T17:50:07Z

src/Test.UnitTests.BinaryParsers/PEBinary/PEBinaryTests.cs

@@ -70,6 +74,19 @@ public void PEBinary_PdbIsStripped()
            }
        }

+        [Fact]
+        public void PEBinary_LanguagueCode()


PEBinary_LanguagueCode

Language is misspelled in this name.

This name doesn't describe what is being tested here. #Resolved

Language is misspelled in this name. --- found I have several places, searched and fixed all.

updated the name.

michaelcfanning · 2022-01-25T17:51:43Z

src/BinaryParsers/PEBinary/ProgramDatabase/ObjectModuleDetails.cs

@@ -224,28 +224,35 @@ public string GetDialect(out string versionNumber)
        }
    }

+    /// <summary>
+    /// Information about the CV_CFL_LANG enumeration, 


Information

This class is an implementation of the enumeration not information about the enumeration (though you do have some code comments that won't be visible to anyone viewing your summary comment via intellisense. #Resolved

michaelcfanning · 2022-01-25T17:53:52Z

  "results": [

How did you review these data files?

In general, these files contain too much junk to be easily verified by simply viewing them as json. Look at all the not applicable warnings.

In reply to: 1021455602

Refers to: src/Test.FunctionalTests.BinSkim.Driver/BaselineTestsData/Expected/Native_x64_RustC_Rust_debuginfo2_v1.57.exe.sarif:6 in 9b9d3a6. [](commit_id = 9b9d3a6, deletion_comment = False)

michaelcfanning

🕐

michaelcfanning · 2022-01-25T17:54:08Z

src/BinaryParsers/PEBinary/ProgramDatabase/ObjectModuleDetails.cs

-        LINK = 0x07,
-        CVTRES = 0x08,
-        CVTPGD = 0x09,
+        LINK = 0x07, // linker-generated module


linker

Is this comment adding anything? #Resolved

agree some of the comments does not add value at all, removed all to be consistent

eddynaka · 2022-02-03T22:27:07Z

src/ReleaseHistory.md

@@ -2,6 +2,7 @@

 ## Unreleased

+* BUGFIX: Add new PE `CV_CFL_LANG` language code for `ALIASOBJ` and `Rust`. [530](https://github.com/microsoft/binskim/pull/530)


BUGFIX

If this is a bugfix, what are we fixing and why? what was the behavior? #Resolved

originally I set as FEATURE,
but we think FEATURE is not buying us much in this document.
I don't think BREAKING is good here too,

@michaelcfanning and @VCOLAWTON is there a better label we can use for this type of code change for documentation purposes? FEATURE and BUGFIX both don't fit this scenario.

eddynaka

🕐

eddynaka · 2022-02-03T22:29:45Z

We should stop pushing SARIF files to the code.
Every time we have a new rule, re-ordering, we need to re-baseline over and over.
This is hard to maintain.

Can you figure it out a way to prevent this?

How can we test without re-baseline over and over?

In reply to: 1029461711

shaopeng-gh · 2022-02-03T23:02:54Z

This is hard to maintain. --- agree.

but I think this do have value, remember we found out somehow our order of the json node changes,
if not for this baseline tests we will not notice that.

In reply to: 1029461711

michaelcfanning · 2022-02-03T23:08:44Z

What we need to do is to stop depending entirely on file diffs to verify analysis. It is a useful technique but you can't use it for everything, it's a fragile approach and the diffs become overwhelming.

Notice that you haven't added a code-only test for this change: that's the primary issue: we have no code-based unit test mechanism for you to isolate/verify code changes. We need to address that.

Separately, my sense is that we should stop producing highly verbose log files for every test binary. i.e., our inventory of test binaries should generally be focused on specific rules (and their error-level output).

Now, what we lose here is exhaustive testing of verbose output (like 'not applicable' testing). We could retain verbose testing for a smaller number of binaries. Or we could author more unit-tests that report conditions in the IDE, rather than via code diffs.

In reply to: 1029481349

shaopeng-gh · 2022-02-04T00:34:05Z

got it, will change to add code-based unit tests.

In reply to: 1029484759

shaopeng-gh · 2022-02-14T21:14:21Z

I see Rust have a new version 1.58.1 will give it a try and update PR.

In reply to: 1029530224

shaopeng-gh · 2022-02-14T22:08:03Z

as of 1.58.1 it is still not using the new lang code. I have updated the test to use the new version.

In reply to: 1039566742

michaelcfanning · 2022-03-02T22:45:18Z

src/ReleaseHistory.md

@@ -1,5 +1,9 @@
 # BinSkim Release History

+## Unreleased
+
+* BUGFIX: Add new PE `CV_CFL_LANG` language code for `ALIASOBJ` and `Rust`. [530](https://github.com/microsoft/binskim/pull/530)


BUGFIX

Call this 'FEATURE:

Outdated review.

shaopeng-gh added 6 commits September 30, 2021 19:53

Add Language Code for PE

cffed44

Merge branch 'main' into users/shaopeng-gh/addnewlangcode

e497d5f

Add Objective C baseline

484bf18

Merge branch 'users/shaopeng-gh/addnewlangcode' of https://github.com…

84728fa

…/microsoft/binskim into users/shaopeng-gh/addnewlangcode

Add new PE CV_CFL_LANG language code for ALIASOBJ and RUST

1e03879

Update Rust string

21329eb

shaopeng-gh requested review from eddynaka and michaelcfanning as code owners December 15, 2021 04:13

eddynaka reviewed Dec 15, 2021

View reviewed changes

eddynaka and others added 3 commits December 15, 2021 01:42

Merge branch 'main' into users/shaopeng-gh/addnewlangcode

ba2136f

Merge branch 'main' into users/shaopeng-gh/addnewlangcode

914b14d

re-baseline with latest Sarif SDK

9b9d3a6

eddynaka approved these changes Jan 11, 2022

View reviewed changes

shaopeng-gh requested a review from marmegh January 25, 2022 03:53

marmegh reviewed Jan 25, 2022

View reviewed changes

michaelcfanning reviewed Jan 25, 2022

View reviewed changes

michaelcfanning requested changes Jan 25, 2022

View reviewed changes

michaelcfanning reviewed Jan 25, 2022

View reviewed changes

Merge branch 'main' into users/shaopeng-gh/addnewlangcode

a8c7f72

update FunctionalTestBuildScripts.md

03fc7fb

shaopeng-gh requested a review from marmegh February 3, 2022 07:18

eddynaka reviewed Feb 3, 2022

View reviewed changes

eddynaka previously requested changes Feb 3, 2022

View reviewed changes

Merge branch 'main' into users/shaopeng-gh/addnewlangcode

50cfd14

shaopeng-gh marked this pull request as draft February 14, 2022 19:58

shaopeng-gh added 2 commits February 14, 2022 13:51

update to Rust 1.5.8.1

43bcf65

Merge branch 'main' into users/shaopeng-gh/addnewlangcode

9a60f7f

shaopeng-gh added 2 commits February 14, 2022 14:16

Fix merge issue for release history file

268eae2

remove extra baseline expected sarif file

d2d41f0

shaopeng-gh marked this pull request as ready for review February 14, 2022 22:30

Merge branch 'main' into users/shaopeng-gh/addnewlangcode

af8906a

shaopeng-gh mentioned this pull request Feb 16, 2022

BUGFIX: Fix BA2014.DoNotDisableStackProtectionForFunctions to eliminate false positive reports that GsDriverEntry has disabled the stack protector. #551

Merged

merge from master

719fbc1

michaelcfanning reviewed Mar 2, 2022

View reviewed changes

michaelcfanning added 2 commits March 2, 2022 14:49

Merge branch 'main' into users/shaopeng-gh/addnewlangcode

4c816b9

Update ReleaseHistory.md

a8b922d

michaelcfanning approved these changes Mar 2, 2022

View reviewed changes

michaelcfanning enabled auto-merge (squash) March 2, 2022 22:56

michaelcfanning disabled auto-merge March 2, 2022 23:43

michaelcfanning merged commit 95e5b35 into main Mar 2, 2022

michaelcfanning deleted the users/shaopeng-gh/addnewlangcode branch March 2, 2022 23:43

marmegh mentioned this pull request Mar 14, 2022

Releasing v1.9.4 #614

Merged

		@@ -2,6 +2,7 @@

		## Unreleased

		* BUGFIX: Add new PE `CV_CFL_LANG` language code for `ALIASOBJ` and `Rust`. [530](https://github.com/microsoft/binskim/pull/530)

Add new PE CV_CFL_LANG language code for ALIASOBJ and Rust #530

Add new PE CV_CFL_LANG language code for ALIASOBJ and Rust #530

Conversation

shaopeng-gh commented Dec 15, 2021 • edited Loading

Description:

eddynaka Dec 15, 2021 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka left a comment

Choose a reason for hiding this comment

marmegh Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmegh Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

michaelcfanning Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning commented Jan 25, 2022 • edited by shaopeng-gh Loading

michaelcfanning Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

michaelcfanning commented Jan 25, 2022 • edited by shaopeng-gh Loading

michaelcfanning left a comment

Choose a reason for hiding this comment

michaelcfanning Jan 25, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Feb 3, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

shaopeng-gh Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka left a comment

Choose a reason for hiding this comment

eddynaka commented Feb 3, 2022 • edited by shaopeng-gh Loading

shaopeng-gh commented Feb 3, 2022

michaelcfanning commented Feb 3, 2022

shaopeng-gh commented Feb 4, 2022

shaopeng-gh commented Feb 14, 2022

shaopeng-gh commented Feb 14, 2022

Choose a reason for hiding this comment

shaopeng-gh commented Dec 15, 2021 •

edited

Loading

eddynaka Dec 15, 2021 •

edited by shaopeng-gh

Loading

marmegh Jan 25, 2022 •

edited by shaopeng-gh

Loading

marmegh Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jan 25, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 25, 2022 •

edited by shaopeng-gh

Loading

eddynaka Feb 3, 2022 •

edited by shaopeng-gh

Loading

shaopeng-gh Feb 3, 2022 •

edited

Loading

eddynaka commented Feb 3, 2022 •

edited by shaopeng-gh

Loading