Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add conditional check for TryGetPortablePdbMetadataReader when PDB file is missing #1001

Conversation

lukaskohl-msft
Copy link
Collaborator

@lukaskohl-msft lukaskohl-msft commented Jul 26, 2024

File.OpenRead can throw if pdb.PdbLocation is a directory.

This can happen if (for some reason) the pdb file is not found -- In which case PdbLocation will return the working directory, causing File.OpenRead to throw.

There's perhaps a better place to put this check, or a better way to handle this. Suggestions welcome :)

It would be great to have this scenario logged in some way, as there are E_PDB_NOT_FOUND or E_PDB_DBG_NOT_FOUND HRESULT codes.
For example:
C:\somePath\sni.dll : error ERR997.ExceptionLoadingPdb : 'sni.dll' was not evaluated because its PDB could not be loaded (E_PDB_NOT_FOUND).
However, I was unable to find a good way to integrate that into existing functionality, let me know if that's easily implementable.

File.OpenRead can throw if pdb.PdbLocation is a directory.

This can happen if (for some reason) the pdb file is not found.
In which case PdbLocation will return the working directory, causing
File.OpenRead to throw.
@lukaskohl-msft lukaskohl-msft force-pushed the fix/pdbLocation-invalid branch from 66af743 to 81e1bc9 Compare August 5, 2024 12:18
@lukaskohl-msft lukaskohl-msft changed the title Add conditional check for FromPortablePdbStream inside PortableExecutable/PE.cs Add conditional check for TryGetPortablePdbMetadataReader when PDB file is missing Aug 5, 2024
@@ -16,6 +16,7 @@
- NEW => new feature

## UNRELEASED
* BUG: Fix `TryGetPortablePdbMetadataReader` unexpectedly causes `UnauthorizedAccessException` error when the PDB file is missing. [1001](https://github.com/microsoft/binskim/pull/1001).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TryGetPortablePdbMetadataReader

due to evaluating to Pdb.PdbLocation being populated with the load directory (not an existing location).

@@ -16,6 +16,7 @@
- NEW => new feature

## UNRELEASED
* BUG: Fix `TryGetPortablePdbMetadataReader` unexpectedly causes `UnauthorizedAccessException` error when the PDB file is missing. [1001](https://github.com/microsoft/binskim/pull/1001).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TryGetPortablePdbMetadataReader

I'd suggest creating a repro binary to check-in.

Copy link
Member

@michaelcfanning michaelcfanning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Adds a sample library that has been compiled as a standalone portable
executable.

The library doesn't have an accompanying pdb file, where prior to commit
`81e1bc93` would make binskim throw on the working thread.
@lukaskohl-msft lukaskohl-msft changed the base branch from main to users/lukaskohl/pdblocation-invalid August 7, 2024 09:27
@lukaskohl-msft lukaskohl-msft merged commit b839fdd into microsoft:users/lukaskohl/pdblocation-invalid Aug 7, 2024
3 of 4 checks passed
@lukaskohl-msft lukaskohl-msft deleted the fix/pdbLocation-invalid branch August 7, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants