From: Rodrigo Kumpera Date: Mon, 11 Sep 2017 22:53:14 +0000 (-0700) Subject: [verifier] Implement verification of byref returns. X-Git-Url: http://wien.tomnetworks.com/gitweb/?a=commitdiff_plain;ds=sidebyside;h=8493513ad3876e73163f64ecab0299bd5e6e0346;p=mono.git [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 fa38edffc03..7c9a48a2f15 100644 --- 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 index 00000000000..6350f1ec251 --- /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 7a2776c8514..788704a9cb9 100644 --- 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