Skip to content

Commit

Permalink
Ensure to clean-up Value instances (in arguments for a function cal…
Browse files Browse the repository at this point in the history
…l, and in results for an untyped callback) when e.g. allocating an `externref` fails.

We don't need to do such a clean-up for unchecked function calls that use `ValueRaw` because in that case we don't own `externref` values.
  • Loading branch information
kpreisser committed Jun 27, 2024
1 parent 58c7f84 commit ad6764d
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 9 deletions.
41 changes: 35 additions & 6 deletions src/Function.cs
Original file line number Diff line number Diff line change
Expand Up @@ -295,14 +295,28 @@ private unsafe void InvokeWithoutReturn(Span<ValueRaw> arguments, StoreContext s
Span<Value> args = stackalloc Value[Parameters.Count];
for (var i = 0; i < arguments.Length; ++i)
{
args[i] = arguments[i].ToValue(store, Parameters[i]);
}
try
{
args[i] = arguments[i].ToValue(store, Parameters[i]);
}
catch
{
// Clean-up the previous values in case an exception occured (e.g. when
// `wasmtime_externref_new` failed).
for (int releaseIndex = 0; releaseIndex < i; releaseIndex++)
{
args[releaseIndex].Release(store);
}

// Make some space to store the return results
Span<Value> resultsSpan = stackalloc Value[Results.Count];
throw;
}
}

try
{
// Make some space to store the return results
Span<Value> resultsSpan = stackalloc Value[Results.Count];

var trap = Invoke(args, resultsSpan);
if (trap != IntPtr.Zero)
{
Expand Down Expand Up @@ -344,7 +358,6 @@ private unsafe void InvokeWithoutReturn(Span<ValueRaw> arguments, StoreContext s
args[i].Release(store);
}
}

}

/// <summary>
Expand Down Expand Up @@ -647,7 +660,23 @@ internal static unsafe IntPtr InvokeUntypedCallback(UntypedCallbackDelegate call

for (int i = 0; i < resultsSpan.Length; i++)
{
results[i] = resultsSpan[i].ToValue(caller.Store, resultKinds[i]);
try
{
results[i] = resultsSpan[i].ToValue(caller.Store, resultKinds[i]);
}
catch
{
// Clean-up the previous result values in case an exception occured
// (e.g. when `wasmtime_externref_new` failed), because we will
// return an error in that case and therefore can't pass ownership
// of already allocated result values.
for (int releaseIndex = 0; releaseIndex < i; releaseIndex++)
{
results[releaseIndex].Release(caller.Store);
}

throw;
}
}
}
finally
Expand Down
18 changes: 16 additions & 2 deletions src/Value.cs
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,22 @@ private static class Native
}
}

/// <summary>
///
/// </summary>
/// <remarks>
/// <para>
/// When owning the value and you are finished with using it, you must release/unroot
/// it by calling the <see cref="Release(Store)"/> method. After that, the
/// <see cref="Value"/> must no longer be used.
/// </para>
/// <para>
/// Previously, this type implemented the <see cref="IDisposable"/> interface, but since
/// Wasmtime v20.0.0, unrooting the value requires passing a store context, which is why
/// the <see cref="Release(Store)"/> method needs to explicitly be called, passing a
/// <see cref="Store"/>.
/// </para>
/// </remarks>
[StructLayout(LayoutKind.Sequential)]
internal struct Value
{
Expand Down Expand Up @@ -276,7 +292,6 @@ public static Value FromValueBox(Store store, ValueBox box)
ref value.of.externref
))
{
// TODO: Check in which places this exception could be thrown.
throw new WasmtimeException("The host wasn't able to create more GC values at this time.");
}
}
Expand Down Expand Up @@ -365,7 +380,6 @@ public static Value FromObject(Store store, object? o, ValueKind kind)
ref value.of.externref
))
{
// TODO: Check in which places this exception could be thrown.
throw new WasmtimeException("The host wasn't able to create more GC values at this time.");
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/ValueRaw.cs
Original file line number Diff line number Diff line change
Expand Up @@ -292,13 +292,16 @@ public void Box(StoreContext storeContext, Store store, ref ValueRaw valueRaw, T

try
{
// The externref allocated here won't be rooted when this method returns
// (see comments below), so unlike with the regular `Value`, we don't
// need to do a special clean-up in the caller when trying to allocate
// multiple (externref) values and one of it fails.
if (!Value.Native.wasmtime_externref_new(
storeContext.handle,
GCHandle.ToIntPtr(gcHandle),
Value.Finalizer,
ref externref))
{
// TODO: Check in which places this exception could be thrown.
throw new WasmtimeException("The host wasn't able to create more GC values at this time.");
}
}
Expand Down

0 comments on commit ad6764d

Please sign in to comment.