* BUGFIX: Fix `error ERR997.ExceptionLoadingPdb : '[filename]' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).` when reading PE file built with `PDBPageSize:8192` or greater, by upgrading msdia140.dll from `14.27.28826.96` to `14.32.31326.0`. #685

shaopeng-gh · 2022-08-02T21:03:37Z

Description:

User reported issue by email:

"RE: BinSkim PDB Error related to PdbPageSize:8192"

Briefly:
The BinSkim has been running well on our build pipeline, but has hit an error (below) when I am testing my change to enable “PDBPageSize:8192” support in all of our PDB generation. The .PDB files are present in the same folder as before, but now have a changed internal format. I assert that E_PDB_NOT_FOUND error may be reported on cases where the file is present, but reading its content fails unexpectedly.

D:\a\_work\1\a\BrowserCommonHarness.exe : error ERR997.ExceptionLoadingPdb : 'BrowserCommonHarness.exe' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).

The files do exist, with file names:
BrowserCommonHarness.exe
BrowserCommonHarness.exe.pdb

Cause:

The creating Big PdbPageSize support only starts with Clang 14, and reading it need a newer version of msdia library as well.

Fix:

update to the latest msdia140.dll comes with the VS 2022.

…evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).` when reading PE file built with `PDBPageSize:8192` or grater, by upgrading msdia140.dll from `14.27.28826.96` to `14.32.31326.0`.

shaopeng-gh · 2022-08-02T21:23:29Z

...ctionalTests.BinSkim.Driver/BaselineTestData/Expected/MixedMode_x64_VS2015_Default.exe.sarif

@@ -988,20 +988,20 @@
              "id": "BA6001",
              "name": "DisableIncrementalLinkingInReleaseBuilds",
              "fullDescription": {
-                "text": "Incremental linking support increases binary size and can reduce runtime performance. Fully optimized release builds should not specify incremental linking."
+                "text": "Incremental linking support increases binary size and can reduce runtime performance. The support for incremental linking adds padding and other overhead to support the ability to modify a binary without a full link.  The use of incrementally linked binaries may reduce the level of determinism because previous compilations will have lingering effects on subsequent compilations.  Fully optimized release builds should not specify incremental linking."


"text": "Incremental

I re-ran the baseline to see if my change affect baselines, all the changes are related to previous change BA6001. #Closed

Sorry about that one. Somehow this staleness didn't break anything so I didn't notice it when I updated the strings based on PR feedback. I was seeing these updates too when working on some additional rules and updating the baselines accordingly.

no worries at all I just call it out to save reviewer's when see this diff.

With no enforcement in the build and test no one will remember to run re-baseline.
(
My preference is to change the re-baseline test to strict compare so that we are forced to re-run baseline and review the changes caused, expectedly and unexpectedly :)
)

Shaopeng, if you haven't already, please create an Issue to track this suggestion.

shaopeng-gh · 2022-08-02T22:12:55Z

btw, I think there are some potential wrong assumptions in the space:

assumption with PE we are looking for pdb.
PE can also build with dwarf.
assumption of PE and PDB are build with MS toolchain.
LLVM can also build PE with PDB
assumption the exe and pdb naming with a.exe and a.pdb
the pdb file name is coded inside exe, it can be any naming convention.
assuming the pdb file is the one for the exe just by name matches, a.exe and a.pdb.
I delete the pdb for a exe and try re-naming a random pdb back, code loads without question. This can be verified by matching GUID with exe and pdb.
PE rules and also how to fix considers only MS toolchain, some of them clang does not support. If we are supporting clang toolchain we may make some modification as appropriate.

shaopeng-gh · 2022-08-02T22:28:15Z

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

-        [Fact]
-        public void PEBinary_PdbAvailable()
+        [Theory]
+        [InlineData("clangcl.14.pe.c.codeview.pdbpagesize_default.exe")]


14

The only version of test is 14 because it only starts support at v14, and the LLVM v15 is not yet in released status.

shaopeng-gh · 2022-08-02T22:32:38Z

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

+        [InlineData("clangcl.14.pe.c.codeview.pdbpagesize_default.exe")]
+        [InlineData("clangcl.14.pe.c.codeview.pdbpagesize_4096.exe")]
+        [InlineData("clangcl.14.pe.c.codeview.pdbpagesize_8192.exe")]
+        [InlineData("clangcl.14.pe.c.codeview.pdbpagesize_16384.exe")]


16384

these are not random test, it should be all the supported number:
/PdbPageSize:[4096|8192|16384|32768]
the difference between default and 4096 is whether it is explicitly passed.

marmegh · 2022-08-15T15:57:39Z

src/ReleaseHistory.md

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

 ## Unreleased

+* BUGFIX: Fix `error ERR997.ExceptionLoadingPdb : '[filename]' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).` when reading PE file built with `PDBPageSize:8192` or grater, by upgrading msdia140.dll from `14.27.28826.96` to `14.32.31326.0`. [685](https://github.com/microsoft/binskim/pull/685)


BUGFIX: Fix error ERR997.ExceptionLoadingPdb : '[filename]' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND). when reading PE file built with PDBPageSize:8192 or grater

Nice description! #Closed

marmegh · 2022-08-15T16:02:39Z

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

+        [InlineData("clangcl.14.pe.c.codeview.pdbpagesize_16384.exe")]
+        [InlineData("clangcl.14.pe.c.codeview.pdbpagesize_32768.exe")]
+        [InlineData("Native_x64_VS2022_PDBPageSize_8192.exe")]
+        [InlineData("Native_x64_VS2013_Default.dll", true)]


This type of test adds extra latency to test runtime. We can get the same effect without the latency by adding a foreach loop over a list containing the same strings within the test. #Closed

Oh I just discovered this syntax in the existing tests,
looks cool, did not know it is slow, let me change it to a loop.

marmegh · 2022-08-15T16:03:09Z

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

-        public void PEBinary_NoPdbAvailable()
+        [Theory]
+        [InlineData("clangcl.14.pe.c.codeview.pdbpagesize_8192_pdbmissing.exe")]
+        [InlineData("Native_x86_VS2013_PdbMissing.exe", true)]


This type of test adds extra latency to test runtime. We can get the same effect without the latency by adding a foreach loop over a list containing the same strings within the test. #Closed

src/ReleaseHistory.md

marmegh · 2022-08-15T17:05:54Z

5: should we then also update to check the EXE to identify the correct PDB? The customer who reported this will not be fixed with this change, then.

In reply to: 1203265011

shaopeng-gh · 2022-08-15T18:40:58Z

5: --- No the code is fine we are using library which internally will do the lookup of the file name and load it. Just to note expect it may be different when we do troubleshooting.

In reply to: 1215392485

marmegh · 2022-08-15T20:44:52Z

I have a concern around additional time to run with these changes. Analyzing the four files provided by the customer for repro, took almost 2 minutes. Is this an indicator that we could see an increase in BinSkim timing out for customers with this change. @shaopeng-gh, did you experience any changes in time to run with this change?

marmegh · 2022-08-15T20:50:45Z

Good news here, is that smaller PDBs appear to be parsed with a similar timing as before this change. The issue is going to come with PDBs >4K could result in binskim timeouts. Is there a way to decrease the load time for these?

In reply to: 1215773022

shaopeng-gh · 2022-08-15T20:51:10Z

The files provided by the customer was about 4GB in file size, it was slow due to size, our test files in the test is small so is fast.

In reply to: 1215773022

shaopeng-gh · 2022-08-15T21:23:35Z

I think I might mixed up the question about the fix and the testcase.

the run time diff for the fix:

First, PdbPageSize 8192 does not indicate it is a huge pdb,

case 1. normal pdb file size, but using PdbPageSize 8192:
they were errored out, now they are analyzed, this will increase the total run time.
The impact of run time is similar to user has more files.

case 2. huge pdb file size, and using PdbPageSize 8192:
they were errored out, now they are analyzed, this will increase the total run time.
This has big impact, the impact of run time I think also depend on disk speed, loading a large binary locally SSD may be faster than a small file on network share.

case 3&4. any pdb file size, and not using PdbPageSize 8192:
same as before.

Yes, I agree it will increase time and increase the chance of timeout.

In reply to: 1215773022

shaopeng-gh · 2022-08-15T21:27:14Z

for the testcase:
I have changed to use the loop, and the test cases checked in are normal file size so they are fast.

In reply to: 1215849086

marmegh · 2022-08-15T23:19:13Z

This is an awesome overview of the time consideration. Thank you for expanding on the details.

In reply to: 1215849086

marmegh

marmegh · 2022-08-15T23:27:52Z

I've reviewed the changes and run them locally against the provided repros (both original and generated alternate test cases). @michaelcfanning, your opinion on the timeout risk is likely the main blocker to merging.

In reply to: 1215973925

shaopeng-gh added 2 commits August 2, 2022 13:46

BUGFIX: Fix `error ERR997.ExceptionLoadingPdb : '[filename]' was not …

