Hi,<br><br>  I&#39;m pretty sure these problems are already fixed on the mono 2.4 branch, and will be in<br>the upcoming 2.4.2.<br><br>                         Zoltan<br><br><div class="gmail_quote">On Fri, May 29, 2009 at 10:14 PM, Ulrich Weigand <span dir="ltr">&lt;<a href="mailto:uweigand@de.ibm.com">uweigand@de.ibm.com</a>&gt;</span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Hello,<br>
<br>
we&#39;ve been running into random mono crashes due to what appears to<br>
be accesses to data structures that were already freed.<br>
<br>
The test case is the OpenSim build and unit-test process using NAnt.<br>
For some reason I don&#39;t quite understand the bug seems to trigger<br>
very frequently on PowerPC64 (to the extent that 75% or so of all<br>
OpenSim build attempts never complete), and sometimes on x86-64,<br>
but rarely (never?) on 32-bit ppc or i386.<br>
<br>
Here&#39;s some results of my debugging attempts of this problem.  This<br>
is my first look at the mono sources -- I may be off track with some<br>
of my conclusions here.  I&#39;d appreciate any help in getting this fixed.<br>
<br>
All tests and the discussion below refer to the mono 2.4 sources.<br>
<br>
<br>
The first problem occurs during mono_metadata_clean_for_image, and<br>
seems related to a recent change introduce to fix bug<br>
<a href="https://bugzilla.novell.com/show_bug.cgi?id=458168" target="_blank">https://bugzilla.novell.com/show_bug.cgi?id=458168</a>.<br>
This change added a call to<br>
  signature_in_image (mono_method_signature ((MonoMethod*)method), image)<br>
to the inflated_method_in_image routine.<br>
<br>
Now the problem with this is that if the inflated method in question<br>
doesn&#39;t yet have a signature allocated.  In this case, the above call<br>
will *cause* the allocation to happen at this time.  In particular,<br>
this can cause new MonoGenericInst or MonoGenericClass structures to<br>
be allocated at this point.<br>
<br>
However, this happens at a time where mono_metadata_clean_for_image<br>
is currently traversing the generic_inst_cache / generic_class_cache<br>
structures, and does not expect new elements to be added to them<br>
while that traversal is ongoing.<br>
<br>
In the case I was debugging, a new MonoGenericInst was created in<br>
such a way that pointed to an already existing MonoGenericClass.<br>
This MonoGenericClass structure, however, was marked as unneeded<br>
during that very same traversal -- but the freshly created<br>
MonoGenericInst was not, as it was added too late.<br>
<br>
This had the effect that at the end of the mono_metadata_clean_for_image<br>
call, the MonoGenericClass was deleted -- but the new MonoGenericInst<br>
was still pointing to it.   During *another* invocation of<br>
mono_metadata_clean_for_image, the ginst_in_image routine would be<br>
called on that broken MonoGenericInst and traverse the pointer to<br>
the freed MonoGenericClass.  Depending on what happened to the memory<br>
in the meantime, crashes or failed assertions result.<br>
<br>
<br>
It seems to be that it is wrong to allocate new data structures<br>
during mono_metadata_clean_for_image, so I guess mono_method_signature<br>
should not be called here.  While I&#39;m not completely sure this is the<br>
right fix, the following patch makes the problem go away for me:<br>
<br>
diff -urNp mono-2.4-orig/mono/metadata/metadata.c mono-2.4/mono/metadata/metadata.c<br>
--- mono-2.4-orig/mono/metadata/metadata.c      2009-02-14 00:33:05.000000000 +0100<br>
+++ mono-2.4/mono/metadata/metadata.c   2009-05-28 20:12:38.000000000 +0200<br>
@@ -2196,7 +2197,8 @@ inflated_method_in_image (gpointer key,<br>
        // <a href="https://bugzilla.novell.com/show_bug.cgi?id=458168" target="_blank">https://bugzilla.novell.com/show_bug.cgi?id=458168</a><br>
        return method-&gt;declaring-&gt;klass-&gt;image == image ||<br>
                (method-&gt;context.class_inst &amp;&amp; ginst_in_image (method-&gt;context.class_inst, image)) ||<br>
-               (method-&gt;context.method_inst &amp;&amp; ginst_in_image (method-&gt;context.method_inst, image)) || signature_in_image (mono_method_signature ((MonoMethod*)method), image);<br>
+               (method-&gt;context.method_inst &amp;&amp; ginst_in_image (method-&gt;context.method_inst, image)) ||<br>
+               (((MonoMethod*)method)-&gt;signature &amp;&amp; signature_in_image (((MonoMethod*)method)-&gt;signature, image));<br>
 }<br>
