Unify decoding PE headers and reading other PE sections

[tmat]

PEHeaders are currently decoded using (customized) BinaryReader while the rest of the PE file is read using blob readers. We should use blob readers everywhere to avoid doing things in two different ways. As a by-product reading PE headers from non-zero position in the stream would just work. Currently it's broken.

[tmat]

See skipped test PEReaderTests.SubStream.

[karelz]

Be warned: This is rather tricky. You have to be careful about allocation lifetime, memory mapping, etc.

[danmoseley]

@tmat I bulk edited disabled tests to 2.0. Does this one actually need fixing in 2.0? It sounds like an enhancement. Can you change milestone if appropraite?

[steveharter]

As a by-product reading PE headers from non-zero position in the stream would just work. Currently it's broken.

When would the position be non-zero?

This is a minor refactoring task that appears valid but low priority.

[teo-tsirpanis]

I took a look at this. In most cases the stream we've been reading from is in one way or another a wrapping of the underlying AbstractMemoryBlock, so we could easily use a BlobReader in place of a PEBinaryReader. The only case where we directly read from the user-provided stream is when we create a PEReader with PEStreamOptions.PrefetchMetadata and no other flag.

One way to unify the encoding without regressing performance for this scenario, is by introducing an internal IBinaryReader interface that is implemented by both BlobReader and PEBinaryReader, and changing all ref PEBinaryReader to ref TReader, where TReader : IBinaryReader. I am working on this and will open a PR soon.

reading PE headers from non-zero position in the stream [is] broken.

Maybe changing offset to _startOffset + offset in this line would fix it?

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

Line 43 in 010e61a

_reader.BaseStream.Seek(offset, SeekOrigin.Begin);