[Mono-dev] [PATCH] Implement field load/store verification

[Paolo Molaro]

On 06/20/07 Rodrigo Kumpera wrote:
> The attached patch does the following:
> 
> -implement all field load/store verifications
> -fixes a small typo in a class.c comment
> -function pointer validation now handles call convention
> -fixed: brtrue/false flaged unmanaged pointers as invalid types (now just
> flags as unverifiable)

> Index: verify.c
> ===================================================================
> --- verify.c	(revision 80327)
> +++ verify.c	(working copy)
> @@ -2359,6 +2362,156 @@
>  	}
>  }
>  
> +static void
> +do_push_static_field (VerifyContext *ctx, int token, gboolean take_addr)
> +{
> +	MonoClassField *field;
> +	MonoClass *klass;
> +
> +	field = mono_field_from_token (ctx->image, token, &klass, ctx->generic_context);
> +	if (!field) {
> +		ADD_VERIFY_ERROR (ctx, g_strdup_printf ("Cannot load field from token 0x%08x at 0x%04x", token, ctx->ip_offset));
> +		return;
> +	}
> +
> +	if (!(field->type->attrs & FIELD_ATTRIBUTE_STATIC)) { 
> +		ADD_VERIFY_ERROR (ctx, g_strdup_printf ("Cannot load non static field at 0x%04x", ctx->ip_offset));
> +		return;
> +	}
> +
> +	if (take_addr && (field->type->attrs & FIELD_ATTRIBUTE_INIT_ONLY))
> +		CODE_NOT_VERIFIABLE (ctx, g_strdup_printf ("Cannot take the address of a init-only field at 0x%04x", ctx->ip_offset));

This should be allowed in the .cctor for the class (same for the
equivalent case of an instance field and the .ctor): a test case is
something like:

	readonly static MyValueType v = new MyValueType (5);

Note, eventually you also need to add accessibility checks: the method
needs to be able to access the field keeping in mind both the field's
and the field's class visibility.

> +static gboolean
> +check_is_valid_type_for_field_ops (VerifyContext *ctx, int token, ILStackDesc *obj, MonoClassField **ret_field)
> +{
> +	MonoClassField *field;
> +	MonoClass *klass;
> +
> +	/*must be one of: object type, managed pointer, unmanaged pointer (native int) or an instance of a value type */
> +	if (!((obj->stype == TYPE_COMPLEX)
> +		/* the managed reference must be to an object or value type */
> +		|| ((obj->stype & POINTER_MASK) && (UNMASK_TYPE (obj->stype) == TYPE_COMPLEX))
> +		|| (obj->stype == TYPE_NATIVE_INT)
> +		|| (obj->stype == TYPE_PTR)
> +		|| (obj->stype == TYPE_COMPLEX))) {
> +		ADD_VERIFY_ERROR (ctx, g_strdup_printf ("Invalid argument %s to load field at 0x%04x", type_names [UNMASK_TYPE (obj->stype)], ctx->ip_offset));
> +		return FALSE;
> +	}
> +
> +	field = mono_field_from_token (ctx->image, token, &klass, ctx->generic_context);
> +	if (!field) {
> +		ADD_VERIFY_ERROR (ctx, g_strdup_printf ("Cannot load field from token 0x%08x at 0x%04x", token, ctx->ip_offset));
> +		return FALSE;
> +	}
> +
> +	*ret_field = field;
> +
> +	g_assert (obj->type);
> +
> +	/*The value on the stack must be a subclass of the defining type of the field*/ 
> +	/* we need to check if we can load the field from the stack value*/
> +	if (UNMASK_TYPE (obj->stype) == TYPE_COMPLEX) {
> +		MonoType *type = obj->type->byref ? &field->parent->this_arg : &field->parent->byval_arg;
> +
> +		if (!verify_stack_type_compatibility (ctx, type, obj->type, FALSE)) {
> +			ADD_VERIFY_ERROR (ctx, g_strdup_printf ("Type at stack is not compatible to reference the field at 0x%04x", ctx->ip_offset));
> +			return FALSE;
> +		}
> +	}
> +
> +	if (MONO_TYPE_IS_REFERENCE (field->type) && !(field->type->attrs & FIELD_ATTRIBUTE_STATIC)) {
> +		MonoClass *p;
> +		MonoClassField * other;
> +
> +		for (p = field->parent; p != NULL; p = p->parent) {
> +			gpointer iter = NULL;
> +			while ((other = mono_class_get_fields (p, &iter))) {
> +				if (other != field && other->offset == field->offset && !(other->type->attrs & FIELD_ATTRIBUTE_STATIC)) {
> +					CODE_NOT_VERIFIABLE (ctx, g_strdup_printf ("Overlapped reference field at 0x%04x", ctx->ip_offset));
> +					goto overlap_check_end;
> +				}
> +			}
> +		}
> +	}

The code for overlapped fields is not as trivial as this. It is moreover
something you should do on a per-type basis instead of per-field access
in the IL code. metadata/object.c has the code to do proper overlapping
field detection: that code should not be duplicated.

Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better