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

naricc · 2022-10-24T15:11:31Z

This fixes a race condition on ilgen_cb_inited; this flag was used to determine if callbacks were already installed, but had no synchronization around it. Replaced with a pointer to a dynamically allocated buffer and a cas.

Fixes: #74603
Fixes: #77090

naricc · 2022-10-24T15:12:05Z

/azp run runtime-extra-platforms

naricc · 2022-10-24T15:12:13Z

/azp run runtime-wasm

azure-pipelines · 2022-10-24T15:12:23Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2022-10-24T15:12:23Z

Azure Pipelines successfully started running 1 pipeline(s).

vargaz · 2022-10-24T15:23:01Z

src/mono/mono/metadata/marshal-ilgen.c


 void
 mono_install_marshal_callbacks_ilgen (MonoMarshalIlgenCallbacks *cb)
 {
-	g_assert (!ilgen_cb_inited);


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

@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.

@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.

Doesn't it get called during runtime startup ?

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

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.

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?

@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?

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?

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.

lambdageek

lgtm. needs barriers. also some style nits

lambdageek · 2022-10-24T18:15:26Z

src/mono/mono/metadata/marshal-ilgen.c

-	memcpy (&ilgen_marshal_cb, cb, sizeof (MonoMarshalIlgenCallbacks));
-	ilgen_cb_inited = TRUE;
-}
+	MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));


nit: use g_malloc/g_free

lambdageek · 2022-10-24T18:16:41Z

src/mono/mono/metadata/marshal-ilgen.c


+	if (mono_atomic_cas_ptr((void**)&ilgen_marshal_cb, local_cb, NULL  ))


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)

lambdageek · 2022-10-24T18:18:29Z

src/mono/mono/metadata/marshal-ilgen.c

-	ilgen_cb_inited = TRUE;
-}
+	MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));
+	memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));


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 ();

lambdageek · 2022-10-24T18:19:30Z

src/mono/mono/metadata/marshal.c

+	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))


same comments as the other method:

use g_malloc/g_free

barrier after memcpy

add != NULL to the if condition

lambdageek · 2022-10-24T18:21:03Z

Added fix for race condition.

Please use a better summary. something like [mono][marshal] Fix race condition in callback initialization

I guess we're going with a flag instead of atomics

naricc · 2022-10-25T15:46:54Z

I am abandoing this PR in favor of doing it the flag way ( see discussion above). That is a different change, so I will do it in a different PR.

Added fix for race condition.

80c4635

naricc requested review from vargaz, lambdageek and thaystg as code owners October 24, 2022 15:11

ghost assigned naricc Oct 24, 2022

dotnet-issue-labeler bot added the area-VM-meta-mono label Oct 24, 2022

vargaz reviewed Oct 24, 2022

View reviewed changes

runfoapp bot mentioned this pull request Oct 24, 2022

system.text.regularexpressions.tests work item #72834

Open

lambdageek previously approved these changes Oct 24, 2022

View reviewed changes

Write barrier/g_free.

c5ac405

naricc changed the title ~~Added fix for race condition.~~ [MONO] Fix race condition in marshal callback installation Oct 24, 2022

naricc changed the title ~~[MONO] Fix race condition in marshal callback installation~~ [MONO][Marshal] Fix race condition in marshal callback installation Oct 24, 2022

naricc closed this Oct 25, 2022

naricc mentioned this pull request Oct 25, 2022

[MONO][MARSHAL] Initialize ilgen with a flag #77448

Merged

ghost locked as resolved and limited conversation to collaborators Nov 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

naricc commented Oct 24, 2022 •

edited

Loading

naricc commented Oct 24, 2022

naricc commented Oct 24, 2022

azure-pipelines bot commented Oct 24, 2022

azure-pipelines bot commented Oct 24, 2022

vargaz Oct 24, 2022

naricc Oct 24, 2022 •

edited

Loading

lambdageek Oct 24, 2022

vargaz Oct 24, 2022

lambdageek Oct 24, 2022

vargaz Oct 24, 2022

lambdageek Oct 24, 2022

naricc Oct 24, 2022

naricc Oct 24, 2022

lambdageek Oct 25, 2022

lambdageek left a comment

lambdageek Oct 24, 2022

lambdageek Oct 24, 2022

lambdageek Oct 24, 2022

lambdageek Oct 24, 2022

lambdageek commented Oct 24, 2022

naricc commented Oct 25, 2022


		if (mono_atomic_cas_ptr((void**)&ilgen_marshal_cb, local_cb, NULL ))

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

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

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

Conversation

naricc commented Oct 24, 2022 • edited Loading

naricc commented Oct 24, 2022

naricc commented Oct 24, 2022

azure-pipelines bot commented Oct 24, 2022

azure-pipelines bot commented Oct 24, 2022

Choose a reason for hiding this comment

naricc Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek commented Oct 24, 2022

naricc commented Oct 25, 2022

naricc commented Oct 24, 2022 •

edited

Loading

naricc Oct 24, 2022 •

edited

Loading