<br>
 static gboolean<br>
<br>
<br>
The second problem is related to wrapper classes allocated by the<br>
routines in marshal.c.  I&#39;ve been seeing various instances of crashes<br>
caused by those routines returning apparently clobbered method structures.<br>
<br>
It turns out this was caused by stale entried in the caches maintained<br>
by the marshal.c routines.  For example, the<br>
MonoMethod *mono_marshal_get_static_rgctx_invoke (MonoMethod *method)<br>
routine will store the MonoMethod structure describing the wrapper for<br>
&quot;method&quot; into a cache allocated on the mempool associated with the<br>
image related to the method&#39;s class.  For &quot;normal&quot; methods, the method<br>
structure itself was already allocated on that same mempool, so the<br>
wrapper has identical lifetime as the method it wraps.<br>
<br>
However, there is one case where things are more complex: &quot;inflated&quot;<br>
generic methods.  These are *not* allocated on a mempool, but on the<br>
heap, and will be freed at a certain point in time.  However, nothing<br>
ensures that any previously allocated wrapper for such a method is<br>
also freed at this time.<br>
<br>
For the most part, this does not matter much, as the wrapper cache is<br>
indexed using the address of the MonoMethod structure as key.  If the<br>
method no longer exists, it isn&#39;t looked up, so it doesn&#39;t matter that<br>
a stale value is still in the hash table.<br>
<br>
However, it *can* happen that a later allocation of a fresh MonoMethod<br>
just happens to reside at the same address as a method that was deleted<br>
previously.  Now, when looking up a wrapper for the new method, the<br>
stale entry for the old method may indeed be found.  This causes various<br>
problems; in particular, while the cached wrapper method itself is still<br>
live, some of the structures it points to (type, signature) may themselves<br>
have been deleted in the meantime, so random memory may be accessed.<br>
<br>
A similar problem seems to occur for dynamic methods, and for those it<br>
seems special care is taken to remove wrappers for such methods from the<br>
cache when the method is deleted (mono_marshal_free_dynamic_wrappers).<br>
<br>
It looks like a similar approach ought to be taken for inflated methods.<br>
The following patch implements this.  Again, I&#39;m not complete sure this<br>
is the right approach, but it fixes the symptoms for me.<br>
<br>
diff -urNp mono-2.4-orig/mono/metadata/marshal.c mono-2.4/mono/metadata/marshal.c<br>
--- mono-2.4-orig/mono/metadata/marshal.c       2009-02-23 19:43:32.000000000 +0100<br>
+++ mono-2.4/mono/metadata/marshal.c    2009-05-28 19:45:27.000000000 +0200<br>
@@ -75,6 +75,7 @@ typedef struct _MonoRemotingMethods Mono<br>
 #define mono_marshal_lock() EnterCriticalSection (&amp;marshal_mutex)<br>
 #define mono_marshal_unlock() LeaveCriticalSection (&amp;marshal_mutex)<br>
 static CRITICAL_SECTION marshal_mutex;<br>
+static gboolean marshal_mutex_initialized;<br>
<br>
 /* This mutex protects the various cominterop related caches in MonoImage */<br>
 #define mono_cominterop_lock() EnterCriticalSection (&amp;cominterop_mutex)<br>