ddedd75

…evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).` when reading PE file built with `PDBPageSize:8192` or grater, by upgrading msdia140.dll from `14.27.28826.96` to `14.32.31326.0`.

Re-run baseline

d8a671f

shaopeng-gh requested a review from yongyan-gh August 2, 2022 21:03

shaopeng-gh requested review from michaelcfanning and marmegh as code owners August 2, 2022 21:03

shaopeng-gh commented Aug 2, 2022

View reviewed changes

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

86670a6

marmegh reviewed Aug 15, 2022

View reviewed changes

dmachaj reviewed Aug 15, 2022

View reviewed changes

src/ReleaseHistory.md Outdated Show resolved Hide resolved

shaopeng-gh added 2 commits August 15, 2022 11:45

Fix spelling

76413b8

Change test to not use [Theory]

62d9f84

marmegh approved these changes Aug 15, 2022

View reviewed changes

michaelcfanning merged commit e2a9db5 into main Aug 31, 2022

michaelcfanning deleted the users/shaopeng-gh/dia branch August 31, 2022 20:41

marmegh mentioned this pull request Aug 31, 2022

Preparing to release BinSkim v1.10.0 #701

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

* BUGFIX: Fix `error ERR997.ExceptionLoadingPdb : '[filename]' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).` when reading PE file built with `PDBPageSize:8192` or greater, by upgrading msdia140.dll from `14.27.28826.96` to `14.32.31326.0`. #685

* BUGFIX: Fix `error ERR997.ExceptionLoadingPdb : '[filename]' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).` when reading PE file built with `PDBPageSize:8192` or greater, by upgrading msdia140.dll from `14.27.28826.96` to `14.32.31326.0`. #685

shaopeng-gh commented Aug 2, 2022

shaopeng-gh Aug 2, 2022 •

edited by marmegh

Loading

dmachaj Aug 3, 2022

shaopeng-gh Aug 3, 2022

marmegh Aug 15, 2022

shaopeng-gh Aug 16, 2022

shaopeng-gh commented Aug 2, 2022

shaopeng-gh Aug 2, 2022

shaopeng-gh Aug 2, 2022

marmegh Aug 15, 2022 •

edited

Loading

marmegh Aug 15, 2022 •

edited

Loading

shaopeng-gh Aug 15, 2022

marmegh Aug 15, 2022 •

edited

Loading

marmegh commented Aug 15, 2022

shaopeng-gh commented Aug 15, 2022

marmegh commented Aug 15, 2022

marmegh commented Aug 15, 2022

shaopeng-gh commented Aug 15, 2022 •

edited

Loading

shaopeng-gh commented Aug 15, 2022 •

edited

Loading

shaopeng-gh commented Aug 15, 2022

marmegh commented Aug 15, 2022

marmegh left a comment

marmegh commented Aug 15, 2022

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

		## Unreleased

		* BUGFIX: Fix `error ERR997.ExceptionLoadingPdb : '[filename]' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).` when reading PE file built with `PDBPageSize:8192` or grater, by upgrading msdia140.dll from `14.27.28826.96` to `14.32.31326.0`. [685](https://github.com/microsoft/binskim/pull/685)

* BUGFIX: Fix error ERR997.ExceptionLoadingPdb : '[filename]' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND). when reading PE file built with PDBPageSize:8192 or greater, by upgrading msdia140.dll from 14.27.28826.96 to 14.32.31326.0. #685

* BUGFIX: Fix error ERR997.ExceptionLoadingPdb : '[filename]' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND). when reading PE file built with PDBPageSize:8192 or greater, by upgrading msdia140.dll from 14.27.28826.96 to 14.32.31326.0. #685

Conversation

shaopeng-gh commented Aug 2, 2022

Description:

Cause:

Fix:

shaopeng-gh Aug 2, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

dmachaj Aug 3, 2022

Choose a reason for hiding this comment

shaopeng-gh Aug 3, 2022

Choose a reason for hiding this comment

marmegh Aug 15, 2022

Choose a reason for hiding this comment

shaopeng-gh Aug 16, 2022

Choose a reason for hiding this comment

shaopeng-gh commented Aug 2, 2022

shaopeng-gh Aug 2, 2022

Choose a reason for hiding this comment

shaopeng-gh Aug 2, 2022

Choose a reason for hiding this comment

marmegh Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

marmegh Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

shaopeng-gh Aug 15, 2022

Choose a reason for hiding this comment

marmegh Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

marmegh commented Aug 15, 2022

shaopeng-gh commented Aug 15, 2022

marmegh commented Aug 15, 2022

marmegh commented Aug 15, 2022

shaopeng-gh commented Aug 15, 2022 • edited Loading

shaopeng-gh commented Aug 15, 2022 • edited Loading

shaopeng-gh commented Aug 15, 2022

marmegh commented Aug 15, 2022

marmegh left a comment

Choose a reason for hiding this comment

marmegh commented Aug 15, 2022

* BUGFIX: Fix `error ERR997.ExceptionLoadingPdb : '[filename]' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).` when reading PE file built with `PDBPageSize:8192` or greater, by upgrading msdia140.dll from `14.27.28826.96` to `14.32.31326.0`. #685

* BUGFIX: Fix `error ERR997.ExceptionLoadingPdb : '[filename]' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).` when reading PE file built with `PDBPageSize:8192` or greater, by upgrading msdia140.dll from `14.27.28826.96` to `14.32.31326.0`. #685

shaopeng-gh Aug 2, 2022 •

edited by marmegh

Loading

marmegh Aug 15, 2022 •

edited

Loading

marmegh Aug 15, 2022 •

edited

Loading

marmegh Aug 15, 2022 •

edited

Loading

shaopeng-gh commented Aug 15, 2022 •

edited

Loading

shaopeng-gh commented Aug 15, 2022 •

edited

Loading