Check overlaps

[CreateAndInject]

When I saved a file, I got an invalid .NET file, since COR20 header signature is rewritten.

[ElektroKill]

When I saved a file, I got an invalid .NET file, since COR20 header signature is rewritten.

Hello, could you provide a way to reproduce this bug?

[ElektroKill]

If we have an issue with chunks that cannot be reused in reusedChunks collection it would be better to investigate why the CanReuse method is allowing them to be reused rather than implementing yet another sanity check!

[CreateAndInject]

When I saved a file, I got an invalid .NET file, since COR20 header signature is rewritten.

Hello, could you provide a way to reproduce this bug?

@wtfsck @ElektroKill

Test.zip
Create a console application, add a string resource, and write code:

        static void Main(string[] args)
        {
            Console.WriteLine(Properties.Resources.String1);
            Console.ReadKey();
        }

Build it, and load in dnSpy:

As we can see:
The Resource is start at the end of Metadata (0x2100+ 0x904= 0x2A04)
Change Metadata.Size from 0x904 to 0x914, save it, the saved file is still runnable.
Load the saved file to dnSpy, change the entry point name from Main to IncreaseMetadataSize, save it with Mixed-Mode Module option checked. This time, the output file is no longer runnable, since metadata is rewritten when writing resource

[ElektroKill]

When I saved a file, I got an invalid .NET file, since COR20 header signature is rewritten.

Hello, could you provide a way to reproduce this bug?

@wtfsck @ElektroKill

Test.zip Create a console application, add a string resource, and write code:

        static void Main(string[] args)
        {
            Console.WriteLine(Properties.Resources.String1);
            Console.ReadKey();
        }

Build it, and load in dnSpy:

As we can see: The Resource is start at the end of Metadata (0x2100+ 0x904= 0x2A04) Change Metadata.Size from 0x904 to 0x914, save it, the saved file is still runnable. Load the saved file to dnSpy, change the entry point name from Main to IncreaseMetadataSize, save it with Mixed-Mode Module option checked. This time, the output file is no longer runnable, since metadata is rewritten when writing resource

Hello, I tried to reproduce this issue by running the following code:

var mod = ModuleDefMD.Load("MetadataSizeChanged.exe");

var modOpts = new NativeModuleWriterOptions(mod, true);
mod.NativeWrite("final2.exe", modOpts);

and also by opening MetadataSizeChanged.exe in dnSpyEx 6.5.0-rc1 and saving it after enabling Mixed-Mode module and both ways produced working files. I'm unable to reproduce the bug you are describing here!

[ElektroKill]

My bad, I missed the part about the entry point name change! I'm now able to reproduce the issue and will look further into it!

[ElektroKill]

I came up with a much simpler solution for this issue of overlapping reused chunks (the check regarding overlapping could do some work).

Add this code to ReuseIfPossible

var origEnd = origRva + origSize;
foreach (var reusedChunk in reusedChunks) {
	var start = reusedChunk.RVA;
	var end = start + reusedChunk.Chunk.GetVirtualSize();
	if (start <= origRva && end > origRva)
		return;
	if (start <= origEnd && end > origEnd)
		return;
	if (origRva <= start && origEnd > start)
		return;
	if (origRva <= end && origEnd > end)
		return;
}

after

if (origRva == 0 || origSize == 0)
	return;
if (chunk is null)
	return;
if (!chunk.CanReuse(origRva, origSize))
	return;
if (((uint)origRva & (requiredAlignment - 1)) != 0)
	return;

[CreateAndInject]

Seems there're unnecessary so many checks:

	if (start <= origRva && end > origRva)
		return;
	if (start <= origEnd && end > origEnd)
		return;
	if (origRva <= start && origEnd > start)
		return;
	if (origRva <= end && origEnd > end)
		return;

=>

	if (start < origEnd && end > origRva)
		return;

[ElektroKill]

Seems there're unnecessary so many checks:

	if (start <= origRva && end > origRva)
		return;
	if (start <= origEnd && end > origEnd)
		return;
	if (origRva <= start && origEnd > start)
		return;
	if (origRva <= end && origEnd > end)
		return;

=>

	if (start < origEnd && end > origRva)
		return;

Yes, indeed.

Here is the new ReuseIfPossible method which contains the fix:

void ReuseIfPossible(PESection section, IReuseChunk chunk, RVA origRva, uint origSize, uint requiredAlignment) {
	if (origRva == 0 || origSize == 0)
		return;
	if (chunk is null)
		return;
	if (!chunk.CanReuse(origRva, origSize))
		return;
	if (((uint)origRva & (requiredAlignment - 1)) != 0)
		return;

	var origEnd = origRva + origSize;
	foreach (var reusedChunk in reusedChunks) {
		var start = reusedChunk.RVA;
		var end = start + reusedChunk.Chunk.GetVirtualSize();
		if (start < origEnd && end > origRva)
			return;
	}

	if (section.Remove(chunk) is null)
		throw new InvalidOperationException();
	reusedChunks.Add(new ReusedChunkInfo(chunk, origRva));
}

This fix is better than the one currently in the PR as it will only prevent the usage of overlapping chunks but will still reuse chunks that don't overlap.

@wtfsck What do you think?

[CreateAndInject]

Is the red area will keep in the PE, but the data is useless?

[ElektroKill]

Is the red area will keep in the PE, but the data is useless?

Yes, the data in the region marked in red will be kept in the output file. This is by design for the native writer as the native writer reuses the original PE file and just adds/overwrites the old data with new data. If optimizeImageSize parameter is set to false when writing then all old data in the file will be kept and all new data will just be added in. Enabling optimizeImageSize allows dnlib to overwrite some older data with the new data if the place is appropriate.

[CreateAndInject]

Is GetFileLength better than GetVirtualSize?
It's different in DataReaderChunk(Although they aways have the same value in reusedChunks)

[ElektroKill]

Is GetFileLength better than GetVirtualSize?
It's different in DataReaderChunk(Although they aways have the same value in reusedChunks)

I used GetVirtualSize() since in ReuseIfPossible we are dealing with RVAs rather than file offsets.

[wtfsck]

@wtfsck What do you think?

@ElektroKill Looks good assuming it fixed the repro above.

@CreateAndInject Could you add that fix instead?

[CreateAndInject]

OK

[wtfsck]

Thanks, it's been merged!