Fix unnecessary failure when Crossgen'ing dll's without proper Debug Directory data #73824

ivdiazsa · 2022-08-11T22:41:57Z

Fixes #72425. When trying to use Crossgen2 with dll's that do not have the proper debug directory data (e.g. obfuscated dll's), a BadImageException error is thrown and thus failing the process. In this case, the dll is still usable though debugging tools will probably not work. So, instead of failing, we ought to let the user continue but we shall warn them about this. As a history tidbit, this was the default behavior in .NET 5.

…d by Crossgen2. Instead of failing unnecessarily, it now emits a warning.

mangod9 · 2022-08-11T22:59:06Z

src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.cs

+                    }
+                    catch (BadImageFormatException e)
+                    {
+                        if (e.Message.Equals("Invalid directory size."))


guess this can be factored out to avoid duplication?

mangod9 · 2022-08-11T23:00:31Z

...tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DebugDirectoryNode.cs

+                    // This comes from the Size property of the DebugDirectoryEntry class.
+                    int expectedDbgDirSizeBase = 28;
+
+                    Console.WriteLine("WARNING: This image's Debug Directory Entry size should be a"


these can be created as exception strings instead of string literals?

I agree we should make this an exception string. I would throw away the "artificial intelligence" analyzing the exception message and just print something generic like "WARNING: %s contains invalid debug information: %s" with the name of the file and the message of the exception caught.

I think we can afford to keep the part that says "Debugging tools may not work correctly with this image". I feel our users would like to know what the warning entails for them.

mangod9 · 2022-08-11T23:01:38Z

...tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DebugDirectoryNode.cs

+            }
+            catch (BadImageFormatException e)
+            {
+                if (e.Message.Equals("Invalid directory size."))


not sure if this should be special cased, since on non-english locales this would probably fail anyways

I special cased this as well because it failed otherwise with the same BadImageFormatException.

Since e.Message will be localized, this check will not work for any language except English.

trylek

Mostly looks good, thanks Ivan! I concur with Manish that we should somewhat simplify the warning logic, I have shared my thoughts on that in the PR comments.

AntonLapounov · 2022-08-11T23:42:45Z

Could we create a helper method like below and use it instead of ReadDebugDirectory everywhere?

public static ImmutableArray<DebugDirectoryEntry> ReadDebugDirectoryIgnoringErrors(this PEReader moduleReader)
{
    try
    {
        return moduleReader.ReadDebugDirectory();
    }
    catch (BadImageFormatException e)
    {
        // Report a warning using e.Message
        return ImmutableArray<DebugDirectoryEntry>.Empty;
    }
}

ivdiazsa · 2022-08-12T00:12:45Z

Thank you all for the feedback! I have a few questions whatsoever.

First, I'm a big advocate of helper functions as long as it doesn't end up being a big scramble of chained functions (which this is clearly not the case). My question I would like guidance on is where would be the most appropriate place/file to write the helper function, since these two files are in different csproj's. One is in ILCompiler.Compiler and the other one is in ILCompiler.ReadyToRun.

My other question is regarding the string literal, and it's in a very similar boat. The original throwing function in System.Reflection.Metadata, gets the string from a class called SR. Here is the original throwing line:

runtime/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.cs

Lines 541 to 544 in 10438a5

    
           if (debugDirectory.Size % DebugDirectoryEntry.Size != 0) 
        
           { 
        
               throw new BadImageFormatException(SR.InvalidDirectorySize); 
        
           }

This SR class then called a method named GetResourceString() to fetch it from here:

runtime/src/libraries/System.Reflection.Metadata/src/Resources/Strings.resx

Lines 270 to 272 in d34c0ff

    
           <data name="InvalidDirectorySize" xml:space="preserve"> 
        
             <value>Invalid directory size.</value> 
        
           </data>

I might have missed something, but I couldn't get any sort of access to there from our codebase. So, we might just have to create a constant like you mentioned, but my question is where would be most appropriate.

AntonLapounov · 2022-08-12T00:20:03Z

Would it be possible to add both the helper and the resource string to a shared assembly, e.g., src\coreclr\tools\Common\TypeSystem\Common\Properties\Resources.resx?

trylek · 2022-08-12T00:21:19Z

@ivdiazsa - what you're looking at is standardized support for localized resources. Crossgen2 uses them too, e.g. here:

runtime/src/coreclr/tools/aot/crossgen2/CommandLineOptions.cs

Line 152 in 48ce77b

    
           syntax.DefineOptionList("codegenopt|codegen-options", ref CodegenOptions, SR.CodeGenOptions);

This is coming from the file

runtime/src/coreclr/tools/aot/crossgen2/Properties/Resources.resx

Line 126 in 48ce77b

that you can edit with VS (or manually). Having said that, proper location depends on the assembly you put the helper in. Are you sure that you see a failure in ILCompiler.Compiler? I was under the impression that is only used by NativeAOT.

AntonLapounov · 2022-08-12T00:24:49Z

Are you sure that you see a failure in ILCompiler.Compiler?

It would be nice to have the same behavior for both. And there are other places that should be fixed, e.g.,

runtime/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DebugDirectoryEntryNode.cs

Line 253 in ad60e44

    
           ImmutableArray<DebugDirectoryEntry> entries = _module.PEReader.ReadDebugDirectory();

trylek · 2022-08-12T00:30:55Z

If we want a common fix for Crossgen2 and NativeAOT, it should be probably under src/coreclr/tools/Common that is shared between them. Perhaps this file seems loosely related:

https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/Common/System/FormattingHelpers.cs

jkotas · 2022-08-12T02:32:04Z

src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.cs

+
+                            Console.WriteLine("WARNING: This image's Debug Directory Entry size should be a"
+                                            + $" multiple of {expectedDbgDirSizeBase}, but was {actualDbgDirSize}."
+                                            + " Debugging tools may not work correctly with this image.");


Suggested change

+ " Debugging tools may not work correctly with this image.");

+ " Diagnostic tools may not work correctly with this image.");

