[runtime] Avoid a read-after-free in JI table that frees tomstones. Fixes some of...

Due to how JI table search works, tombstones must be malloc'd as they must have the same address range
of the JI they are replacing.

Since tombstones are heap allocated, they must be freed together with the chunks they belong to.

The current code free tombstones when the chunk refcount goes to zero. The freeing code identify
tombstones by looping through the chunk entries.

The problem is that, under heavy contention, a domain can have multiple tables live at the same time.
It happens there's another thread using the old table while the first creates the new one.

During that period of multiple live tables, we don't free JIs straight away, we queue them to be
freed together with the next table to go through it.

So far so good. Here's the problematic scenario:

1) We expand the JI table, there's contention and the old old is delay freed.
While doing it, chunk X is split. Meaning its refcount will remain 1.

2) JI Y that belongs to chunk X is freed, the tombstone is installed in a chunk of the new table.
That chunk *is not* X.
Since there was contention in (1), Y is put in jit_info_free_queue.

3) We run mono_jit_info_table_free for the old table.

3.a) We free all delayed JIs. Y is freed.
3.b) The old table is freed, chunk X will be freed too, since its refcount dropped to zero.
We walk X looking for tombstones, which includes a pointer to Y, which is freed memory at this point.

I found this issue to be possible with delegates being marshaled to function pointers.
Part (1) just requires a process with high activity, like XS.
Part 2 & 3 requires really bad scheduling in that once sgen restarts the world, the finalizer thread
runs before mono_thread_hazardous_try_free_some (done right after dropping the GC lock).
An alternate to bad scheduling is that the finalizer thread was suspended holding the domain lock and
now after restart, it forces the GC initiator to deschedule when it tries to get that lock.

It goes without saying that this bug is EXTREMELY hard to reproduce. It required me to change the runtime
to force all the above conditions to happen. Let me mention them in case someone needs it:

- In mono_thread_hazardous_free_or_queue, I commented the calls to try_free_delayed_free_item to avoid
freeing the old table too early. I changed the conditional so free_func is never called straight away.

- In mono_thread_hazardous_try_free_some I added a sleep of 100ms, this gave the finalizer thread enough
time to free some JIs.

Finally, the fix itself, the solution is pretty straightforward. Stop scanning chunks and store a singly
linked list of tombstones installed in a given chunk. We build that list by coopting next_jit_code_hash
for this use.

This is safe because chunks are ref counted so we know nothing else could point to those tombstones and
when we copy a chunk into a new one we remove all tombstones.

diff --git a/mono/metadata/domain-internals.h b/mono/metadata/domain-internals.h
index bcf4495e502eee7ce76740ccd20f0178a3c40c0f..019b8f173242fb1ad7f7f0f9d6c6b45cce9eb94a 100644 (file)
--- a/mono/metadata/domain-internals.h
+++ b/mono/metadata/domain-internals.h
@@ -60,6 +60,7 @@ struct _MonoJitInfoTableChunk
        int                    refcount;
        volatile int           num_elements;
        volatile gint8        *last_code_end;
+       MonoJitInfo *next_tombstone;
        MonoJitInfo * volatile data [MONO_JIT_INFO_TABLE_CHUNK_SIZE];
 };
 
@@ -201,7 +202,10 @@ struct _MonoJitInfo {
                gpointer aot_info;
                gpointer tramp_info;
        } d;
-       struct _MonoJitInfo *next_jit_code_hash;
+       union {
+               struct _MonoJitInfo *next_jit_code_hash;
+               struct _MonoJitInfo *next_tombstone;
+       } n;
        gpointer    code_start;
        guint32     unwind_info;
        int         code_size;

diff --git a/mono/metadata/jit-info.c b/mono/metadata/jit-info.c
index 33dd5471c5fc713016ed172149a0b0642720d9bb..17b7107e63d873a495ed3fa70569a79bde57499b 100644 (file)
--- a/mono/metadata/jit-info.c
+++ b/mono/metadata/jit-info.c
@@ -125,18 +125,15 @@ mono_jit_info_table_free (MonoJitInfoTable *table)
 
        for (i = 0; i < num_chunks; ++i) {
                MonoJitInfoTableChunk *chunk = table->chunks [i];
-               int num_elements;
-               int j;
+               MonoJitInfo *tombstone;
 
                if (--chunk->refcount > 0)
                        continue;
 
-               num_elements = chunk->num_elements;
-               for (j = 0; j < num_elements; ++j) {
-                       MonoJitInfo *ji = chunk->data [j];
-
-                       if (IS_JIT_INFO_TOMBSTONE (ji))
-                               g_free (ji);
+               for (tombstone = chunk->next_tombstone; tombstone; ) {
+                       MonoJitInfo *next = tombstone->n.next_tombstone;
+                       g_free (tombstone);
+                       tombstone = next;
                }
 
                g_free (chunk);
@@ -657,13 +654,15 @@ mono_jit_info_table_add (MonoDomain *domain, MonoJitInfo *ji)
 }
 
 static MonoJitInfo*
-mono_jit_info_make_tombstone (MonoJitInfo *ji)
+mono_jit_info_make_tombstone (MonoJitInfoTableChunk *chunk, MonoJitInfo *ji)
 {
        MonoJitInfo *tombstone = g_new0 (MonoJitInfo, 1);
 
        tombstone->code_start = ji->code_start;
        tombstone->code_size = ji->code_size;
        tombstone->d.method = JIT_INFO_TOMBSTONE_MARKER;
+       tombstone->n.next_tombstone = chunk->next_tombstone;
+       chunk->next_tombstone = tombstone;
 
        return tombstone;
 }
@@ -716,7 +715,7 @@ jit_info_table_remove (MonoJitInfoTable *table, MonoJitInfo *ji)
  found:
        g_assert (chunk->data [pos] == ji);
 
-       chunk->data [pos] = mono_jit_info_make_tombstone (ji);
+       chunk->data [pos] = mono_jit_info_make_tombstone (chunk, ji);
 
        /* Debugging code, should be removed. */
        //jit_info_table_check (table);
@@ -840,7 +839,7 @@ jit_info_next_value (gpointer value)
 {
        MonoJitInfo *info = (MonoJitInfo*)value;
 
-       return (gpointer*)&info->next_jit_code_hash;
+       return (gpointer*)&info->n.next_jit_code_hash;
 }
 
 void