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

Use EmptyErrorListener as the Default Reader Error Handler #472

Open
Washi1337 opened this issue Aug 1, 2023 · 4 comments
Open

Use EmptyErrorListener as the Default Reader Error Handler #472

Washi1337 opened this issue Aug 1, 2023 · 4 comments
Milestone

Comments

@Washi1337
Copy link
Owner

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.

@Washi1337 Washi1337 added this to the 6.0.0 milestone Aug 1, 2023
@ds5678
Copy link
Contributor

ds5678 commented Aug 1, 2023

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
Copy link
Owner Author

Washi1337 commented Aug 1, 2023

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
Copy link

1point7 commented Aug 2, 2023

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
Copy link
Owner Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants