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.

Sign up for GitHub

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

Jump to bottom

Possible ArgumentOutOfRangeException when there are 65535 params in metadata table #913

Open
UlyssesWu opened this issue Jun 22, 2023 · 5 comments · May be fixed by #914
Open

Possible ArgumentOutOfRangeException when there are 65535 params in metadata table #913

UlyssesWu opened this issue Jun 22, 2023 · 5 comments · May be fixed by #914

Comments

@UlyssesWu
Copy link

UlyssesWu commented Jun 22, 2023 •
edited
Loading

cecil/Mono.Cecil/AssemblyReader.cs

Lines 979 to 1003 in ba9c6c7

Range ReadListRange (uint current_index, Table current, Table target)
{
var list = new Range ();
var start = ReadTableIndex (target);
if (start == 0)
return list;
uint next_index;
var current_table = image.TableHeap [current];
if (current_index == current_table.Length)
next_index = image.TableHeap [target].Length + 1;
else {
var position = this.position;
this.position += (int) (current_table.RowSize - image.GetTableIndexSize (target));
next_index = ReadTableIndex (target);
this.position = position;
}
list.Start = start;
list.Length = next_index - start;
return list;
}

(Sorry but I'm unable to provide a sample dll.)

We have a big dll which exactly has 65535 parameters in metadata table.
image

When processing it using cecil, it throw exception like this:

System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
    at Mono.Collections.Generic.Collection`1[T].get_Item (System.Int32 index)
    at Mono.Cecil.MetadataReader.ReadParameter (System.UInt32 param_rid, Mono.Cecil.MethodDefinition method)
    at Mono.Cecil.MetadataReader.ReadParameters (Mono.Cecil.MethodDefinition method, Mono.Cecil.Range param_range)
    at Mono.Cecil.MetadataReader.ReadMethod (System.UInt32 method_rid, Mono.Collections.Generic.Collection`1[T] methods)
    at Mono.Cecil.MetadataReader.ReadMethods (Mono.Cecil.TypeDefinition type)
    at Mono.Cecil.TypeDefinition+<>c.<get_Methods>b__43_0 (Mono.Cecil.TypeDefinition type, Mono.Cecil.MetadataReader reader)
    at Mono.Cecil.ModuleDefinition.Read[TItem,TRet] (TRet& variable, TItem item, System.Func`3[T1,T2,TResult] read)
    at Mono.Cecil.TypeDefinition.get_Methods ()

After some debugging I managed to locate the problem:

cecil/Mono.Cecil/AssemblyReader.cs

Line 995 in ba9c6c7

next_index = ReadTableIndex (target);

When it's trying to read the 65535th (the last) param (it is used in a compiler-generated anonymous method), and image.GetTableIndexSize returns 2 (entry < 65536), ReadTableIndex returns 0, next_index = 0.

cecil/Mono.Cecil/AssemblyReader.cs

Line 1000 in ba9c6c7

list.Length = next_index - start;

list.Length = next_index - start = 0u - 65535u = 4294901761 💥

image

On the other hand, dnlib already handled this problem, so it still works for our dll:

https://github.com/0xd4d/dnlib/blob/97e07a8f1ea0ccbf31231dad0f7cb093805b8eee/src/DotNet/MD/CompressedMetadata.cs#L143-L157

The magic happens here:
image
@jbevain
Copy link
Owner

jbevain commented Jun 23, 2023 •
edited
Loading

That's a good catch, thank you :)

@jbevain
Copy link
Owner

jbevain commented Jun 23, 2023 •
edited
Loading

The magic actually happens here in dnlib:

uint endRid = !hasNext || (nextListRid == 0 && tableSourceRid + 1 == tableSource.Rows && tableDest.Rows == 0xFFFF) ? lastRid : nextListRid;

@UlyssesWu
Copy link
Author 

uint endRid = !hasNext || (nextListRid == 0 && tableSourceRid + 1 == tableSource.Rows && tableDest.Rows == 0xFFFF) ? lastRid : nextListRid;

Thanks for pointing out. In my situation, endRid was set to 0 at this line. Then it was set to 65535 in L153, which I thought was the key to keep endRid - startRid correct in L156.

This was referenced Jun 23, 2023
Open
Open
@alexey-zakharov
Copy link

Hi! @jbevain we have a symmetric issue at Unity, but mono - Mono.Cecil generates an assembly with exactly 65535 fields and loading it in mono results in the crash due to very similar table row usage logic.

Are there any reasons that #914 was not merged yet and would be feasible to use int for field indices when patching assembly with Cecil? (should I make a new issue for this?)
Thanks!

@alexey-zakharov alexey-zakharov mentioned this issue Oct 24, 2024
Merged
6 tasks
@alexey-zakharov
Copy link

wrt my last comment - I checked what happens if we use large indices when table size is 65535 and it doesn't work as no reader can then read assembly properly. According to the standard large indices are only used for row count 2^16 and above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix issue with 0xffff parameters jbevain/cecil
3 participants
@jbevain @UlyssesWu @alexey-zakharov