[verifier] Implement verification of byref returns.

The verification algorithm is inspired on internal specs by the Roslyn team but adapted to what it generates today.
Here's how this patch works:

We introduce a new verification type, SAFE_BYREF, that's used for a byref value that is safe to return from the stack.

It's unverifiable to return a byref value that is not marked SAFE_BYREF.

We mark the following values are SAFE_BYREF:
	- byref arguments, unless it's the `this` arg of a valuetype instance method
	- return values of method calls when all byref arguments have SAFE_BYREF values - the this arg is ignored.
	- the address of a static field (LDSFLDA)
	- the address of an array element (LDELEMA)
	- the address of a field if the receiver is either a SAFE_BYREF value or a reference

Notoriously from the above is the value of local variables. We took a conservative approach of unsafe tainting.
We do this by associating 3 states to a local: unassigned (initial value of zero), assigned and safe (SAFE_BYREF_LOCAL), assigned and unsafe (UNSAFE_BYREF_LOCAL)
When loading a local, we set its value as SAFE_BYREF if the local is in the SAFE_BYREF_LOCAL state.

When storing to a local, we set it to SAFE_BYREF_LOCAL if, only if, it's not marked as UNSAFE_BYREF_LOCAL and the value is SAFE_BYREF.
If we store a non-safe value to a local, we mark it as UNSAFE_BYREF_LOCAL.
If we store a non-safe value to a SAFE_BYREF_LOCAL local, we mark the method as unverifiable.

This algorithm makes sure we don't mix safe and unsafe byref and side with safety in case of control-flow dependent values.

diff --git a/mono/metadata/verify.c b/mono/metadata/verify.c
index fa38edffc031b768aa6ef39b683e33ace1408eae..7c9a48a2f1566a91745e2c049606496fab65cde8 100644 (file)
--- a/mono/metadata/verify.c
+++ b/mono/metadata/verify.c
@@ -190,6 +190,7 @@ typedef struct {
 
        int num_locals;
        MonoType **locals;
+       char *locals_verification_state;
 
        /*TODO get rid of target here, need_merge in mono_method_verify and hoist the merging code in the branching code*/
        int target;
@@ -283,6 +284,9 @@ enum {
 
        /*This is an unitialized this ref*/
        UNINIT_THIS_MASK = 0x2000,
+
+       /* This is a safe to return byref */
+       SAFE_BYREF_MASK = 0x4000,
 };
 
 static const char* const
@@ -1109,12 +1113,38 @@ stack_slot_is_boxed_value (ILStackDesc *value)
        return (value->stype & BOXED_MASK) == BOXED_MASK;
 }
 
+/* stack_slot_is_safe_byref:
+ *
+ * Returns TRUE is @value is a safe byref
+ */
+static gboolean
+stack_slot_is_safe_byref (ILStackDesc *value)
+{
+       return (value->stype & SAFE_BYREF_MASK) == SAFE_BYREF_MASK;
+}
+
 static const char *
 stack_slot_get_name (ILStackDesc *value)
 {
        return type_names [value->stype & TYPE_MASK];
 }
 
+enum {
+       SAFE_BYREF_LOCAL = 1,
+       UNSAFE_BYREF_LOCAL = 2
+};
+static gboolean
+local_is_safe_byref (VerifyContext *ctx, unsigned int arg)
+{
+       return ctx->locals_verification_state [arg] == SAFE_BYREF_LOCAL;
+}
+
+static gboolean
+local_is_unsafe_byref (VerifyContext *ctx, unsigned int arg)
+{
+       return ctx->locals_verification_state [arg] == UNSAFE_BYREF_LOCAL;
+}
+
 #define APPEND_WITH_PREDICATE(PRED,NAME) do {\
        if (PRED (value)) { \
                if (!first) \
@@ -1137,6 +1167,7 @@ stack_slot_stack_type_full_name (ILStackDesc *value)
                APPEND_WITH_PREDICATE (stack_slot_is_null_literal, "null");
                APPEND_WITH_PREDICATE (stack_slot_is_managed_mutability_pointer, "cmmp");
                APPEND_WITH_PREDICATE (stack_slot_is_managed_pointer, "mp");
+               APPEND_WITH_PREDICATE (stack_slot_is_safe_byref, "safe-byref");
                has_pred = TRUE;
        }
 
