[Mono-dev] [PATCH] Handle more gracefully invalid generic instantiations

[Raja R Harinath]

Hi,

Rodrigo Kumpera <kumpera at gmail.com> writes:

> The attached patch changes class.c/inflate_generic_type to not abort the runtime when facing a bad instantiation.
>
> My only issue is that I'm not sure if mono_class_inflate_generic_type* and mono_class_inflate_generic_class should
> set a loader error when returning null. I don't think this is necessary as this is something their callers should do. But maybe
> mono_class_inflate_generic_method_full should.
>
> This patch doesn't fix the whole thing as an audit of all callers of mono_class_inflate_generic_type(_with_mempool | _no_copy)
> and mono_class_inflate_generic_class are required to property handle them returning NULL.
>
> I got some local tests for these errors, but they are not in good shape for been part of this patch.

[snip]

> +static gboolean
> +inflate_generic_type (MonoImage *image, MonoType *type, MonoGenericContext *context, MonoType **res)
>  {
> +#define SUCCESS(VAL) do { *res = VAL; return TRUE; } while (0)
> +#define ERROR() do { *res = NULL; return FALSE; } while (0)
>  	switch (type->type) {
>  	case MONO_TYPE_MVAR: {
>  		MonoType *nt;
>  		int num = mono_type_get_generic_param_num (type);
>  		MonoGenericInst *inst = context->method_inst;
>  		if (!inst || !inst->type_argv)
> -			return NULL;
> +			SUCCESS (NULL);
>  		if (num >= inst->type_argc)
> -			g_error ("MVAR %d (%s) cannot be expanded in this context with %d instantiations",
> -				num, mono_generic_param_info (type->data.generic_param)->name, inst->type_argc);
> -
> +			ERROR ();

Hmm, so much machinery for the one use of ERROR!

I think the issue is that we're forced to intertwine this particular
error check in the middle of code dealing with the mechanics of
inflating.  It'd be much nicer for inflate_generic_type to have the
precondition that no VAR or MVAR in 'type' will be out-of-bounds WRT
'context'.

The problem is that this precondition check is currently expensive, as
it would duplicate the same recursive traversal.  However, we _can_ and
_should_ make it non-recursive -- we can replace the field and
computation of MonoGenericInst::is_open with something like
MonoGenericInst::min_context_size (yeah that name is horribly bad.  I've
been putting of writing that code since I couldn't get a better name).

The reason I think this is better is that we already have code in JIT
and the verifier that need only that information (i.e. are equivalent to
that precondition check), but are forced to walk the MonoType tree
because 'is_open' is too limited.

- Hari