[Mono-dev] [PATCH] Method invoke verification

[Paolo Molaro]

On 06/11/07 Rodrigo Kumpera wrote:
> Index: verify.c
> ===================================================================
> --- verify.c	(revision 79204)
> +++ verify.c	(working copy)
> +/* FIXME: we could just load the signature instead of the whole MonoMethod
> + * TODO handle vararg calls
> + * TODO handle non virt calls to non-final virtual calls (exception clause in page 52 of partition 3)
> + */
> +static void
> +do_invoke_method (VerifyContext *ctx, int method_token)
> +{
> +	int param_count, i;
> +	MonoMethodSignature *sig;
> +	ILStackDesc *value;
> +	MonoMethod *method = mono_get_method_full (ctx->image, method_token, NULL, ctx->generic_context);
> +
> +	if (!method) {
> +		ADD_VERIFY_ERROR (ctx, g_strdup_printf ("Method 0x%08x not found at 0x%04x", method_token, ctx->ip_offset));
> +		return;
> +	}
> +
> +	if (!(sig = mono_method_signature (method)))
> +		sig = mono_method_get_signature (method, ctx->image, method_token);
> +
> +	param_count = sig->param_count + sig->hasthis;
> +	check_underflow (ctx, param_count);
> +
> +	for (i = sig->param_count - 1; i >= 0; --i) {
> +		VERIFIER_DEBUG ( printf ("verifying argument %d\n", i); );
> +		value = stack_pop (ctx);

If check_underflow failed, stack_pop() will access random memory: there
are a few cases where after some errors, you can't continue checking for
more, as it would involve either bugs as above or adding many more
checks that slow down everything. Here we see a mismatch between the
needs of a verifier for the JIT's use and an offline verifier: for the
JIT it needs to bail out as soon as possible, for an offline verifier
you may want to try to continue and provide to the user more useful
feedback (sort of like compilers will try to check for more errors after
the first). Please check all the uses of check_underflow() and similar
for this issue.

> +		if (!verify_type_compat (ctx, sig->params[i], value)) {
> +			ADD_VERIFY_ERROR (ctx, g_strdup_printf ("Incompatible parameter value with function signature at 0x%04x", ctx->ip_offset));
> +			return;
> +		}
> +	}
> +
> +	if (sig->hasthis) {
> +		MonoType dummy;
> +
> +		VERIFIER_DEBUG ( printf ("verifying this argument\n"); );
> +
> +		memset (&dummy, 0, sizeof (MonoType));
> +		dummy.data.klass = method->klass;
> +
> +		if (method->klass == mono_defaults.object_class)
> +			dummy.type = MONO_TYPE_OBJECT;
> +		else if(method->klass == mono_defaults.string_class)
> +			dummy.type = MONO_TYPE_STRING;
> +		else
> +			dummy.type = MONO_TYPE_CLASS;

Please don't construct a dummy type here: use klass->byval_arg for references
and klass->this_arg for valuetypes.

> +	if (sig->ret->type != MONO_TYPE_VOID) {
> +		check_overflow (ctx);
> +		set_stack_value (stack_push (ctx), sig->ret, FALSE);

Here is another example where we flag the overflow error, but continue
and scribble over memory corrupting it.

Thanks!

lupus

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