@@ -1805,6 +1836,9 @@ dump_stack_value (ILStackDesc *value)
        if (stack_slot_is_managed_pointer (value))
                printf ("Managed Pointer to: ");
 
+       if (stack_slot_is_safe_byref (value))
+               printf ("Safe ByRef to: ");
+
        switch (stack_slot_get_underlying_type (value)) {
                case TYPE_INV:
                        printf ("invalid type]"); 
@@ -2788,6 +2822,18 @@ verify_delegate_compatibility (VerifyContext *ctx, MonoClass *delegate, ILStackD
 #undef IS_LOAD_FUN_PTR
 }
 
+static gboolean
+is_this_arg_of_struct_instance_method (unsigned int arg, VerifyContext *ctx)
+{
+       if (arg != 0)
+               return FALSE;
+       if (ctx->method->flags & METHOD_ATTRIBUTE_STATIC)
+               return FALSE;
+       if (!ctx->method->klass->valuetype)
+               return FALSE;
+       return TRUE;
+}
+
 /* implement the opcode checks*/
 static void
 push_arg (VerifyContext *ctx, unsigned int arg, int take_addr) 
@@ -2819,6 +2865,8 @@ push_arg (VerifyContext *ctx, unsigned int arg, int take_addr)
                        if (mono_method_is_constructor (ctx->method) && !ctx->super_ctor_called && !ctx->method->klass->valuetype)
                                top->stype |= UNINIT_THIS_MASK;
                }
+               if (!take_addr && ctx->params [arg]->byref && !is_this_arg_of_struct_instance_method (arg, ctx))
+                       top->stype |= SAFE_BYREF_MASK;
        } 
 }
 
@@ -2833,8 +2881,11 @@ push_local (VerifyContext *ctx, guint32 arg, int take_addr)
                if (ctx->locals [arg]->byref && take_addr)
                        CODE_NOT_VERIFIABLE (ctx, g_strdup_printf ("ByRef of ByRef at 0x%04x", ctx->ip_offset));
 
-               set_stack_value (ctx, stack_push (ctx), ctx->locals [arg], take_addr);
-       } 
+               ILStackDesc *value = stack_push (ctx);
+               set_stack_value (ctx, value, ctx->locals [arg], take_addr);
+               if (local_is_safe_byref (ctx, arg))
+                       value->stype |= SAFE_BYREF_MASK;
+       }
 }
 
 static void
@@ -2873,9 +2924,20 @@ store_local (VerifyContext *ctx, guint32 arg)
                return;
 
        value = stack_pop (ctx);
-       if (ctx->locals [arg]->byref && stack_slot_is_managed_mutability_pointer (value))
-               CODE_NOT_VERIFIABLE (ctx, g_strdup_printf ("Cannot use a readonly managed reference when storing on a local variable at 0x%04x", ctx->ip_offset));
-                       
+       if (ctx->locals [arg]->byref) {
+               if (stack_slot_is_managed_mutability_pointer (value))
+                       CODE_NOT_VERIFIABLE (ctx, g_strdup_printf ("Cannot use a readonly managed reference when storing on a local variable at 0x%04x", ctx->ip_offset));
+
+               if (local_is_safe_byref (ctx, arg) && !stack_slot_is_safe_byref (value))
+                       CODE_NOT_VERIFIABLE (ctx, g_strdup_printf ("Cannot store an unsafe ret byref to a local that was previously stored a save ret byref value at 0x%04x", ctx->ip_offset));
+
+               if (stack_slot_is_safe_byref (value) && !local_is_unsafe_byref (ctx, arg))
+                       ctx->locals_verification_state [arg] |= SAFE_BYREF_LOCAL;
+
+               if (!stack_slot_is_safe_byref (value))
+                       ctx->locals_verification_state [arg] |= UNSAFE_BYREF_LOCAL;
+
+       }
        if (!verify_stack_type_compatibility (ctx, ctx->locals [arg], value)) {
                char *expected = mono_type_full_name (ctx->locals [arg]);
                char *found = stack_slot_full_name (value);
@@ -3127,7 +3189,10 @@ do_ret (VerifyContext *ctx)
                        return;
                }
 
