0xd4d / dnlib

Reads and writes .NET assemblies and modules
MIT License
2.14k stars 583 forks source link

Align field initial value to match the Microsoft's ECMA augments #518

Closed ElektroKill closed 1 year ago

ElektroKill commented 1 year ago

Current changes:

Things left to do:

I've also been wondering if chunks like StrongNameSignature or Win32ResourcesChunk should define a CalculateAlignment which returns the default alignments used when adding them to their respective chunk lists to better reflect the alignment they necessitate to the user.

fixes #454

wtfsck commented 1 year ago

Run tests to see if the implementation works :p

Some simple tests would be to use the old and new dnlib on some random dlls and compare the output. It should produce the exact same output unless there's some new data that has an alignment > previous default value.

I've also been wondering if chunks like StrongNameSignature or Win32ResourcesChunk should define a CalculateAlignment which returns the default alignments used when adding them to their respective chunk lists to better reflect the alignment they necessitate to the user.

It's not really needed but doesn't hurt either if you really want to do it.

ElektroKill commented 1 year ago

Run tests to see if the implementation works :p

Some simple tests would be to use the old and new dnlib on some random dlls and compare the output. It should produce the exact same output unless there's some new data that has an alignment > previous default value.

Will run such tests when I complete the implementation :p

I've also been wondering if chunks like StrongNameSignature or Win32ResourcesChunk should define a CalculateAlignment which returns the default alignments used when adding them to their respective chunk lists to better reflect the alignment they necessitate to the user.

It's not really needed but doesn't hurt either if you really want to do it.

I think I will go ahead and do this to avoid potential confusion when the user calls the methods on those chunks and expects their alignment to be returned :p

The chunks modified would be:

Regarding point number 3 on my TODO list, I'm wondering if it is even necessary to add... The code after the modification would look like this:

protected void CalculateRvasAndFileOffsets(List<IChunk> chunks, FileOffset offset, RVA rva, uint fileAlignment, uint sectionAlignment) {
    int count = chunks.Count;
    for (int i = 0; i < count; i++) {
        var chunk = chunks[i];
        var alignment = chunk.CalculateAlignment();
        if (alignment > 0) {
            offset = offset.AlignUp(alignment);
            rva = rva.AlignUp(alignment);
        }

        chunk.SetOffset(offset, rva);
        // If it has zero size, it's not present in the file (eg. a section that wasn't needed)
        if (chunk.GetVirtualSize() != 0) {
            offset += chunk.GetFileLength();
            rva += chunk.GetVirtualSize();
            offset = offset.AlignUp(fileAlignment);
            rva = rva.AlignUp(sectionAlignment);
        }
    }
}

However, after some additional analysis of the code which populates the List<IChunk> that gets passed into this method, it seems that only two types of chunks are added. A PEHeaders chunk with a required alignment of 0, a PESection chunk which derives from ChunListBase<T> which already computes and aligns up the nested chunks properly, or a DataReaderChunk in NativeModuleWriter which is directly copied from the source module and thus does not need any extra alignment processing. This makes this use of the result of chunk.CalculateAlignment() and subsequent alignment of the offsets unnecessary. The rationale for keeping it would be to make adding chunks with custom alignment to the top-level list easier. A potential reason against this would be a waste of space necessary to align a potentially unordered PESection to the maximum alignment of every chunk.

As for potentially sorting the chunk lists, I'm still not sure whether we should make a new type along the lines of SortedChunkList or bake the behavior into UniqueChunkList. I'm also not really sure what kind of sorting we go with to guarantee the best file size. Is a simple descending sort on alignment good enough for this purpose?

wtfsck commented 1 year ago

or bake the behavior into UniqueChunkList

It's easiest and no-one assumes any order so can just be added at the end of its CalculateAlignment() after it calls the base CalculateAlignment() method. I'd just sort it first by alignment (biggest alignment first), then by the order it was added (so we always produce the same output and there should be no randomness).

ElektroKill commented 1 year ago

I have tested the implementation by rewriting dnlib with the old version and the new version and the outputs are identical. I have also created a file with a field that would need alignment and when rewriting with the new version it indeed gets aligned to the correct alignment while previously they were not.

wtfsck commented 1 year ago

Thanks, merged!