Bug 58118 - Race conditions of mini-trampolines.c
Summary: Race conditions of mini-trampolines.c
Status: ASSIGNED
Alias: None
Product: Runtime
Classification: Mono
Component: General (show other bugs)
Version: unspecified
Hardware: PC Linux
: --- normal
Target Milestone: Future Cycle (TBD)
Assignee: Zoltan Varga
URL:
Depends on:
Blocks:
 
Reported: 2017-07-12 22:38 UTC by Armin
Modified: 2017-07-14 02:34 UTC (History)
3 users (show)

See Also:
Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Stack trace (10.69 KB, text/plain)
2017-07-12 22:38 UTC, Armin
Details

Description Armin 2017-07-12 22:38:16 UTC
Created attachment 23509 [details]
Stack trace

I found another data race with clang's ThreadSanitizer that I would love to discuss - this one is part of `common_call_trampoline ()` in mini-trampolines.c.

I have last encountered this race with commit 57239e923f3f5942d62e058d7fe4dc01f67c0735 but I have seen it earlier with 13aeb8836a5c036a7b8918a2e53f58a10475069e already. Since the surrounding code didn't change much lately, I haven't tested it with newer commits.

Please find tsan's corresponding stack trace attached. In addition, I will also write down what I think about this specific race.

I picked a few parts of `common_call_trampoline ()` that I find most interesting (the lines that start with "!" are mentioned by tsan):

static gpointer common_call_trampoline (mgreg_t *regs, guint8 *code, MonoMethod *m, MonoVTable *vt, gpointer *vtable_slot, MonoError *error) {
  // ...
  gpointer *orig_vtable_slot, *vtable_slot_to_patch = NULL;
  // ...
  vtable_slot_to_patch = vtable_slot;
  // ...
  if (imt_call) {
    // ...
    if (mono_object_is_transparent_proxy (this_arg)) {
      // ...
      vtable_slot = mini_resolve_imt_method (/* ... */);
      // ...
      vtable_slot_to_patch = NULL;
      // ...
    } else {
      // ...
      vtable_slot_to_patch = vtable_slot;
      // ...
    }
  }
  // ...
  if (vtable_slot) {
    if (vtable_slot_to_patch && (mono_aot_is_got_entry (code, (guint8*)vtable_slot_to_patch) || mono_domain_owns_vtable_slot (mono_domain_get (), vtable_slot_to_patch))) {
!     g_assert (*vtable_slot_to_patch);
!     *vtable_slot_to_patch = mono_get_addr_from_ftnptr (addr);
    }
  }
  // ...
}

Two threads are working with the same `vtable_slot_to_patch`. I have only encountered this specific race with the read of `g_assert ()` so far, however, I don't see how it does not affect the surrounding if-condition or an even larger part of `common_call_trampoline ()`. I understand that there are no mutexes or other means of direct synchronisation connected to `vtable_slot_to_patch`.
Comment 1 Ludovic Henry 2017-07-12 23:15:48 UTC
Zoltan, could you please assess if these races are fixable? Thank you.
Comment 2 Zoltan Varga 2017-07-12 23:58:59 UTC
This is not really a race, both threads will store the same value, or a sematically equivalent value.
Comment 3 Armin 2017-07-13 23:30:23 UTC
Can this parallel usage of `vtable_slot_to_patch` safely be ignored and whitelisted?
Comment 4 Zoltan Varga 2017-07-14 02:34:24 UTC
I think so.

Note You need to log in before you can comment on or make changes to this bug.