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

[Rodrigo Kumpera]

On 6/21/07, Paolo Molaro <lupus at ximian.com> wrote:
>
> 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.
>

Definely, the spec say nothing about that, I'll add this check and mark it
as a spec issue. About the accessibility checks, I'm going to tackle all
cases at once, as it should quite hairy, I believe.


> +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.


I don't quite follow you, I should test it it's a load/store to a class with
bad overlapping, even if the target is a good field?

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/20070621/20e856e8/attachment.html