[MONO][Marshal] Fix race condition in marshal callback installation by naricc · Pull Request #77383 · dotnet/runtime

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

 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_initmay 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

-	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

+	MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));
+	memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));

+	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

-	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

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

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

 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_initmay 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

-	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

+	MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));
+	memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));

+	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

-	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

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

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

 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_initmay 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

-	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

+	MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));
+	memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));

+	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

-	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

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

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

 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_initmay 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

-	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

+	MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));
+	memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));

+	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

-	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

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

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

 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_initmay 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

-	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

+	MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));
+	memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));

+	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

-	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

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

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

 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_initmay 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

-	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

+	MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));
+	memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));

+	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

-	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

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

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

 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_initmay 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

-	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

+	MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));
+	memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));

+	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

-	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

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

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

 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_initmay 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

-	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

+	MonoMarshalIlgenCallbacks* local_cb = (MonoMarshalIlgenCallbacks*)malloc(sizeof(MonoMarshalIlgenCallbacks));
+	memcpy (local_cb, cb, sizeof (MonoMarshalIlgenCallbacks));

+	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

-	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

+	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 lambdageek, thaystg and vargaz as code owners October 24, 2022 15:11

ghost assigned naricc Oct 24, 2022

ghost added the area-VM-meta-mono label Oct 24, 2022

vargaz reviewed Oct 24, 2022
View reviewed changes

runfoappBot 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 installationOct 24, 2022

naricc changed the title ~~[MONO] Fix race condition in marshal callback installation~~[MONO][Marshal] Fix race condition in marshal callback installationOct 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

Added fix for race condition.
80c4635

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

ghost assigned naricc Oct 24, 2022

ghost added the area-VM-meta-mono label Oct 24, 2022

vargaz reviewed Oct 24, 2022
View reviewed changes

runfoappBot 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 installationOct 24, 2022

naricc changed the title ~~[MONO] Fix race condition in marshal callback installation~~[MONO][Marshal] Fix race condition in marshal callback installationOct 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

Added fix for race condition.
80c4635

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

ghost assigned naricc Oct 24, 2022

ghost added the area-VM-meta-mono label Oct 24, 2022

vargaz reviewed Oct 24, 2022
View reviewed changes

runfoappBot 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 installationOct 24, 2022

naricc changed the title ~~[MONO] Fix race condition in marshal callback installation~~[MONO][Marshal] Fix race condition in marshal callback installationOct 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

Added fix for race condition.
80c4635

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

ghost assigned naricc Oct 24, 2022

ghost added the area-VM-meta-mono label Oct 24, 2022

vargaz reviewed Oct 24, 2022
View reviewed changes

runfoappBot 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 installationOct 24, 2022

naricc changed the title ~~[MONO] Fix race condition in marshal callback installation~~[MONO][Marshal] Fix race condition in marshal callback installationOct 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

Added fix for race condition.
80c4635

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

ghost assigned naricc Oct 24, 2022

ghost added the area-VM-meta-mono label Oct 24, 2022

vargaz reviewed Oct 24, 2022
View reviewed changes

runfoappBot 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 installationOct 24, 2022

naricc changed the title ~~[MONO] Fix race condition in marshal callback installation~~[MONO][Marshal] Fix race condition in marshal callback installationOct 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

Added fix for race condition.
80c4635

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

ghost assigned naricc Oct 24, 2022

ghost added the area-VM-meta-mono label Oct 24, 2022

vargaz reviewed Oct 24, 2022
View reviewed changes

runfoappBot 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 installationOct 24, 2022

naricc changed the title ~~[MONO] Fix race condition in marshal callback installation~~[MONO][Marshal] Fix race condition in marshal callback installationOct 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

Added fix for race condition.
80c4635

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

ghost assigned naricc Oct 24, 2022

ghost added the area-VM-meta-mono label Oct 24, 2022

vargaz reviewed Oct 24, 2022
View reviewed changes

runfoappBot 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 installationOct 24, 2022

naricc changed the title ~~[MONO] Fix race condition in marshal callback installation~~[MONO][Marshal] Fix race condition in marshal callback installationOct 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

Added fix for race condition.
80c4635

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

ghost assigned naricc Oct 24, 2022

ghost added the area-VM-meta-mono label Oct 24, 2022

vargaz reviewed Oct 24, 2022
View reviewed changes

runfoappBot 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 installationOct 24, 2022

naricc changed the title ~~[MONO] Fix race condition in marshal callback installation~~[MONO][Marshal] Fix race condition in marshal callback installationOct 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

	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)

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

Uh oh!

Conversation

naricc commented Oct 24, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naricc commented Oct 24, 2022

Uh oh!

naricc commented Oct 24, 2022

Uh oh!

azure-pipelinesBot commented Oct 24, 2022

Uh oh!

azure-pipelinesBot commented Oct 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nariccOct 24, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdageek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdageek commented Oct 24, 2022

Uh oh!

naricc commented Oct 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

naricc commented Oct 24, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naricc commented Oct 24, 2022

Uh oh!

naricc commented Oct 24, 2022

Uh oh!

azure-pipelinesBot commented Oct 24, 2022

Uh oh!

azure-pipelinesBot commented Oct 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nariccOct 24, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

naricc commented Oct 24, 2022•
edited
Loading

nariccOct 24, 2022•
edited
Loading

naricc commented Oct 24, 2022•
edited
Loading

nariccOct 24, 2022•
edited
Loading