Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FieldRVA alignment #817

Merged
merged 3 commits into from
Jan 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion Mono.Cecil.Metadata/Buffers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -256,17 +256,33 @@ public uint AddResource (byte [] resource)

sealed class DataBuffer : ByteBuffer {

int buffer_align = 4;

public DataBuffer ()
: base (0)
{
}

public RVA AddData (byte [] data)
void Align (int align)
{
align--;
// Compute the number of bytes to align the current position.
// Values of 0 will be written.
WriteBytes (((position + align) & ~align) - position);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worthwhile to unpack this logic in a comment.

davidwrighton marked this conversation as resolved.
Show resolved Hide resolved
}

public RVA AddData (byte [] data, int align)
{
if (buffer_align < align)
buffer_align = align;

Align (align);
var rva = (RVA) position;
WriteBytes (data);
return rva;
}

public int BufferAlign => buffer_align;
}

abstract class HeapBuffer : ByteBuffer {
Expand Down
2 changes: 1 addition & 1 deletion Mono.Cecil.PE/ImageWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ void BuildTextMap ()

map.AddMap (TextSegment.Code, metadata.code.length, !pe64 ? 4 : 16);
map.AddMap (TextSegment.Resources, metadata.resources.length, 8);
map.AddMap (TextSegment.Data, metadata.data.length, 4);
map.AddMap (TextSegment.Data, metadata.data.length, metadata.data.BufferAlign);
if (metadata.data.length > 0)
metadata.table_heap.FixupData (map.GetRVA (TextSegment.Data));
map.AddMap (TextSegment.StrongNameSignature, GetStrongNameLength (), 4);
Expand Down
15 changes: 14 additions & 1 deletion Mono.Cecil/AssemblyWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1613,8 +1613,21 @@ void AddField (FieldDefinition field)
void AddFieldRVA (FieldDefinition field)
{
var table = GetTable<FieldRVATable> (Table.FieldRVA);

// To allow for safe implementation of metadata rewriters for code which uses CreateSpan<T>
// if the Field RVA refers to a locally defined type with a pack > 1, align the InitialValue
// to pack boundary. This logic is restricted to only being affected by metadata local to the module
// as PackingSize is only used when examining a type local to the module being written.

int align = 1;
if (field.FieldType.IsDefinition && !field.FieldType.IsGenericInstance) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that the IsDefinition is enough to check that it's in the same assembly. It will be true for object read from IL, but it might not be true for objects modified or created programatically. As far as I know it's perfectly valid to assign a TypeDefinition to the FieldType.

var type = field.FieldType.Resolve ();

if ((type.Module == module) && (type.PackingSize > 1))
align = type.PackingSize;
}
table.AddRow (new FieldRVARow (
data.AddData (field.InitialValue),
data.AddData (field.InitialValue, align),
field.token.RID));
}

Expand Down
44 changes: 44 additions & 0 deletions Test/Mono.Cecil.Tests/FieldTests.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.IO;

using Mono.Cecil.PE;

Expand Down Expand Up @@ -133,6 +134,49 @@ public void FieldRVA ()
});
}

int AlignmentOfInteger(int input)
{
if (input == 0)
return 0x40000000;
if (input < 0)
Assert.Fail ();
int alignment = 1;
while ((input & alignment) == 0)
alignment *= 2;

return alignment;
}

[Test]
public void FieldRVAAlignment ()
{
TestIL ("FieldRVAAlignment.il", ilmodule => {

var path = Path.GetTempFileName ();

ilmodule.Write (path);

using (var module = ModuleDefinition.ReadModule (path, new ReaderParameters { ReadWrite = true })) {
var priv_impl = GetPrivateImplementationType (module);
Assert.IsNotNull (priv_impl);

Assert.AreEqual (6, priv_impl.Fields.Count);

foreach (var field in priv_impl.Fields)
{
Assert.IsNotNull (field);

Assert.AreNotEqual (0, field.RVA);
Assert.IsNotNull (field.InitialValue);

int rvaAlignment = AlignmentOfInteger (field.RVA);
int desiredAlignment = Math.Min(8, AlignmentOfInteger (field.InitialValue.Length));
Assert.GreaterOrEqual (rvaAlignment, desiredAlignment);
}
}
});
}

