Closed CreateAndInject closed 8 months ago
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?
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!
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
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
toIncreaseMetadataSize
, save it withMixed-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!
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!
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;
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;
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?
Is the red area will keep in the PE, but the data is useless?
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.
Is GetFileLength
better than GetVirtualSize
?
It's different in DataReaderChunk(Although they aways have the same value in reusedChunks)
Is
GetFileLength
better thanGetVirtualSize
? 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 What do you think?
@ElektroKill Looks good assuming it fixed the repro above.
@CreateAndInject Could you add that fix instead?
OK
Thanks, it's been merged!
When I saved a file, I got an invalid .NET file, since COR20 header signature is rewritten.