Skip to content

[MONO][Marshal] Fix race condition in marshal callback installation #77383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
naricc wants to merge 2 commits into from
Closed

[MONO][Marshal] Fix race condition in marshal callback installation #77383

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80c4635
Added fix for race condition.
Oct 22, 2022
c5ac405
Write barrier/g_free.
Oct 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions src/mono/mono/metadata/marshal-ilgen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,21 @@ static void emit_string_free_icall (MonoMethodBuilder *mb, MonoMarshalConv conv)
// TODO: Does this need to loose the mono_ prefix?
static void mono_marshal_ilgen_legacy_init (void);

static gboolean ilgen_cb_inited = FALSE;
static MonoMarshalIlgenCallbacks ilgen_marshal_cb;
static MonoMarshalIlgenCallbacks* ilgen_marshal_cb = NULL;

void
mono_install_marshal_callbacks_ilgen (MonoMarshalIlgenCallbacks *cb)
{
g_assert (!ilgen_cb_inited);
Copy link
Contributor

@vargaz vargaz Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the race occur ? This should be called during startup.

Copy link
Contributor Author

@naricc naricc Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas Was reporting what looks like a race condition here: #77090. I'm not sure exactly what interleaving can cause this though, and have not been able to trigger it locally.

Copy link
Member

@lambdageek lambdageek Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vargaz @naricc since e107bcb, mono_marshal_ilgen_init may be called by embedders early, but it doesn't have to be called. In the case where the driver doesn't call it early (for example the normal desktop mono driver doesn't call it), get_marshal_cb will initialize lazily. The race is in the lazy case where one thread can see ilgen_cb_inited == TRUE, but the callbacks are all still null.

Copy link
Contributor

@vargaz vargaz Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it get called during runtime startup ?

Copy link
Member

@lambdageek lambdageek Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it get called during runtime startup ?

I was surprised, but apparently it doesn't. Going back to the very first PR it's always been lazy

Copy link
Contributor

@vargaz vargaz Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now on wasm at least, all such initialization needs to happen before the runtime is initialized, i.e.
https://github.com/dotnet/runtime/blob/main/src/mono/wasm/runtime/driver.c#L593
When mono_jit_init_version () is called, it can check whenever the embedder has made some custom changes, and if not, initialize things the default way.

Copy link
Member

@lambdageek lambdageek Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I don't have any objection to just using mono_marshal_ilgen_init to set a flag and doing all the callback initialization late during startup. @naricc I think that's what you wanted to do for the component in the first place right?

Copy link
Contributor Author

@naricc naricc Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lambdageek That is what I am doing for the componentization change.

So is that what we should do here too, instead of the thing with atomics? mono_marshal_ilgen_init sets a flag; we check that flag during, say, mini_init(). At which point we do callback installation if it is set?

Copy link
Contributor Author

@naricc naricc Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess in the case that we are using NOILGEN and no one has called mono_marshal_ilgen_init we just want to go ahead and install the noilgen callbacks?

Copy link
Member

@lambdageek lambdageek Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that sounds right.

So is that what we should do here too, instead of the thing with atomics? mono_marshal_ilgen_init sets a flag; we check that flag during, say, mini_init(). At which point we do callback installation if it is set?

Yea, basically at the point where in the componentized versino we'd call the component init function, here we'd just set up the callbacks.

I think writing down #77383 (comment) helped me to think about what the real problem is. It's not concurrency. It's that we have a confusing API for choosing between ilgen/noilgen. We should init the marshaling stack at startup - there's no need for it to be lazy. You were right in your proposal about mono_marshal_ilgen_init (just use it to set a flag), but I didn't understand the problem properly.

g_assert (cb->version == MONO_MARSHAL_CALLBACKS_VERSION);
memcpy (&ilgen_marshal_cb, cb, sizeof (MonoMarshalIlgenCallbacks));
ilgen_cb_inited = TRUE;
}
MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));
Copy link
Member

@lambdageek lambdageek Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use g_malloc/g_free

memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));
Copy link
Member

@lambdageek lambdageek Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a memory barrier between the memcpy and the CAS

Suggested change
memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));
memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));
mono_memory_barrier ();


if (mono_atomic_cas_ptr((void**)&ilgen_marshal_cb, local_cb, NULL ))
Copy link
Member

@lambdageek lambdageek Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (mono_atomic_cas_ptr((void**)&ilgen_marshal_cb, local_cb, NULL ))
if (mono_atomic_cas_ptr((void**)&ilgen_marshal_cb, local_cb, NULL ) != NULL)

nit: usually the pattern is if (mono_atomic_cas_ptr (dest, newValue, expectedValue) != expectedValue)

{
// cas failed
free(local_cb);
}
}

static void
emit_struct_free (MonoMethodBuilder *mb, MonoClass *klass, int struct_var)
Expand Down Expand Up @@ -2717,14 +2720,14 @@ emit_marshal_variant_ilgen (EmitMarshalContext *m, int argnum, MonoType *t,
static MonoMarshalIlgenCallbacks *
get_marshal_cb (void)
{
if (G_UNLIKELY (!ilgen_cb_inited)) {
if (G_UNLIKELY (!ilgen_marshal_cb)) {
#ifdef ENABLE_ILGEN
mono_marshal_ilgen_init ();
#else
mono_marshal_noilgen_init_heavyweight ();
#endif
}
return &ilgen_marshal_cb;
return ilgen_marshal_cb;
}

int
Expand Down
20 changes: 13 additions & 7 deletions src/mono/mono/metadata/marshal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6237,31 +6237,37 @@ mono_marshal_emit_native_wrapper (MonoImage *image, MonoMethodBuilder *mb, MonoM
get_marshal_cb ()->emit_native_wrapper (image, mb, sig, piinfo, mspecs, func, flags);
}

static MonoMarshalLightweightCallbacks marshal_lightweight_cb;
static gboolean lightweight_cb_inited = FALSE;
static MonoMarshalLightweightCallbacks* marshal_lightweight_cb = NULL;


void
mono_install_marshal_callbacks (MonoMarshalLightweightCallbacks *cb)
{
g_assert (!lightweight_cb_inited);
g_assert (cb->version == MONO_MARSHAL_CALLBACKS_VERSION);
memcpy (&marshal_lightweight_cb, cb, sizeof (MonoMarshalLightweightCallbacks));
lightweight_cb_inited = TRUE;
MonoMarshalLightweightCallbacks* local_cb = (MonoMarshalLightweightCallbacks*)malloc(sizeof(MonoMarshalLightweightCallbacks));
memcpy (local_cb, cb, sizeof (MonoMarshalLightweightCallbacks));

if (mono_atomic_cas_ptr((void**)&marshal_lightweight_cb, local_cb, NULL))
Copy link
Member

@lambdageek lambdageek Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comments as the other method:

  1. use g_malloc/g_free
  2. barrier after memcpy
  3. add != NULL to the if condition

{
// cas failed
free(local_cb);
}
}


static MonoMarshalLightweightCallbacks *
get_marshal_cb (void)
{

if (G_UNLIKELY (!lightweight_cb_inited)) {
if (G_UNLIKELY (!marshal_lightweight_cb)) {
#ifdef ENABLE_ILGEN
mono_marshal_lightweight_init ();
#else
mono_marshal_noilgen_init_lightweight ();
#endif
}

return &marshal_lightweight_cb;
return marshal_lightweight_cb;
}


Expand Down