MetadataReader overuses uint

[nguerrera]

This was sent to me from @tmat, quote:

We should so something about all the int <-> uint casts in the reader – some of them are actual bugs or questionable behaviors, since we shouldn’t be throwing overflow exceptions.

(found the following by looking for “internal uint” in Tables.cs)

TypeDefinition.GetLayout() 
{ 
   … 
   // here we overflow:
   int size = (int)reader.ClassLayoutTable.GetClassSize(classLayoutRowId);
   … 
}

/// Returns field layout offset, or -1 if not available.      
FieldDefinition.GetOffset()
{

    uint layoutRowId = reader.FieldLayoutTable.FindFieldLayoutRowId(Handle);
    if (layoutRowId == 0)
    {
        return -1;
    }

    uint offset = reader.FieldLayoutTable.GetOffset(layoutRowId);

    // we reinterpret too big (but strictly speaking not invalid) values:
    if (offset > int.MaxValue)
    {
        // CLI spec says:

        // "Offset (a 4-byte constant)"
        // "Offset shall be zero or more"
        // 
        // Peverify fails with "Type load failed" error if offset is greater than Int32.MaxValue.
        return -1;
    }

    return (int)offset;
}

int ExportedTypeExtensions.GetTypeDefinitionId
{
   // here we overflow:
   return (int)exportedType.reader.ExportedTypeTable.GetTypeDefId(exportedType.rowId);
}

internal void GetMethodRange(
    TypeDefinitionHandle typeDef,
    out int firstMethodRowId, 
    out int lastMethodRowId)
{
   uint typeDefRowId = typeDef.RowId;

   // here we overflow:
   firstMethodRowId = (int)this.TypeDefTable.GetMethodStart(typeDefRowId);
   …
}

We should probably check the range and throw BadImageFormat when passing the value out thru public API to be consistent – we’re already doing that for handles in MemoryBlock.PeekReference:

if (!TokenTypeIds.IsValidRowId(value)) ThrowReferenceOverflow();

Internally “rowId” fields of handles should probably be ints, not uints since we assert: Debug.Assert(TokenTypeIds.IsValidRowId(rowId))

“token” fields of heap handles should stay uints since we use all the bits.

Once we change the rowId to int and apply the type transitively I think we should be able to get rid of most uints.

He also shared the following implementation suggestion:

Currently we have

internal Int32 PeekInt32(int offset)
{
    CheckBounds(offset, sizeof(Int32));
    return *(Int32*)(Pointer + offset);
}

internal UInt32 PeekUInt32(int offset)
{
     CheckBounds(offset, sizeof(UInt32));
     return *(UInt32*)(Pointer + offset);}
}

on MemoryBlock. What about

internal uint PeekUnsignedInt32(int offset)
{
    CheckBounds(offset, sizeof(uint));
    return *(uint*)(Pointer + offset);
}

internal int PeekSignedInt32(int offset)
{
    CheckBounds(offset, sizeof(int));
    return *(int*)(Pointer + offset);
}

internal int PeekInt32(int offset)
{
    CheckBounds(offset, sizeof(int));
    var result = *(int*)(Pointer + offset);
    if (result < 0) Throw();
    return result;
}

[talebi]

not sure if its related but when I add a reference to Autodesk Revit 2013/2015 to a project I get a compile error in (VS 2015 CTP):
Metadata file 'E:\dev\cadtools_revit\Source\Lib\RevitAPI\RevitAPI_x64_2013.dll' could not be opened -- Row ID or heap offset is too large.

[ellismg]

@nguerrera The VS folks think this is a problem in the metadata reader. Who's a good person to take a look at this?

[nguerrera]

@ellismg @talebi. I can take a look even though I'll be out of the office.

I've reopened dotnet/corefx#1238 as this tracks a separate issue. The RowId/Offset bounds checking is believed to be entirely correct and this tracks some engineering work (only slightly related) to have less use of uint in the codebase, but I see no reason why this would fix the issue.

[nguerrera]

Fixed by dotnet/corefx#1557