-               if (ret->byref || ret->type == MONO_TYPE_TYPEDBYREF || mono_type_is_value_type (ret, "System", "ArgIterator") || mono_type_is_value_type (ret, "System", "RuntimeArgumentHandle"))
+               if (ret->byref && !stack_slot_is_safe_byref (top))
+                       CODE_NOT_VERIFIABLE (ctx, g_strdup_printf ("Method returns byref and return value is not a safe-to-return-byref at 0x%04x", ctx->ip_offset));
+
+               if (ret->type == MONO_TYPE_TYPEDBYREF || mono_type_is_value_type (ret, "System", "ArgIterator") || mono_type_is_value_type (ret, "System", "RuntimeArgumentHandle"))
                        CODE_NOT_VERIFIABLE (ctx, g_strdup_printf ("Method returns byref, TypedReference, ArgIterator or RuntimeArgumentHandle at 0x%04x", ctx->ip_offset));
        }
 
@@ -3193,6 +3258,8 @@ do_invoke_method (VerifyContext *ctx, int method_token, gboolean virtual_)
        if (!check_underflow (ctx, param_count))
                return;
 
+       gboolean is_safe_byref_call = TRUE;
+
        for (i = sig->param_count - 1; i >= 0; --i) {
                VERIFIER_DEBUG ( printf ("verifying argument %d\n", i); );
                value = stack_pop (ctx);
@@ -3211,6 +3278,8 @@ do_invoke_method (VerifyContext *ctx, int method_token, gboolean virtual_)
                        ADD_VERIFY_ERROR (ctx, g_strdup_printf ("Cannot  pass a byref argument to a tail %s at 0x%04x", virtual_ ? "callvirt" : "call",  ctx->ip_offset));
                        return;
                }
+               if (stack_slot_is_managed_pointer (value) && !stack_slot_is_safe_byref (value))
+                       is_safe_byref_call = FALSE;
        }
 
        if (sig->hasthis) {
@@ -3296,6 +3365,8 @@ do_invoke_method (VerifyContext *ctx, int method_token, gboolean virtual_)
                                ctx->prefix_set &= ~PREFIX_READONLY;
                                value->stype |= CMMP_MASK;
                        }
+                       if (sig->ret->byref && is_safe_byref_call)
+                               value->stype |= SAFE_BYREF_MASK;
                }
        }
 
@@ -3333,7 +3404,10 @@ do_push_static_field (VerifyContext *ctx, int token, gboolean take_addr)
        if (!IS_SKIP_VISIBILITY (ctx) && !mono_method_can_access_field_full (ctx->method, field, NULL))
                CODE_NOT_VERIFIABLE2 (ctx, g_strdup_printf ("Type at stack is not accessible at 0x%04x", ctx->ip_offset), MONO_EXCEPTION_FIELD_ACCESS);
 
-       set_stack_value (ctx, stack_push (ctx), field->type, take_addr);
+       ILStackDesc *value = stack_push (ctx);
+       set_stack_value (ctx, value, field->type, take_addr);
+       if (take_addr)
+               value->stype |= SAFE_BYREF_MASK;
 }
 
 static void
@@ -3432,6 +3506,7 @@ do_push_field (VerifyContext *ctx, int token, gboolean take_addr)
 {
        ILStackDesc *obj;
        MonoClassField *field;
+       gboolean is_safe_byref = FALSE;
 
        if (!take_addr)
                CLEAR_PREFIX (ctx, PREFIX_UNALIGNED | PREFIX_VOLATILE);
@@ -3450,7 +3525,14 @@ do_push_field (VerifyContext *ctx, int token, gboolean take_addr)
                !(field->parent == ctx->method->klass && mono_method_is_constructor (ctx->method)))
                CODE_NOT_VERIFIABLE (ctx, g_strdup_printf ("Cannot take the address of a init-only field at 0x%04x", ctx->ip_offset));
 
