Use dbghelp APIs to find PDB for PE when disassembling #779
Conversation
src/agent/coverage/src/pe.rs
Outdated
| let handle = win_util::process::current_process_handle(); | ||
|
|
||
| let dbghelp = debugger::dbghelp::lock()?; | ||
| dbghelp.sym_initialize(handle)?; |
There was a problem hiding this comment.
Is this necessary? I can see this if we want to build the cache when not debugging, but I think this is only called in a debugging context where symbols are already loaded using the target process handle.
Note that the value passed can be anything, it need not be a process handle, and in fact, it would probably be better to use something else because the warning in the docs getting unexpected results.
There was a problem hiding this comment.
So, at one point I actually threaded an optional handle parameter through so we could reuse an existing handle-addressed symbol resolver context, and only fell back on this. But, it ended up touching a lot of places, so I shelved it, thinking that it'd be better to revisit more holistically.
After we address everything else, I'll push a commit for you to take a peek at.
There was a problem hiding this comment.
Take a look at: 98670cf
As discussed, ideally we'll eventually migrate the handle parameter to something like a DbghelpContext.
|
|
||
| log::debug!("initial search path = {:?}", search_path); | ||
|
|
||
| // Try to add the directory of the PE to the PDB search path. |
There was a problem hiding this comment.
Is this really necessary? I'm thinking onefuzz should be setting the symbol path for other reasons, e.g. the input tester needs the correct symbol path to produce good call stacks, and also if the user remotes in to debug a crash, the symbol path should be set correctly for them already.
For the "local" scenario where we run coverage on the same machine the target is built on, the full path in the PE should be all we need.
There was a problem hiding this comment.
I'm thinking onefuzz should be setting the symbol path for other reasons
Agreed. The goal here was to make this code path more robust in various contexts. This is inessential and we could remove it.
[...] the full path in the PE should be all we need.
Maybe. I found that this was painfully brittle in practice. In local fuzzing, I would actually expect that the user typically would NOT build in the same directory the target would be invoked from. But again, OneFuzz should really be setting _NT_SYMBOL_PATH to include the setup dir, which will handle almost all cases. However, the instant a user nests a target module in the setup dir, it will then break coverage.
The original inspiration was actually item (3) here, which appears to actually be false, and in need of correction. The actual behavior is described by the docs for the UserSearchPath parameter of SymInitializeW. That said, IMO, the incorrectly-described behavior is more intuitive.
ghost
locked as resolved and limited conversation to collaborators
Closes #777.