Runtime: System.Reflection.Metadata throws BadImageFormatException on bad input

[ericstj]

If a user passes in a bad handle, for instance they get an ExportedTypeHandle from one MetadataReader instance's MetadataReader.ExportedTypes, and pass it to a different MetadataReader instance via MetadataReader.GetExportedType we will throw BadImageFormatException. This is unintuitive since both images are fine and the problem was with the argument passed to GetExportedType. We should probably be throwing something derived from ArgumentException, like ArgumentOutOfRangeException or something similar.

/cc @nguerrera @tmat

[tmat]

This is by design for performance reasons. Handles do not know their owning reader. We also don't want to add extra checks.

[nguerrera]

I agree here. IMHO, it is an example of why this low-level API can be harder to use and may not be best suited to scenarios where you can afford significant slowdown in exchange for better usability and diagnostics.

Note that you won't necessarily get a BadImageFormatException, you might also get something from the same cell in the other reader's metadata.

There are also API to produce handles from metadata tokens or row indices, which could no longer work if the resulting handle needs to be affinitized to a reader.

[nguerrera]

Another thing is if we ignore the reader affinity check, it could still be considered reasonable to complain about the handle when it is out of range (ArgumentException) of the reader's metadata in Xxx instead of complaining about the metadata being invalid (BadImageFormatException). The problem with that, however, is that we cannot really distinguish between the common case of a bad token in the image leading to a bad handle, leading to a read from bad handle, and a bad handle that the user conjured up some other way.

[ericstj]

I wasn't thinking to add reader affinity. I was thinking that when accessing a row ID into a table we could examine for bounds and throw, rather than letting the MemoryBlock fail when trying to read the table.

For example, in the constructor:
https://github.com/dotnet/corefx/blob/21278249c6b417469737b20445a0dd045cd1b16e/src/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeSystem/ExportedType.cs#L16-L23

We could check

if (rowId > reader.ExportedTypeTable.NumberOfRows)
  throw new ArgumentOutOfRangeException();

Do you think this will impact perf?

@nguerrera is your concern here that BIFE is more appropriate if this out of range value came from another table in the same image?

[nguerrera]

is your concern here that BIFE is more appropriate if this out of range value came from another table in the same image?

Yes, this is my concern in my second comment. It is very common for the bad handle to come from a bad image.

[nguerrera]

 if (rowId > reader.ExportedTypeTable.NumberOfRows)
  throw new ArgumentOutOfRangeException();

I think this will impact perf as well. The fetch from a handle to data was extremely optimized to the point of spending many iterations on getting the native code gen just right. Adding another bounds check will definitely show up in scenarios fetching from handles in a tight loop, which are real scenarios in the compiler.

[ericstj]

That's fair, I guess we just need to better document the S.R.M APIs that they can throw BIFE when given bad data. I'll repurpose this as a doc bug.
https://docs.microsoft.com/en-us/dotnet/api/system.reflection.metadata.metadatareader.getexportedtype?view=netcore-3.0

For that matter we need to better document all these APIs.