-       set_stack_value (ctx, stack_push (ctx), field->type, take_addr);
+       //must do it here cuz stack_push will return the same slot as obj above
+       is_safe_byref = take_addr && (stack_slot_is_reference_value (obj) || stack_slot_is_safe_byref (obj));
+
+       ILStackDesc *value = stack_push (ctx);
+       set_stack_value (ctx, value, field->type, take_addr);
+
+       if (is_safe_byref)
+               value->stype |= SAFE_BYREF_MASK;
 }
 
 static void
@@ -4116,6 +4198,8 @@ do_ldelema (VerifyContext *ctx, int klass_token)
                ctx->prefix_set &= ~PREFIX_READONLY;
                res->stype |= CMMP_MASK;
        }
+
+       res->stype |= SAFE_BYREF_MASK;
 }
 
 /*
@@ -4891,6 +4975,7 @@ mono_method_verify (MonoMethod *method, int level)
        ctx.num_locals = ctx.header->num_locals;
        ctx.locals = (MonoType **)g_memdup (ctx.header->locals, sizeof (MonoType*) * ctx.header->num_locals);
        _MEM_ALLOC (sizeof (MonoType*) * ctx.header->num_locals);
+       ctx.locals_verification_state = g_new0 (char, ctx.num_locals);
 
        if (ctx.num_locals > 0 && !ctx.header->init_locals)
                CODE_NOT_VERIFIABLE (&ctx, g_strdup_printf ("Method with locals variable but without init locals set"));
@@ -6008,6 +6093,7 @@ cleanup:
        if (ctx.code)
                g_free (ctx.code);
        g_free (ctx.locals);
+       g_free (ctx.locals_verification_state);
        g_free (ctx.params);
        mono_basic_block_free (original_bb);
        mono_metadata_free_mh (ctx.header);

diff --git a/mono/tests/verifier/valid_ref_return.cs b/mono/tests/verifier/valid_ref_return.cs
new file mode 100644 (file)
index 0000000..6350f1e
--- /dev/null
+++ b/mono/tests/verifier/valid_ref_return.cs
@@ -0,0 +1,55 @@
+using System;
+
+class Foo {
+       static int X = 10;
+       static int[] Arr = new int[1];
+       int y;
+
+       static void Main () {
+       }
+
+       static ref int ReturnStatic () {
+               return ref X;
+       }
+
+       ref int ReturnField () {
+               return ref this.y;
+       }
+
+       ref int ReturnArrayElement () {
+               return ref Arr [0];
+       }
+
+       ref int ReturnArg (ref int arg) {
+               return ref arg;
+       }
+
+       ref int TwoReturns (bool b) {
+               if (b) 
+                       return ref X;
+               else
+                       return ref Arr [0];
+       }
+
+       ref int LocalVarRet (bool b) {
+               ref int x = ref X;
+               ReturnArg (ref x);
+               return ref x;
+       }
+
+       ref int ReturnRet (ref int arg) {
+               return ref ReturnArg (ref arg);
+       }
+
+       ref int ReturnFromCatch () {
+               try {
+                       return ref X;
+               } catch (Exception) {
+                       return ref X;
+               } 
+       }
+
+       ref int ReturnFunc () {
+               return ref ReturnStatic ();
+       }
+}

diff --git a/tools/pedump/Makefile.am b/tools/pedump/Makefile.am
index 7a2776c851421bcac377310c5bf2ac9627e7335c..788704a9cb9498fe3da408bed3daa0fb7d355f47 100644 (file)
--- a/tools/pedump/Makefile.am
+++ b/tools/pedump/Makefile.am
@@ -28,3 +28,8 @@ pedump_LDADD =                        \
 if HOST_DARWIN
 pedump_LDFLAGS=-framework CoreFoundation -framework Foundation
 endif
+
+#Helper target to rebuild metadata as well, it's useful when working on the verifier as its source still on metadata
+md:
+       make -C ../../mono/metadata all
+       make -C . all
\ No newline at end of file