Washi1337 / AsmResolver

A library for creating, reading and editing PE files and .NET modules.
https://docs.washi.dev/asmresolver/
MIT License
872 stars 127 forks source link

Use `EmptyErrorListener` as the Default Reader Error Handler #472

Closed Washi1337 closed 5 months ago

Washi1337 commented 1 year ago

Problem Description

Many use-cases of AsmResolver will involve opening "weird" binaries. These include binaries that are either half-broken / corrupted, or intentionally made difficult to read properly as a means of obfuscation.

Currently, by default, AsmResolver will load PE files using a ThrowErrorListener object. This enables a very strict validation on the structure of the input PE and let AsmResolver throw an exception if anything looks even slightly out of order. Although this can be disabled (e.g., by setting this listener to an instance of EmptyErrorListener), this is not too obvious and/or is often forgotten. This can lead to confusion when trying to access properties (which will be throwing exceptions all of the sudden), or when errors / diagnostics are ignored during the rebuild stage of a file. This can also lead to the impression that either the user is doing something wrong, or the AsmResolver's PE builder has bugs, when neither is really the case.

Proposal

Make EmptyErrorListener the default error listener that is used for reading files.

Alternatives

Document better the behavior in which the reader and writer independently handle errors.

Additional Context

This is a breaking change.

ds5678 commented 1 year ago

I disagree with this direction because ignoring problems is something that should be opted into, rather than being an invisible default. An API, just like a programming language, should always strive to have a pit of success, where doing wrong things is more difficult than doing right things.

The documentation suggestion is good.

Another option might be to have an extra method explicitly named for code clarity, eg LoadWithoutErrorChecking. This has downsides, such as redundancy and a larger API surface.

Washi1337 commented 1 year ago

I agree with the sentiment of doing things right should be easier than doing things wrong. And that is exactly why I am thinking of introducing this change.

An input binary is not necessarily something that users can do "wrong" (unless the binary is a binary they produced themselves or they are passing in the wrong file). Forgetting to set a parameter, however, is something that can be done easily.

Keep in mind that for critical parser errors (such as the absence of a crucial PE header) would still result in an exception to be thrown even with this change applied.

1point7 commented 1 year ago

Hello. I'm not sure it is the right place. But I have found the related topic about Preservation of TypeRef tokens: https://github.com/Washi1337/AsmResolver/issues/329#issuecomment-1173159861 So I have a dumper. When I do try to preserve all metadata, I get an error. There are no errors when I comment 3 flags: PreserveTypeReferenceIndices, PreserveTypeDefinitionIndices and PreserveMemberReferenceIndices. But I would like to preserve them.

Here you recommend to use EmptyErrorListener.Instance, but it doesn't help: https://github.com/Washi1337/AsmResolver/issues/329#issuecomment-1178872325

My code: `var assembly = Assembly.LoadFrom(args[0]); var moduleHandle = assembly!.ManifestModule.ModuleHandle; RuntimeHelpers.RunModuleConstructor(moduleHandle); var moduleBaseAddress = Marshal.GetHINSTANCE(assembly.ManifestModule); AsmResolver.DotNet.Serialized.ModuleReaderParameters parameters = new AsmResolver.DotNet.Serialized.ModuleReaderParameters(AsmResolver.EmptyErrorListener.Instance); // Added as recommended _moduleDefinition = ModuleDefinition.FromModuleBaseAddress(moduleBaseAddress, parameters); var assemblyResolver = (AssemblyResolverBase)_moduleDefinition.MetadataResolver.AssemblyResolver; assemblyResolver.SearchDirectories.Add(Path.GetDirectoryName(assembly.Location));

/do my stuff/

var imageBuilder = new ManagedPEImageBuilder(MetadataBuilderFlags.PreserveAll); MetadataBuilderFlags mdbf =

             MetadataBuilderFlags.PreserveBlobIndices | MetadataBuilderFlags.PreserveGuidIndices | MetadataBuilderFlags.PreserveStringIndices |
             MetadataBuilderFlags.PreserveUserStringIndices | MetadataBuilderFlags.PreserveStreamOrder | MetadataBuilderFlags.NoStringsStreamOptimization | MetadataBuilderFlags.PreserveUnknownStreams
              | MetadataBuilderFlags.PreserveTypeReferenceIndices | MetadataBuilderFlags.PreserveTypeDefinitionIndices  |
              MetadataBuilderFlags.PreserveFieldDefinitionIndices | MetadataBuilderFlags.PreserveMethodDefinitionIndices | MetadataBuilderFlags.PreserveParameterDefinitionIndices |
              MetadataBuilderFlags.PreserveMemberReferenceIndices | MetadataBuilderFlags.PreserveStandAloneSignatureIndices | MetadataBuilderFlags.PreserveEventDefinitionIndices |
              MetadataBuilderFlags.PreservePropertyDefinitionIndices | MetadataBuilderFlags.PreserveModuleReferenceIndices | MetadataBuilderFlags.PreserveTypeSpecificationIndices
              | MetadataBuilderFlags.PreserveAssemblyReferenceIndices | MetadataBuilderFlags.PreserveMethodSpecificationIndices;

var factory = new DotNetDirectoryFactory(mdbf); factory.MethodBodySerializer = new CilMethodBodySerializer { ComputeMaxStackOnBuildOverride = false }; imageBuilder.DotNetDirectoryFactory = factory; string output = args[0].Insert(args[0].Length - 4, "-solved"); _moduleDefinition.Write(output, imageBuilder);`

Washi1337 commented 1 year ago

@1point7 Your code only ignores reader errors. Refer to the documentation on Image builder diagnostics to ignore writer errors. If you have further questions, please create a new issue, discussion post or join the Discord. Please do not hijack existing issue posts.