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

RAR BadImageReferenceException needs to log the callstack and exception type #6224

Closed
KirillOsenkov opened this issue Mar 4, 2021 · 15 comments · Fixed by #6240 or #8271
Closed

RAR BadImageReferenceException needs to log the callstack and exception type #6224

KirillOsenkov opened this issue Mar 4, 2021 · 15 comments · Fixed by #6240 or #8271
Labels
Area: Debuggability Issues impacting the diagnosability of builds, including logging and clearer error messages. Area: Logging bug needs-design Requires discussion with the dev team before attempting a fix. Task: Resolve Assembly References (RAR) triaged

Comments

@KirillOsenkov
Copy link
Member

I'm investigating a build with this warning from RAR:

/Users/builder/azdo/_work/1/s/main/build/tools/MSBuild/bin/Microsoft.Common.CurrentVersion.targets(2143,5): warning MSB3246: Resolved file has a bad image, no metadata, or is otherwise inaccessible.

This exception is thrown in four places:
https://source.dot.net/#Microsoft.Build.Tasks.Core/AssemblyDependency/BadImageReferenceException.cs,cbeccdcba5e80a17,references

It could be a BadImageFormatException, FileNotFoundException, FileLoadException or any other IORelated exception. We throw away the callstack and don't display the type of the underlying exception. We should print the original exception ToString() not just the Message.

@KirillOsenkov KirillOsenkov added bug needs-triage Have yet to determine what bucket this goes in. Task: Resolve Assembly References (RAR) Area: Debuggability Issues impacting the diagnosability of builds, including logging and clearer error messages. Area: Logging labels Mar 4, 2021
@benvillalobos
Copy link
Member

@KirillOsenkov are you investigating this currently?

/cc: @Forgind

@KirillOsenkov
Copy link
Member Author

No, this one is up-for-grabs if anyone’s interested

@Forgind
Copy link
Member

Forgind commented Mar 10, 2021

When you said there were four places that threw the exception, did you mean this? It looks like all the information is saved in an inner exception, so it should be easily accessible when it gets thrown here.

@Forgind
Copy link
Member

Forgind commented Mar 10, 2021

If so, does #6240 look about how you expected?

@KirillOsenkov
Copy link
Member Author

Yes, perfect.

@KirillOsenkov
Copy link
Member Author

And for completeness, I've found the underlying issue that was causing an AccessDenied exception. Zip on .NET doesn't preserve file permissions, so unpacking the Zip on Mac was creating files with 0 permissions instead of 644:
dotnet/runtime#17912 (comment)

@KirillOsenkov
Copy link
Member Author

Seeing that it was Access Denied and the full ToString() would have certainly helped.

@danmoseley
Copy link
Member

This looks a bit ugly to the user though

...warning MSB3246: Resolved file has a bad image, no metadata, or is otherwise inaccessible. System.BadImageFormatException: Assembly file 'C:\git\runtime\artifacts\bin\microsoft.netcore.app.ref\ref\net6.0\System.Security.Cryptography.Encoding.dll' could not be opened -- PE image doesn't contain managed metadata. [...]
...warning MSB3246:    at Microsoft.Build.Shared.AssemblyNameExtension.GetAssemblyNameEx(String path) [...]
...warning MSB3246:    at Microsoft.Build.Tasks.SystemState.GetAssemblyName(String path) [...]
...warning MSB3246:    at Microsoft.Build.Tasks.ReferenceTable.SetPrimaryAssemblyReferenceItem(ITaskItem referenceAssemblyName) [...]

do we want to show these internal frames here?

@KirillOsenkov
Copy link
Member Author

Not sure what’s the best way to truncate them: just keep the first text line from ToString() or take just the message from the innermost exception?

@danmoseley
Copy link
Member

I'm not sure what other variations look like or what information you expect the user to get from them? Maybe log the message normally, but reserve ToString (or StackTrace) for the highest verbosity logging? In this case, the dll was corrupted, when I see callstack from my build tool it makes me think there is a bug in it.

@KirillOsenkov
Copy link
Member Author

I agree showing the callstack is unpolished, just curious on how to actually fix this. Because there are nested exceptions so we should probably print the message from each nested exception?

@KirillOsenkov KirillOsenkov reopened this Aug 31, 2021
@danmoseley
Copy link
Member

For the aggregated message including nested you could concat something like [AggregateException does]?(https://github.com/dotnet/runtime/blob/fc854d9921475366875658180afa495d452451be/src/libraries/System.Private.CoreLib/src/System/AggregateException.cs#L346-L367)
I don't think it's the prettiest, but that's precedent.

For the diagnostic verbosity, you could dump ToSTring() which will include nested info.

@Forgind Forgind removed their assignment Aug 31, 2021
@Forgind
Copy link
Member

Forgind commented Aug 31, 2021

I'm not opposed to trying to make this clearer/simpler, but it seems lower priority than getting the information out, so I removed my assignment.

I'm curious if this could be an opportunity to use the check-verbosity-before-logging idea. Diagnostic --> current behavior. Quiet/minimal --> nothing? Normal --> message from innermost exception?

@Forgind Forgind added the needs-design Requires discussion with the dev team before attempting a fix. label Aug 31, 2021
@danmoseley
Copy link
Member

I guess I meant log the error with the messages, and immediately follow with a low importance message event with the ToString(), perhaps.

@benvillalobos benvillalobos removed the needs-triage Have yet to determine what bucket this goes in. label Sep 2, 2021
@danmoseley
Copy link
Member

I just hit this a few times building dotnet/runtime. The output folder was "slightly corrupt" I guess. I believe a tool that dumps stack to the console looks like it has a bug. I put up a fix.

JaynieBai pushed a commit that referenced this issue Jan 6, 2023
Fixes #6224

Context
See issue -- a change was made to add more detail when RAR fails to resolve a reference due to encountering a BIFE. This added too much detail -- not only the message from the inner exception, but also the callstack. We should not dump callstack when there is no bug in MSBuild (or a task/logger).

Oddly enough I hit this again #8140 and didn't remember the original discussion, and therefore again assumed there was a bug.

Changes Made
Fix BIFE to include the message from any inner exception. (There was a suggestion in the issue that it also include the exception type, but I don't know of any case where the type is necessary in order to disambiguate what happened, and we don't have precedent for doing it. It would also be ugly.)
Revert the original change, now unnecesssary.

Testing
Added test that fails without the fix.
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Debuggability Issues impacting the diagnosability of builds, including logging and clearer error messages. Area: Logging bug needs-design Requires discussion with the dev team before attempting a fix. Task: Resolve Assembly References (RAR) triaged
Projects
None yet
5 participants