Adding compiler data to AppInsights #404

eddynaka · 2021-07-07T01:23:37Z

No description provided.

Adding improvements

…-data

…ttps://github.com/microsoft/binskim into users/ednakamu/application-insights-compiler-data

lgtm-com · 2021-07-15T00:45:29Z

This pull request fixes 2 alerts when merging fd254b4 into cbae8ad - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

…-data

lgtm-com · 2021-07-15T12:34:37Z

This pull request fixes 2 alerts when merging b0f5106 into ad9e840 - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

…-data

lgtm-com · 2021-07-15T15:01:15Z

This pull request fixes 2 alerts when merging aedb1eb into 350fcf6 - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

src/BinaryParsers/MachOBinary/MachOBinary.cs

michaelcfanning · 2021-07-16T17:59:05Z

    private const string SECTIONNAME_DEBUG_INFO = "__debug_info";

these strings are neither alphabetized nor are they sorted by line length.

can we sort by line length? makes things more readable.

In reply to: 881621666

Refers to: src/BinaryParsers/MachOBinary/SingleMachOBinary.cs:16 in aedb1eb. [](commit_id = aedb1eb, deletion_comment = False)

src/BinSkim.Driver/AnalyzeCommand.cs

michaelcfanning · 2021-07-16T18:07:19Z

src/BinSkim.Driver/AnalyzeOptions.cs

+
+        [Option(
+            "repository-uri",
+            HelpText = "RepositoryUri is required to rule BA4001 and BA4002.")]


RepositoryUri is required to rule BA4001 and BA4002

Whenever you show an opaque rule id to the user, you should add the readable name. Not everyone can remember what BA4001 is. Also, you've described a fact here 'this data is required to run rule BA4001.ReportPECompilerData' but you haven't described what that data is.

What is this data? Does it make sense that we require a single repo URL for a scan run??

I changed the helptext. Let me know what do you think.

About your question...I thought about it, and I could not find anything better based on this:
https://docs.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml

my idea was to find one pre-defined variable that any pipeline would be able to get.

michaelcfanning · 2021-07-16T18:07:36Z

src/BinSkim.Driver/AnalyzeOptions.cs

+
+        [Option(
+            "pipeline-name",
+            HelpText = "PipelineName is required to rule BA4001 and BA4002.")]


PipelineName

same note, what is this data? you don't describe it. #Resolved

updated the helptext, pls, review :)

michaelcfanning · 2021-07-16T18:08:44Z

src/BinSkim.Rules/DwarfRules/BA4002.ReportDwarfCompilerData.cs

-                    Console.Write($"{file},");
-                    Console.Write($"{context?.Hashes?.Sha256},");
-                    Console.WriteLine();
+                    context.CompilerDataLogger.Write(compiler.Compiler.ToString(), compiler.Version.ToString(), language, file);


CompilerDataLogger

why is it ok that some data has gotten dropped? such as the sha256 has? #ByDesign

where I create the CompilerDataLogger, i pass the context. from it I retrieve information such as hash. With that, i can remove from here and just pass the data that i dont have during construction of compilerDataLogger

michaelcfanning · 2021-07-16T18:09:24Z

src/BinSkim.Rules/PERules/BA4001.ReportPECompilerData.cs

-                Console.Write(context.TargetUri.LocalPath + ",");
-                Console.WriteLine($",,,,,,{context?.Hashes?.Sha256},{target.PdbParseException.Message}");
+                string errorMessage = target.PdbParseException.Message;
+                context.CompilerDataLogger.WriteException(errorMessage);


CompilerDataLogger

what about the hash data? #ByDesign

the hash is constructed with the CompilerDataLogger, so we don't need to worry here.

michaelcfanning · 2021-07-16T18:12:26Z

src/BinSkim.Sdk/CompilerDataLogger.cs