It is also profilers, etc.

jkotas · 2022-08-12T02:36:44Z

src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.cs

+                    }
+                    catch (BadImageFormatException e)
+                    {
+                        if (e.Message.Equals("Invalid directory size."))


There are many other ways that the debug symbols can be corrupted, we just have not seen them. Should we catch all exceptions here and print that the debug symbols are corrupted, and include the error message from the exception?

There are many other ways that the debug symbols can be corrupted, we just have not seen them. Should we catch all exceptions here and print that the debug symbols are corrupted, and include the error message from the exception?

None of the compiler is hardened for obfuscators. I wouldn't treat debug information very differently.

(We might successfully open the debug information but there can be exceptions once we start looking into it.)

MichalStrehovsky · 2022-08-12T02:38:43Z

src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.cs

+                    try
+                    {
+                        pdbReader = PortablePdbSymbolReader.TryOpenEmbedded(peReader, GetMetadataStringDecoder())
+                                    ?? OpenAssociatedSymbolFile(filePath, peReader);
+                    }
+                    catch (BadImageFormatException e)


This is placed in a very broad spot. A lot of code is going to run and could throw a BadImageFormat (S.R.Metadata will throw it for any sort of corruption).

Can you instead add a e.g. SafeReadDebugDirectory(this PEReader peReader) extension method that simply checks the directory size before calling the API and returns ImmutableArray<DebugDirectoryEntry>.Empty if it doesn't make sense?

But doing Console.WriteLine in arbitrary spots is not a great practice because we're multithreaded. I would honestly just swallow this.

I did not consider the fact of multithreading. Thanks for your feedback Michal! I think this is a great suggestion, I'll get to work on it.

…ng with exceptions and regional issues.

AntonLapounov · 2022-08-12T22:02:05Z

Thank you, that is much cleaner. I think we should also fix one more place I mentioned above:

runtime/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DebugDirectoryEntryNode.cs

Line 253 in ad60e44

    
           ImmutableArray<DebugDirectoryEntry> entries = _module.PEReader.ReadDebugDirectory();

There are also two occurrences of ReadDebugDirectory() under src\coreclr\tools\dotnet-pgo.

AntonLapounov · 2022-08-12T22:03:41Z

src/coreclr/tools/Common/TypeSystem/Common/PortableExecutableMethodExtensions.cs

+            int actualDbgDirSize = peReader.PEHeaders.PEHeader.DebugTableDirectory.Size;
+
+            // This comes from the Size property of the DebugDirectoryEntry class.
+            int expectedDbgDirSizeBase = 28;


Nit: Might use const here:

Suggested change

int expectedDbgDirSizeBase = 28;

const int ExpectedDbgDirSize = 28;

ivdiazsa · 2022-08-12T22:06:32Z

Thank you, that is much cleaner. I think we should also fix one more place I mentioned above:

runtime/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DebugDirectoryEntryNode.cs

Line 253 in ad60e44

ImmutableArray<DebugDirectoryEntry> entries = _module.PEReader.ReadDebugDirectory();

There are also two occurrences of ReadDebugDirectory() under src\coreclr\tools\dotnet-pgo.

Sure thing. I wasn't sure whether I should be meddling with PGO as well, but you're right, it doesn't seem like it would break anything in this case.

AntonLapounov · 2022-08-12T22:09:00Z

It would be nice to have consistent behavior at least within our own AoT toolchain.

…is codebase.

AntonLapounov

Thank you for fixing this!

Added a check to allow DLL's with bad debug directory sizes to be use…

8e6817b

…d by Crossgen2. Instead of failing unnecessarily, it now emits a warning.

ivdiazsa requested review from AntonLapounov, davidwrighton, mangod9 and trylek August 11, 2022 22:41

ivdiazsa requested a review from MichalStrehovsky as a code owner August 11, 2022 22:41

ghost added the area-crossgen2-coreclr label Aug 11, 2022

ghost assigned ivdiazsa Aug 11, 2022

mangod9 reviewed Aug 11, 2022

View reviewed changes

trylek reviewed Aug 11, 2022

View reviewed changes

jkotas reviewed Aug 12, 2022

View reviewed changes

MichalStrehovsky reviewed Aug 12, 2022

View reviewed changes

runfoapp bot mentioned this pull request Aug 12, 2022

Infra improvements for Helix #68176

Closed

Implemented the suggested PEReader extension method, instead of deali…

62ff43d

…ng with exceptions and regional issues.

Removed unnecessary 'using' statement.

5e6b75d

AntonLapounov reviewed Aug 12, 2022

View reviewed changes

Fixed the remaining 'unsafe' Debug Directory reading points within th…

ed860f4

…is codebase.

AntonLapounov approved these changes Aug 12, 2022

View reviewed changes

Added a missing 'const' modifier for a constant value.

456409b

ivdiazsa merged commit 9814b1b into dotnet:main Aug 15, 2022

ivdiazsa deleted the dealing_with_dlls branch August 15, 2022 16:30

ghost locked as resolved and limited conversation to collaborators Sep 14, 2022

	+ " Debugging tools may not work correctly with this image.");
	+ " Diagnostic tools may not work correctly with this image.");

	int expectedDbgDirSizeBase = 28;
	const int ExpectedDbgDirSize = 28;

Fix unnecessary failure when Crossgen'ing dll's without proper Debug Directory data #73824

Fix unnecessary failure when Crossgen'ing dll's without proper Debug Directory data #73824

Uh oh!

Conversation

ivdiazsa commented Aug 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivdiazsa Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

AntonLapounov commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivdiazsa commented Aug 12, 2022

Uh oh!

AntonLapounov commented Aug 12, 2022

Uh oh!

trylek commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AntonLapounov commented Aug 12, 2022

Uh oh!

trylek commented Aug 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AntonLapounov commented Aug 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivdiazsa commented Aug 12, 2022

Uh oh!

AntonLapounov commented Aug 12, 2022

Uh oh!

AntonLapounov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ivdiazsa Aug 11, 2022 •

edited

Loading

AntonLapounov commented Aug 11, 2022 •

edited

Loading

trylek commented Aug 12, 2022 •

edited

Loading