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

Reflection API Incorrectly Boxes (U)IntPtr Constants as (U)Int32 #104270

Open
LEI-Hongfaan opened this issue Jul 2, 2024 · 17 comments
Open

Reflection API Incorrectly Boxes (U)IntPtr Constants as (U)Int32 #104270

LEI-Hongfaan opened this issue Jul 2, 2024 · 17 comments

Comments

@LEI-Hongfaan
Copy link

Description

The reflection API is expected to box constant literal fields of type (U)IntPtr as their correct types. However, when using reflection to retrieve the value of these constants, they are being boxed as (U)Int32 instead of (U)IntPtr.

Reproduction Steps

public enum E {

    V = 42,
}

internal class Program {

    public const UIntPtr A = 3;

    public const IntPtr B1 = -5;

    public const E C = E.V;

    public const long D = 1;

    static void Main(string[] args) {
        DumpConstLiteralByReflection("UIntPtr", nameof(A));
        DumpConstLiteralByReflection("IntPtr", nameof(B1));
        DumpConstLiteralByReflection("enum E", nameof(C));
        DumpConstLiteralByReflection("long", nameof(D));

        static void DumpConstLiteralByReflection(string label, string fieldName) {
            Console.WriteLine($@"{label}:");
            Console.WriteLine($@"  Field type: {typeof(Program).GetField(fieldName)!.FieldType}");
            var o = typeof(Program).GetField(fieldName)!.GetValue(null)!;
            Console.WriteLine($@"  Value type: {o.GetType()}");
            Console.WriteLine($@"  Value: {o:D}");
            Console.WriteLine();
        }
    }
}

Expected behavior

UIntPtr:
  Field type: System.UIntPtr
  Value type: System.UIntPtr
  Value: 3

IntPtr:
  Field type: System.IntPtr
  Value type: System.IntPtr
  Value: -5

enum E:
  Field type: E
  Value type: E
  Value: 42

long:
  Field type: System.Int64
  Value type: System.Int64
  Value: 1

Actual behavior

UIntPtr:
  Field type: System.UIntPtr
  Value type: System.UInt32
  Value: 3

IntPtr:
  Field type: System.IntPtr
  Value type: System.Int32
  Value: -5

enum E:
  Field type: E
  Value type: E
  Value: 42

long:
  Field type: System.Int64
  Value type: System.Int64
  Value: 1

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

This issue may present a theoretical security risk, although the practical threat is likely negligible.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 2, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

@colejohnson66
Copy link

Const n[u]int are limited to 32 bits in size, so a boxed constant n[u]int must be 32-bit, even on a 64-bit platform. The way to do such a thing is to actually box a [u]int.

@LEI-Hongfaan
Copy link
Author

Const n[u]int are limited to 32 bits in size, so a boxed constant n[u]int must be 32-bit, even on a 64-bit platform. The way to do such a thing is to actually box a [u]int.

I also fail to see how this could be a security risk as it only affects constants.

The runtime is capable of loading an assembly containing a field defined as .field public static literal native uint B2 = int32(0Xffffffff). Moreover, the current behavior is counterintuitive and not what would be generally expected.

@LEI-Hongfaan
Copy link
Author

I believe that the field initialization clause should merely serve as an expression of a literal value, rather than determining the field's type. To illustrate this with a thought experiment: even if the field initialization clause provided a string representing a C++ constant expression, and a tool calculated the field's value based on this C++ source code, I wouldn't find it strange.

@huoyaoyuan
Copy link
Member

Note: according to ECMA-335 §II.16.2 Field init metadata:

Note that while both the type and the field initializer are stored in metadata there is no requirement that they match. (Any importing compiler is responsible for coercing the stored value to the target field type).

The original purpose was to allow conversion from underlying numeric types to enum types. However, it doesn't specify the desired behavior of other casts. It's not clear what's the expected behavior here.

@huoyaoyuan
Copy link
Member

The runtime is capable of loading an assembly containing a field defined as .field public static literal native uint B2 = int32(0Xffffffff).

Also note that currently there's no compiler emitting this, and probably not understanding it.
For example, trying to use the constant in reference assembly.

@steveharter
Copy link
Member

Verified repo; for the first field, the IL is:

.field public static literal native uint A = uint32(0x00000003)

so for constants yes it does change to a int\uint.

Also, for .NET Framework, this code fails compile with CS0283 The type 'IntPtr' cannot be declared const where that link does not call out the native pointer types.

This is not a reflection issue; changing area to runtime, although this is likely an ECMA issue and where we supported this to compile, unlike .NET Framework.

cc @jkotas @AaronRobinsonMSFT

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

@AaronRobinsonMSFT
Copy link
Member

@steveharter Thanks for the investigation. I've also confirmed your findings using a variety of tools. The Constants table (0x0B) does encode this as uint32 so what we are returning is technically correct based on the data we have. We also have the actual field type (that is native uint) and can special case this behavior.

I personally don't like inconsistencies in this domain, makes for too much confusion. The fact that .NET Framework doesn't support this scenario lowers the concern about making it do what the OP expects.

