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)

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


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report for Bug 58118 on GitHub or Developer Community if you have new information to add and do not yet see a matching new report.

If the latest results still closely match this report, you can use the original description:

  • Export the original title and description: GitHub Markdown or Developer Community HTML
  • Copy the title and description into the new report. Adjust them to be up-to-date if needed.
  • Add your new information.

In special cases on GitHub you might also want the comments: GitHub Markdown with public comments

Related Links:
Status:
ASSIGNED

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.