Skip to content
This repository has been archived by the owner on Nov 1, 2020. It is now read-only.

Expect ILVerify error on initonly field outside its constructor #4911

Closed
jcouv opened this issue Nov 10, 2017 · 3 comments · Fixed by #4914
Closed

Expect ILVerify error on initonly field outside its constructor #4911

jcouv opened this issue Nov 10, 2017 · 3 comments · Fixed by #4914

Comments

@jcouv
Copy link
Member

jcouv commented Nov 10, 2017

Found this while testing ILVerify on Roslyn tests.

Compile the program below with C# compiler that ships with Visual Studio 15.5 with csc /t:library /langversion:7.2 test.cs.
Here's also a zipped copy of test.dll: test.zip

PEVerify fails (as expected):

>peverify test.dll
Microsoft (R) .NET Framework PE Verifier.  Version  4.0.30319.0
[IL]: Error: [F:\repos\corert\src\ILVerify\src\bin\Debug\net46\test.dll : Program::Main][offset 0x00000008] Cannot change initonly field outside its .ctor.
1 Error(s) Verifying test.dll

But ILVerify passes:

bin\Debug\net46>ilverify test.dll -r C:\WINDOWS\Microsoft.Net\assembly\GAC_32\mscorlib\v4.0_4.0.0.0__b77a5c561934e089\mscorlib.dll
All Classes and Methods in F:\repos\corert\src\ILVerify\src\bin\Debug\net46\test.dll Verified.

class Program
{
    static void Main()
    {
        var obj = new C1();
        System.Console.WriteLine(obj.field.ToString());
    }
}
class C1
{
    public readonly S1 field;
}
struct S1
{
}

Tagging @ArztSamuel @VSadov

@VSadov
Copy link
Member

VSadov commented Nov 10, 2017

AFAIK, the spec says that LDFLDA of a readonly field is unverifiable.

The actual implementation however permits LDFLDA when it is done in a corresponding constructor. Perhaps there is a special rule that permits that. I am not sure which one. Anyways, I believe both C# and PEVerify allow LDFLDA of a readonly field when done in a corresponding constructor.

Anyways, this is not the case with constructor and per current rules it should not be verifiable.

Once the tool implements proposed extensions from dotnet/designs#21 the code will become verifiable, but for now a failure is expected here.

@jcouv
Copy link
Member Author

jcouv commented Nov 10, 2017

Note: after the spec changes, this probably should verify (Vlad can confirm). So the expectation reflects current PEVerify baseline.

@ArztSamuel
Copy link
Collaborator

ArztSamuel commented Nov 11, 2017

AFAIK, the spec says that LDFLDA of a readonly field is unverifiable.

That is correct. I think the rule you are referring to is described in ECMA II.16.1.2 Field contract attributes. However, I could not find any special rule permitting ldflda in constructors, a note of this section even states the opposite:

The use of ldflda or ldsflda on an initonly field makes code unverifiable.

As far as I understand the PEVerify source, it simply permits all field instructions on initonly fields (including ldflda) in the corresponding constructor. I am not sure why.
(here this exact rule is checked: https://github.com/lewischeng-ms/sscli/blob/master/clr/src/jit64/newverify.cpp#L3699)

I think ILVerify does not check the initonly rule at all yet, but I will look into that right now. Thanks for reporting this!

Edit: A few lines lower in the PEVerify source there is a comment saying:

// Presently the JIT doe not check that we dont store or take the address of init-only fields
// since we can not guarentee their immutability and it is not a security issue

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

Successfully merging a pull request may close this issue.

3 participants