The code to update is below. We could make a check to see if the field is native int / native uint and then determine if the CorElementType disagrees. Many options here. @jkotas or @MichalStrehovsky Any opinions?

return corElementType switch
{
CorElementType.ELEMENT_TYPE_VOID => DBNull.Value,
CorElementType.ELEMENT_TYPE_CHAR => *(char*)&buffer,
CorElementType.ELEMENT_TYPE_I1 => *(sbyte*)&buffer,
CorElementType.ELEMENT_TYPE_U1 => *(byte*)&buffer,
CorElementType.ELEMENT_TYPE_I2 => *(short*)&buffer,
CorElementType.ELEMENT_TYPE_U2 => *(ushort*)&buffer,
CorElementType.ELEMENT_TYPE_I4 => *(int*)&buffer,
CorElementType.ELEMENT_TYPE_U4 => *(uint*)&buffer,
CorElementType.ELEMENT_TYPE_I8 => buffer,
CorElementType.ELEMENT_TYPE_U8 => (ulong)buffer,
CorElementType.ELEMENT_TYPE_BOOLEAN => (*(int*)&buffer != 0),
CorElementType.ELEMENT_TYPE_R4 => *(float*)&buffer,
CorElementType.ELEMENT_TYPE_R8 => *(double*)&buffer,
CorElementType.ELEMENT_TYPE_STRING => stringVal ?? string.Empty,
CorElementType.ELEMENT_TYPE_CLASS => null,
_ => throw new FormatException(SR.Arg_BadLiteralFormat),
};

@davidwrighton
Copy link
Member

I don't think we should change anything in the runtime here.

I took a slightly deeper look, and if you change the repro code to use nint and nuint for the const field types, and compile with a recent C# language version, you can compile and run the test case on the .NET Framework. So we have a legacy support burden that may be in place already. My advice is to write something into the reflection documentation about this weird case.

@jkotas
Copy link
Member

jkotas commented Jul 4, 2024

Handling of constant and default values in reflection have seen number of fixes over the years in .NET Core. Some of the earlier fixes include:

We have systemic problem where Roslyn encodes the constants and default values as it sees a fit, but reflection is not aware of all possible encodings produced by Roslyn and it may produce unexpected results for some of them.

We took number of fixes in this area and the corner case behavior deviated from .NET Framework, so I do not see a problem with fixing more corner cases. However, until somebody sits down and ensures that the reflection behavior is sensible for everything that's possible to write in C#, we are unlikely to be in 100% happy place.

Note that there is a fresh from the press proposal to allow encoding even more types as constants in C#: dotnet/csharplang#8257 . If this proposal is accepted, we will have a work to do here anyway.

This is not a reflection issue

Nit: I see this as Roslyn and reflection interaction issue. Nothing in the runtime outside of reflection cares about encoding of C# constants.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

@huoyaoyuan
Copy link
Member

Do note the signed-ness mismatch in original post: the literal field is declared as native uint, but the initializer is declared as int32.

Recent roslyn and C# versions allow n(u)int constant being initialized with int32 of the same size:

const nint a = -1;
// compiles into
.field private static literal native int a = int32(-1)

In this case, both Field.GetValue(null) and Field.GetRawConstantValue return int32. I think the former should return nint instead. This also matches the behavior for enums.

@huoyaoyuan
Copy link
Member

However, initializing nuint with negative int32 is a different case. (nuint)-1 isn't really a constant value. For example, unchecked((nuint)(-1)) % 7 is 1 on 64bit, but 3 on 32bit.

I'm also not sure whether roslyn can understand .field private static literal native uint a = int32(-1), and how it handles constant propagation.

@steveharter steveharter removed the untriaged New issue has not been triaged by the area owner label Jul 8, 2024
@steveharter steveharter added this to the 9.0.0 milestone Jul 8, 2024
@steveharter
Copy link
Member

Do note the signed-ness mismatch in original post: the literal field is declared as native uint, but the initializer is declared as int32.

@huoyaoyuan I don't see this locally (compiler version: '4.11.0-2.24304.1`) - can you verify please?

C#
public const nuint A = 3; // or public const UIntPtr A = 3

IL
.field public static literal native uint A = uint32(0x00000003)

@huoyaoyuan
Copy link
Member

huoyaoyuan commented Jul 9, 2024

@huoyaoyuan I don't see this locally (compiler version: '4.11.0-2.24304.1`) - can you verify please?

The original post is about the case that never emitted by current compiler, but allowed in runtime.

Personally I don't think the runtime should allow this case.


However, getting the value of const nuint gets an uint which is the underlying constant type. This differs from the behavior of enum.

@steveharter
Copy link
Member

The original post is about the case that never emitted by current compiler, but allowed in runtime.

The original post is about what is emitted by the compiler, unless I'm missing something. For example, this:
const nint a = (nint)1;
is the same as
const nint a = 1;
when looking at the generated IL - both use Int32 for the value. The compiler never emits nint for the right-hand side \ value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants