[Mono-dev] [PATCH] Method invoke verification

[Rodrigo Kumpera]

Hi,

The attached patch fixed this issues and remove some commented code too. I
have changed all call sites of functions that can fail to verify the return
value.

Cheers,
Rodrigo Kumpera

On 6/12/07, Paolo Molaro <lupus at ximian.com> wrote:
>
> 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
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20070612/41cf81ad/attachment.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: application/octet-stream
Size: 39098 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20070612/41cf81ad/attachment.obj