+
+                s_telemetryClient.TrackEvent(CompilerEventName, properties: new Dictionary<string, string>
+                {
+                    { "repositoryUri", repositoryUri ?? string.Empty },


repositoryUri

you say in your help comments that this data is required, but it doesn't look like it's required. it's optional.

i think you meant you say that this data is only consumed by the compiler reporting rules, and not relevant otherwise. #Closed

I changed the relevant code to throw an argumentNullException in case repositoryUri or pipelineName is null/empty.

With that, i cleaned the code

michaelcfanning · 2021-07-16T18:13:07Z

src/BinSkim.Sdk/CompilerDataLogger.cs

+                {
+                    { "repositoryUri", repositoryUri ?? string.Empty },
+                    { "pipelineName", pipelineName ?? string.Empty },
+                    { "target", this.localPath },


localPath

why are we sending absolute paths? is this right? might this data expose some unwanted information, like a user alias? #Resolved

made a change to transform the localPath to a relativeFilePath base on the targetFileSpecifiers

michaelcfanning · 2021-07-16T18:14:35Z

src/BinSkim.Sdk/CompilerDataLogger.cs

+                    { "pipelineName", pipelineName ?? string.Empty },
+                    { "target", this.localPath },
+                    { "compilerName", compilerDataParts[0] },
+                    { "compilerBackEndVersion", compilerDataParts[1] },


compilerDataParts[1]

look at these 'magic constants' where we have special knowledge that backend version is in the first piece.

that tends to be pretty fragile, it might be better for this helper to break out all this data and require the caller to parse into the format.
#Closed

michaelcfanning · 2021-07-16T18:15:12Z

src/BinSkim.Sdk/CompilerDataLogger.cs

+            }
+            else
+            {
+                Console.Write($"{repositoryUri},");


Write

Note that it'd be more performant to concatenate all this information into a single string and then pass it to WriteLine.
#Closed

michaelcfanning

🕐

eddynaka · 2021-07-16T23:25:07Z

    private const string SECTIONNAME_DEBUG_INFO = "__debug_info";

fixed.

In reply to: 881621666

Refers to: src/BinaryParsers/MachOBinary/SingleMachOBinary.cs:16 in aedb1eb. [](commit_id = aedb1eb, deletion_comment = False)

…ttps://github.com/microsoft/binskim into users/ednakamu/application-insights-compiler-data

lgtm-com · 2021-07-16T23:41:37Z

This pull request fixes 2 alerts when merging 9d21968 into be2289a - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

lgtm-com · 2021-07-17T00:33:07Z

This pull request fixes 2 alerts when merging 7ce6027 into be2289a - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

lgtm-com · 2021-07-17T16:41:21Z

This pull request fixes 2 alerts when merging 5112e4a into be2289a - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

lgtm-com · 2021-07-26T22:26:02Z

This pull request fixes 2 alerts when merging 4fb5fbc into be2289a - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

…-data

lgtm-com · 2021-07-26T22:54:40Z

This pull request fixes 2 alerts when merging a6a3632 into be2289a - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

…ttps://github.com/microsoft/binskim into users/ednakamu/application-insights-compiler-data

lgtm-com · 2021-07-27T01:15:59Z

This pull request fixes 2 alerts when merging 843eeed into be2289a - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

lgtm-com · 2021-07-27T15:09:51Z

This pull request fixes 2 alerts when merging 2ddf392 into be2289a - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

lgtm-com · 2021-07-27T20:03:16Z

This pull request fixes 2 alerts when merging 0c7b53b into be2289a - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

lgtm-com · 2021-07-27T20:40:15Z

This pull request fixes 2 alerts when merging ed8cb64 into be2289a - view on LGTM.com

fixed alerts:

2 for Dereferenced variable may be null

michaelcfanning

Adding compiler data to AppInsights

d7d01f9

Adding improvements

eddynaka force-pushed the users/ednakamu/application-insights-compiler-data branch from 3af6b99 to d7d01f9 Compare July 12, 2021 23:35

eddynaka and others added 7 commits July 12, 2021 21:11

Adding basic constructor tests, adding parameters

9a1a8cf

Adding flush method, adding placeholder for dialect

ee25a1c

Merge branch 'main' into users/ednakamu/application-insights-compiler…

aa8d558

…-data

Merge branch 'main' into users/ednakamu/application-insights-compiler…

477a0e9

…-data

Using envvariable to retrieve AppInsightsKey

db3aece

Merge branch 'users/ednakamu/application-insights-compiler-data' of h…

5e6450b

…ttps://github.com/microsoft/binskim into users/ednakamu/application-insights-compiler-data

Simplifying tests

fd254b4

Merge branch 'main' into users/ednakamu/application-insights-compiler…

b0f5106

…-data

eddynaka marked this pull request as ready for review July 15, 2021 12:16

eddynaka requested a review from michaelcfanning as a code owner July 15, 2021 12:16

Merge branch 'main' into users/ednakamu/application-insights-compiler…

aedb1eb

…-data

microsoft deleted a comment from lgtm-com bot Jul 15, 2021

michaelcfanning reviewed Jul 16, 2021

View reviewed changes

src/BinaryParsers/MachOBinary/MachOBinary.cs Outdated Show resolved Hide resolved

michaelcfanning reviewed Jul 16, 2021

View reviewed changes

src/BinSkim.Driver/AnalyzeCommand.cs Outdated Show resolved Hide resolved

michaelcfanning reviewed Jul 16, 2021

View reviewed changes

src/BinSkim.Driver/AnalyzeCommand.cs Outdated Show resolved Hide resolved

michaelcfanning reviewed Jul 16, 2021

View reviewed changes

michaelcfanning requested changes Jul 16, 2021

View reviewed changes

eddynaka added 2 commits July 16, 2021 20:25

Addressing PR feedback - 1

729453c

Merge branch 'users/ednakamu/application-insights-compiler-data' of h…

9d21968

…ttps://github.com/microsoft/binskim into users/ednakamu/application-insights-compiler-data

Addressing PR feedback - 2

7ce6027

Adressing PR feedback - 3 (making target relative)

5112e4a

Adding CompilerData struct to prevent magic strings

4fb5fbc

Merge branch 'main' into users/ednakamu/application-insights-compiler…

a6a3632

…-data

eddynaka added 2 commits July 26, 2021 21:58

Removing data manipulation. let's use the raw data.

0527ae9

Merge branch 'users/ednakamu/application-insights-compiler-data' of h…

843eeed

…ttps://github.com/microsoft/binskim into users/ednakamu/application-insights-compiler-data

Retrieving FileVersion

2ddf392

eddynaka added 2 commits July 27, 2021 16:31

Updating rules, adding more relevant information

ff40b7b

Deleting test

0c7b53b

removing redundant code

ed8cb64

michaelcfanning approved these changes Jul 27, 2021

View reviewed changes

eddynaka merged commit aa1f867 into main Jul 27, 2021

eddynaka deleted the users/ednakamu/application-insights-compiler-data branch July 27, 2021 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding compiler data to AppInsights #404

Adding compiler data to AppInsights #404

eddynaka commented Jul 7, 2021

lgtm-com bot commented Jul 15, 2021

lgtm-com bot commented Jul 15, 2021

lgtm-com bot commented Jul 15, 2021

michaelcfanning commented Jul 16, 2021 •

edited by eddynaka

Loading

michaelcfanning Jul 16, 2021

eddynaka Jul 17, 2021

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

eddynaka Jul 17, 2021

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

eddynaka Jul 16, 2021

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

eddynaka Jul 16, 2021

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

eddynaka Jul 17, 2021

michaelcfanning Jul 16, 2021 •

edited

Loading

eddynaka Jul 17, 2021

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

eddynaka Jul 26, 2021

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

eddynaka Jul 17, 2021

michaelcfanning left a comment

eddynaka commented Jul 16, 2021

lgtm-com bot commented Jul 16, 2021

lgtm-com bot commented Jul 17, 2021

lgtm-com bot commented Jul 17, 2021

lgtm-com bot commented Jul 26, 2021

lgtm-com bot commented Jul 26, 2021

lgtm-com bot commented Jul 27, 2021

lgtm-com bot commented Jul 27, 2021

lgtm-com bot commented Jul 27, 2021

lgtm-com bot commented Jul 27, 2021

michaelcfanning left a comment

Adding compiler data to AppInsights #404

Adding compiler data to AppInsights #404

Conversation

eddynaka commented Jul 7, 2021

lgtm-com bot commented Jul 15, 2021

lgtm-com bot commented Jul 15, 2021

lgtm-com bot commented Jul 15, 2021

michaelcfanning commented Jul 16, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 16, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 16, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 16, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 16, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 16, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 16, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

eddynaka commented Jul 16, 2021

lgtm-com bot commented Jul 16, 2021

lgtm-com bot commented Jul 17, 2021

lgtm-com bot commented Jul 17, 2021

lgtm-com bot commented Jul 26, 2021

lgtm-com bot commented Jul 26, 2021

lgtm-com bot commented Jul 27, 2021

lgtm-com bot commented Jul 27, 2021

lgtm-com bot commented Jul 27, 2021

lgtm-com bot commented Jul 27, 2021

michaelcfanning left a comment

Choose a reason for hiding this comment

michaelcfanning commented Jul 16, 2021 •

edited by eddynaka

Loading

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

michaelcfanning Jul 16, 2021 •

edited

Loading

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading

michaelcfanning Jul 16, 2021 •

edited by eddynaka

Loading