[Test]
public void GenericFieldDefinition ()
{
Expand Down
71 changes: 71 additions & 0 deletions Test/Resources/il/FieldRVAAlignment.il
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
.assembly extern mscorlib
{
.publickeytoken = (B7 7A 5C 56 19 34 E0 89 ) // .z\V.4..
.ver 4:0:0:0
}
.assembly FieldRVAAlignment {}

.module FieldRVAAlignment.dll

.class private auto ansi '<PrivateImplementationDetails>{9B33BB20-87EF-4094-9948-34882DB2F001}'
extends [mscorlib]System.Object
{
.custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )
.class explicit ansi sealed nested private '__StaticArrayInitTypeSize=3'
extends [mscorlib]System.ValueType
{
.pack 1
.size 3
} // end of class '__StaticArrayInitTypeSize=3'

.class explicit ansi sealed nested private '__StaticArrayInitTypeSize=16'
extends [mscorlib]System.ValueType
{
.pack 8
.size 16
} // end of class '__StaticArrayInitTypeSize=16'

.class explicit ansi sealed nested private '__StaticArrayInitTypeSize=20'
extends [mscorlib]System.ValueType
{
.pack 4
.size 20
} // end of class '__StaticArrayInitTypeSize=20'

.class explicit ansi sealed nested private '__StaticArrayInitTypeSize=5'
extends [mscorlib]System.ValueType
{
.pack 1
.size 5
} // end of class '__StaticArrayInitTypeSize=5'

.class explicit ansi sealed nested private '__StaticArrayInitTypeSize=6'
extends [mscorlib]System.ValueType
{
.pack 2
.size 6
} // end of class '__StaticArrayInitTypeSize=6'

.field static assembly valuetype '<PrivateImplementationDetails>{9B33BB20-87EF-4094-9948-34882DB2F001}'/'__StaticArrayInitTypeSize=3' '$$method0x6000001-1' at I_000020F0
.field static assembly valuetype '<PrivateImplementationDetails>{9B33BB20-87EF-4094-9948-34882DB2F001}'/'__StaticArrayInitTypeSize=20' '$$method0x6000001-2' at I_000020F8
.field static assembly valuetype '<PrivateImplementationDetails>{9B33BB20-87EF-4094-9948-34882DB2F001}'/'__StaticArrayInitTypeSize=5' '$$method0x6000001-3' at I_00002108
.field static assembly valuetype '<PrivateImplementationDetails>{9B33BB20-87EF-4094-9948-34882DB2F001}'/'__StaticArrayInitTypeSize=20' '$$method0x6000001-4' at I_00002110
.field static assembly valuetype '<PrivateImplementationDetails>{9B33BB20-87EF-4094-9948-34882DB2F001}'/'__StaticArrayInitTypeSize=6' '$$method0x6000001-5' at I_00002120
.field static assembly valuetype '<PrivateImplementationDetails>{9B33BB20-87EF-4094-9948-34882DB2F001}'/'__StaticArrayInitTypeSize=16' '$$method0x6000001-6' at I_00002130
} // end of class '<PrivateImplementationDetails>{9B33BB20-87EF-4094-9948-34882DB2F001}'


// =============================================================

.data cil I_000020F0 = bytearray (
01 02 03)
.data cil I_000020F8 = bytearray (
01 00 00 00 02 00 00 00 03 00 00 00 04 00 00 00 05 00 00 00)
.data cil I_00002108 = bytearray (
04 05 06 07 08)
.data cil I_00002110 = bytearray (
01 00 00 00 02 00 00 00 03 00 00 00 05 00 00 00 06 00 00 00)
.data cil I_00002120 = bytearray (
08 00 0C 00 0D 00)
.data cil I_00002130 = bytearray (
01 00 00 00 02 00 00 00 03 00 00 00 05 00 00 00)