@@ -599,6 +600,7 @@ mono_marshal_init (void)<br>
                char* com_provider_env = NULL;<br>
                module_initialized = TRUE;<br>
                InitializeCriticalSection (&amp;marshal_mutex);<br>
+               marshal_mutex_initialized = TRUE;<br>
                InitializeCriticalSection (&amp;cominterop_mutex);<br>
                last_error_tls_id = TlsAlloc ();<br>
                load_type_info_tls_id = TlsAlloc ();<br>
@@ -673,6 +675,7 @@ mono_marshal_cleanup (void)<br>
        TlsFree (load_type_info_tls_id);<br>
        TlsFree (last_error_tls_id);<br>
        DeleteCriticalSection (&amp;marshal_mutex);<br>
+       marshal_mutex_initialized = FALSE;<br>
        DeleteCriticalSection (&amp;cominterop_mutex);<br>
 }<br>
<br>
@@ -12908,3 +12911,81 @@ mono_marshal_free_dynamic_wrappers (Mono<br>
                g_hash_table_remove (method-&gt;klass-&gt;image-&gt;runtime_invoke_direct_cache, method);<br>
        mono_marshal_unlock ();<br>
 }<br>
+<br>
+/*<br>
+ * mono_marshal_free_inflated_wrappers:<br>
+ *<br>
+ *   Free wrappers of the inflated method METHOD.<br>
+ */<br>
+<br>
+static gboolean<br>
+signature_method_pair_matches_signature (gpointer key, gpointer value, gpointer user_data)<br>
+{<br>
+       SignatureMethodPair *pair = (SignatureMethodPair*)key;<br>
+       MonoMethodSignature *sig = (MonoMethodSignature*)user_data;<br>
+<br>
+       return mono_metadata_signature_equal (pair-&gt;sig, sig);<br>
+}<br>
+<br>
+void<br>
+mono_marshal_free_inflated_wrappers (MonoMethod *method)<br>
+{<br>
+       MonoMethodSignature *sig = method-&gt;signature;<br>
+<br>
+       g_assert (method-&gt;is_inflated);<br>
+<br>
+       /* Ignore calls occuring late during cleanup.  */<br>
+       if (!marshal_mutex_initialized)<br>
+               return;<br>
+<br>
+       mono_marshal_lock ();<br>
+       /*<br>
+        * FIXME: We currently leak the wrappers. Freeing them would be tricky as<br>
+        * they could be shared with other methods ?<br>
+        */<br>
+<br>
+        /*<br>
+         * indexed by MonoMethodSignature<br>
+         */<br>
+       if (sig &amp;&amp; method-&gt;klass-&gt;image-&gt;delegate_begin_invoke_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;delegate_begin_invoke_cache, sig);<br>
+       if (sig &amp;&amp; method-&gt;klass-&gt;image-&gt;delegate_end_invoke_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;delegate_end_invoke_cache, sig);<br>
+       if (sig &amp;&amp; method-&gt;klass-&gt;image-&gt;delegate_invoke_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;delegate_invoke_cache, sig);<br>
+       if (sig &amp;&amp; method-&gt;klass-&gt;image-&gt;runtime_invoke_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;runtime_invoke_cache, sig);<br>
+<br>
+        /*<br>
+         * indexed by SignatureMethodPair<br>
+         */<br>
+       if (sig &amp;&amp; method-&gt;klass-&gt;image-&gt;delegate_abstract_invoke_cache)<br>
+               g_hash_table_foreach_remove (method-&gt;klass-&gt;image-&gt;delegate_abstract_invoke_cache,<br>
+                                            signature_method_pair_matches_signature, (gpointer)sig);<br>
+<br>
+        /*<br>
+         * indexed by MonoMethod pointers<br>
+         */<br>
+       if (method-&gt;klass-&gt;image-&gt;runtime_invoke_direct_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;runtime_invoke_direct_cache, method);<br>
+       if (method-&gt;klass-&gt;image-&gt;managed_wrapper_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;managed_wrapper_cache, method);<br>
+       if (method-&gt;klass-&gt;image-&gt;native_wrapper_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;native_wrapper_cache, method);<br>
+       if (method-&gt;klass-&gt;image-&gt;remoting_invoke_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;remoting_invoke_cache, method);<br>
+       if (method-&gt;klass-&gt;image-&gt;synchronized_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;synchronized_cache, method);<br>
+       if (method-&gt;klass-&gt;image-&gt;unbox_wrapper_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;unbox_wrapper_cache, method);<br>
+       if (method-&gt;klass-&gt;image-&gt;cominterop_invoke_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;cominterop_invoke_cache, method);<br>
+       if (method-&gt;klass-&gt;image-&gt;cominterop_wrapper_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;cominterop_wrapper_cache, method);<br>
+       if (method-&gt;klass-&gt;image-&gt;static_rgctx_invoke_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;static_rgctx_invoke_cache, method);<br>
+       if (method-&gt;klass-&gt;image-&gt;thunk_invoke_cache)<br>
+               g_hash_table_remove (method-&gt;klass-&gt;image-&gt;thunk_invoke_cache, method);<br>
+<br>
+       mono_marshal_unlock ();<br>
+}<br>
diff -urNp mono-2.4-orig/mono/metadata/marshal.h mono-2.4/mono/metadata/marshal.h<br>
--- mono-2.4-orig/mono/metadata/marshal.h       2009-02-23 19:43:32.000000000 +0100<br>
+++ mono-2.4/mono/metadata/marshal.h    2009-05-27 16:05:38.000000000 +0200<br>
@@ -201,6 +201,9 @@ mono_marshal_get_thunk_invoke_wrapper (M<br>
 void<br>
 mono_marshal_free_dynamic_wrappers (MonoMethod *method) MONO_INTERNAL;<br>
<br>
+void<br>
+mono_marshal_free_inflated_wrappers (MonoMethod *method) MONO_INTERNAL;<br>
+<br>
 /* marshaling internal calls */<br>
<br>
 void *<br>
diff -urNp mono-2.4-orig/mono/metadata/metadata.c mono-2.4/mono/metadata/metadata.c<br>
--- mono-2.4-orig/mono/metadata/metadata.c      2009-02-14 00:33:05.000000000 +0100<br>
+++ mono-2.4/mono/metadata/metadata.c   2009-05-28 20:12:38.000000000 +0200<br>
@@ -21,6 +21,7 @@<br>
 #include &quot;metadata-internals.h&quot;<br>
 #include &quot;class-internals.h&quot;<br>
 #include &quot;class.h&quot;<br>
+#include &quot;marshal.h&quot;<br>
<br>
 static gboolean do_mono_metadata_parse_type (MonoType *type, MonoImage *m, MonoGenericContainer *container,<br>
                                         const char *ptr, const char **rptr);<br>
@@ -2245,6 +2247,8 @@ free_inflated_method (MonoMethodInflated<br>
        int i;<br>
        MonoMethod *method = (MonoMethod*)imethod;<br>
<br>
+       mono_marshal_free_inflated_wrappers (method);<br>
+<br>
        if (method-&gt;signature)<br>
                mono_metadata_free_inflated_signature (method-&gt;signature);<br>
<br>
<br>
With those two patches, I&#39;m no longer able to reproduce crashes during<br>
OpenSim build and unit-test.<br>
<br>
Bye,<br>
Ulrich<br>
<font color="#888888"><br>
--<br>
  Dr. Ulrich Weigand<br>
  GNU Toolchain for Linux on System z and Cell BE<br>
  <a href="mailto:Ulrich.Weigand@de.ibm.com">Ulrich.Weigand@de.ibm.com</a><br>
_______________________________________________<br>
Mono-devel-list mailing list<br>
<a href="mailto:Mono-devel-list@lists.ximian.com">Mono-devel-list@lists.ximian.com</a><br>
<a href="http://lists.ximian.com/mailman/listinfo/mono-devel-list" target="_blank">http://lists.ximian.com/mailman/listinfo/mono-devel-list</a><br>
</font></blockquote></div><br>