From c3d9a8c25e57e8537b7846f0a17cd92438147173 Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Wed, 19 Jul 2023 00:51:54 -0700 Subject: [PATCH 1/3] resolve other comments --- .../MultithreadedAnalyzeCommand.cs | 4 +-- src/BinSkim.Sdk/BinaryAnalyzerContext.cs | 28 ++++++++----------- src/BinaryParsers/BinaryParsersProperties.cs | 20 ++++++++++++- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs b/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs index 2018a3fd..1661fa94 100644 --- a/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs +++ b/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs @@ -68,9 +68,9 @@ public override BinaryAnalyzerContext InitializeGlobalContextFromOptions(Analyze base.InitializeGlobalContextFromOptions(options, ref context); // Update context object based on command-line parameters. - context.SymbolPath = options.SymbolsPath; + context.SymbolPath = options.SymbolsPath ?? context.SymbolPath; context.IgnorePdbLoadError = options.IgnorePdbLoadError != null ? options.IgnorePdbLoadError.Value : context.IgnorePdbLoadError; - context.LocalSymbolDirectories = options.LocalSymbolDirectories; + context.LocalSymbolDirectories = options.LocalSymbolDirectories ?? context.LocalSymbolDirectories; context.TracePdbLoads = options.Trace.Contains(nameof(Traces.PdbLoad)); context.CompilerDataLogger = diff --git a/src/BinSkim.Sdk/BinaryAnalyzerContext.cs b/src/BinSkim.Sdk/BinaryAnalyzerContext.cs index 613290e9..027af485 100644 --- a/src/BinSkim.Sdk/BinaryAnalyzerContext.cs +++ b/src/BinSkim.Sdk/BinaryAnalyzerContext.cs @@ -38,7 +38,11 @@ public override bool IsValidAnalysisTarget get => this.Binary?.Valid == true; } - public string LocalSymbolDirectories { get; set; } + public string LocalSymbolDirectories + { + get => this.Policy?.GetProperty(BinaryParsersProperties.LocalSymbolDirectories); + set => this.Policy.SetProperty(BinaryParsersProperties.LocalSymbolDirectories, value); + } public bool ComprehensiveBinaryParsing { @@ -48,7 +52,11 @@ public bool ComprehensiveBinaryParsing public bool TracePdbLoads { get; set; } - public string SymbolPath { get; set; } + public string SymbolPath + { + get => this.Policy?.GetProperty(BinaryParsersProperties.SymbolPath); + set => this.Policy.SetProperty(BinaryParsersProperties.SymbolPath, value); + } public override IAnalysisLogger Logger { get; set; } @@ -66,16 +74,7 @@ public override string MimeType public override bool AnalysisComplete { get; set; } - public CompilerDataLogger CompilerDataLogger - { - get - { - return this.Policy != null - ? this.Policy.GetProperty(SharedCompilerDataLoggerProperty) - : null; - } - set { this.Policy.SetProperty(SharedCompilerDataLoggerProperty, value); } - } + public CompilerDataLogger CompilerDataLogger { get; set; } public bool IgnorePdbLoadError { @@ -110,11 +109,6 @@ protected virtual void Dispose(bool disposing) } } - public static PerLanguageOption SharedCompilerDataLoggerProperty { get; } = - new PerLanguageOption( - "CompilerTelemetry", nameof(SharedCompilerDataLoggerProperty), defaultValue: () => null, - "A shared CompilerDataLogger instance that will be passed to all skimmers."); - public override void Dispose() { // Do not change this code. Put cleanup code in Dispose(bool disposing) above. diff --git a/src/BinaryParsers/BinaryParsersProperties.cs b/src/BinaryParsers/BinaryParsersProperties.cs index 47b90b60..a9adf61a 100644 --- a/src/BinaryParsers/BinaryParsersProperties.cs +++ b/src/BinaryParsers/BinaryParsersProperties.cs @@ -17,7 +17,9 @@ public IEnumerable GetOptions() { ComprehensiveBinaryParsing, IgnorePdbLoadError, - IncludeWixBinaries + IncludeWixBinaries, + LocalSymbolDirectories, + SymbolPath }.ToImmutableArray(); } @@ -36,5 +38,21 @@ public IEnumerable GetOptions() new PerLanguageOption( "BinaryParsers", nameof(IncludeWixBinaries), defaultValue: () => false, "Set this value to 'true' to include Wix binaries in the analysis."); + + public static PerLanguageOption LocalSymbolDirectories { get; } = + new PerLanguageOption( + "BinaryParsers", nameof(LocalSymbolDirectories), defaultValue: () => null, + "A set of semicolon-delimited local directory paths that will be examined when attempting to locate PDBs."); + + public static PerLanguageOption SymbolPath { get; } = + new PerLanguageOption( + "BinaryParsers", nameof(SymbolPath), defaultValue: () => null, + "Symbols path value, e.g., Cache*c:\\symbols;SRV*https://msdl.microsoft.com/download/symbols " + + "or Cache*d:\\symbols;Srv*https://symweb. See " + + "https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/advanced-symsrv-use for " + + "syntax information. Note that BinSkim will clear the _NT_SYMBOL_PATH and _NT_ALT_SYMBOL_PATH " + + "environment variables at runtime. Use this argument instead for specifying the symbol path." + + "WARNING: Be sure to specify a local file cache in the symbol path if at all possible, in order " + + "to avoid the possibility of significance time-to-analyze performance degradataion."); } } From a2f8595380096978379bbcf0dd2c55734ff4894d Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Wed, 19 Jul 2023 01:11:21 -0700 Subject: [PATCH 2/3] default string.Empty --- src/BinaryParsers/BinaryParsersProperties.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/BinaryParsers/BinaryParsersProperties.cs b/src/BinaryParsers/BinaryParsersProperties.cs index a9adf61a..6afd2b43 100644 --- a/src/BinaryParsers/BinaryParsersProperties.cs +++ b/src/BinaryParsers/BinaryParsersProperties.cs @@ -41,12 +41,12 @@ public IEnumerable GetOptions() public static PerLanguageOption LocalSymbolDirectories { get; } = new PerLanguageOption( - "BinaryParsers", nameof(LocalSymbolDirectories), defaultValue: () => null, + "BinaryParsers", nameof(LocalSymbolDirectories), defaultValue: () => string.Empty, "A set of semicolon-delimited local directory paths that will be examined when attempting to locate PDBs."); public static PerLanguageOption SymbolPath { get; } = new PerLanguageOption( - "BinaryParsers", nameof(SymbolPath), defaultValue: () => null, + "BinaryParsers", nameof(SymbolPath), defaultValue: () => string.Empty, "Symbols path value, e.g., Cache*c:\\symbols;SRV*https://msdl.microsoft.com/download/symbols " + "or Cache*d:\\symbols;Srv*https://symweb. See " + "https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/advanced-symsrv-use for " + From bf31dc30759d70ee4d9ec6092fb08ec06c94c620 Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Wed, 19 Jul 2023 01:22:02 -0700 Subject: [PATCH 3/3] Update Release History --- ReleaseHistory.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ReleaseHistory.md b/ReleaseHistory.md index 5c6738ec..b00978a9 100644 --- a/ReleaseHistory.md +++ b/ReleaseHistory.md @@ -26,7 +26,8 @@ * BUG: Fix `System.InvalidOperationException`: `Sequence contains more than one matching element` when `--trace` is provided. [896](https://github.com/microsoft/binskim/pull/896) * BUG: Fix `--trace` missing supported values from SARIF SDK (`ScanTime`, `RuleScanTime`, `PeakWorkingSet`, `TargetsScanned`, `ResultsSummary`). [896](https://github.com/microsoft/binskim/pull/896) * NEW: `BA2024.EnableSpectreMitigations` now informs user when a compiland `RawCommandLine` value is missing and the rule is therefore not able to determine if `/Qspectre` is specified. [#933](https://github.com/microsoft/binskim/pull/933) -* NEW: Add `--includeWixBinaries` option to include Wix binaries in the analysis. [#944](https://github.com/microsoft/binskim/pull/944) +* NEW: Add `IncludeWixBinaries` option when using config file, to include Wix binaries in the analysis. [#944](https://github.com/microsoft/binskim/pull/944) +* NEW: Support `SymbolPath`, `LocalSymbolDirectories`, `IgnorePdbLoadError` option when using config file, in addtion to passing as command line parameters. [#944](https://github.com/microsoft/binskim/pull/944) ## **v4.1.0** * DEP: Update Sarif.Sdk submodule from [120fae3 to bc8cb57](https://github.com/microsoft/sarif-sdk/compare/120fae3...bc8cb57). Full [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/bc8cb57/ReleaseHistory.md).