Skip to content
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.

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

[browser][AOT] some assignments are not properly rooted #106200

Closed
pavelsavara opened this issue Aug 9, 2024 · 8 comments
Closed

[browser][AOT] some assignments are not properly rooted #106200

pavelsavara opened this issue Aug 9, 2024 · 8 comments
Assignees
Labels
arch-wasm WebAssembly architecture area-Codegen-AOT-mono os-browser Browser variant of arch-wasm
Milestone

Comments

@pavelsavara
Copy link
Member

pavelsavara commented Aug 9, 2024

corlib_System_RuntimeType_CreateInstanceImpl_System_Reflection_BindingFlags_System_Reflection_Binder_object___System_Globalization_CultureInfo cause AOT compiled binary to fail.

RuntimeError: memory access out of bounds
    at dotnet.native.wasm.corlib_System_RuntimeType_CreateInstanceImpl_System_Reflection_BindingFlags_System_Reflection_Binder_object___System_Globalization_CultureInfo (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[15974]:0x3f3994)
    at dotnet.native.wasm.corlib_System_Activator_CreateInstance_System_Type_System_Reflection_BindingFlags_System_Reflection_Binder_object___System_Globalization_CultureInfo_object__ (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[16893]:0x41c986)
    at dotnet.native.wasm.corlib_System_Activator_CreateInstance_System_Type_object__ (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[16894]:0x41c9d3)
    at dotnet.native.wasm.xunit_execution_dotnet_Xunit_Sdk_ReflectionAttributeInfo_Instantiate_System_Reflection_CustomAttributeData (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[77346]:0x144da51)
    at dotnet.native.wasm.xunit_execution_dotnet_Xunit_Sdk_ReflectionAttributeInfo__ctor_System_Reflection_CustomAttributeData (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[77345]:0x144d66a)
    at dotnet.native.wasm.xunit_execution_dotnet_Xunit_Sdk_ReflectionMethodInfo_GetCustomAttributes_System_Reflection_MethodInfo_System_Type_System_AttributeUsageAttribute (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[77394]:0x1451937)
    at dotnet.native.wasm.xunit_execution_dotnet_Xunit_Sdk_ReflectionMethodInfo_GetCustomAttributes_System_Reflection_MethodInfo_string (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[77393]:0x1451512)
    at dotnet.native.wasm.xunit_execution_dotnet_Xunit_Sdk_ReflectionMethodInfo_GetCustomAttributes_string (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[77392]:0x145149e)
    at dotnet.native.wasm.xunit_execution_dotnet_ReflectionAbstractionExtensions_GetCustomAttributes_Xunit_Abstractions_IMethodInfo_System_Type (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[76392]:0x140673e)
    at dotnet.native.wasm.xunit_execution_dotnet_Xunit_Sdk_XunitTestFrameworkDiscoverer_FindTestsForMethod_Xunit_Abstractions_ITestMethod_bool_Xunit_Sdk_IMessageBus_Xunit_Abstractions_ITestFrameworkDiscoveryOptions (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[77289]:0x14490ac)
    at dotnet.native.wasm.aot_instances_aot_wrapper_gsharedvt_out_sig_u1_this_obju1objobj (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[109452]:0x1aab3f2)
    at wasm://wasm/e47fac2a:wasm-function[7]:0xd5
    at mono_interp_invoke_wasm_jit_call_trampoline (file:///C:/Dev/repro/_framework/dotnet.runtime.js:13450:9)
    at dotnet.native.wasm.do_jit_call (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[109924]:0x1ad2e5b)
    at dotnet.native.wasm.mono_interp_exec_method (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[109919]:0x1ad2761)
    at dotnet.native.wasm.interp_entry (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[109998]:0x1ad5f0c)
    at dotnet.native.wasm.interp_entry_instance_ret_4 (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[110019]:0x1ad6f4b)
    at dotnet.native.wasm.aot_instances_aot_wrapper_gsharedvt_in_sig_u1_this_obju1objobj (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[83015]:0x15662d9)
    at dotnet.native.wasm.xunit_execution_dotnet_Xunit_Sdk_XunitTestFrameworkDiscoverer_FindTestsForType_Xunit_Abstractions_ITestClass_bool_Xunit_Sdk_IMessageBus_Xunit_Abstractions_ITestFrameworkDiscoveryOptions (wasm://wasm/dotnet.native.wasm-0e983046:wasm-function[77292]:0x1449b4a)

CreateInstanceImpl(BindingFlags bindingAttr, Binder? binder, object?[]? args, CultureInfo? culture)
See

binder ??= DefaultBinder;
// deal with the __COMObject case first. It is very special because from a reflection point of view it has no ctors
// so a call to GetMemberCons would fail
bool publicOnly = (bindingAttr & BindingFlags.NonPublic) == 0;
bool wrapExceptions = (bindingAttr & BindingFlags.DoNotWrapExceptions) == 0;
if (argCnt == 0 && (bindingAttr & BindingFlags.Public) != 0 && (bindingAttr & BindingFlags.Instance) != 0
&& (IsValueType))
{
server = CreateInstanceDefaultCtor(publicOnly, wrapExceptions);
}
else
{
ConstructorInfo[] candidates = GetConstructors(bindingAttr);
List<MethodBase> matches = new List<MethodBase>(candidates.Length);
// We cannot use Type.GetTypeArray here because some of the args might be null
Type[] argsType = new Type[argCnt];
for (int i = 0; i < argCnt; i++)
{
if (args[i] != null)
{
argsType[i] = args[i]!.GetType();
}
}
for (int i = 0; i < candidates.Length; i++)
{
if (FilterApplyConstructorInfo((RuntimeConstructorInfo)candidates[i], bindingAttr, CallingConventions.Any, argsType))
matches.Add(candidates[i]);
}
MethodBase[]? cons = new MethodBase[matches.Count];
matches.CopyTo(cons);
if (cons != null && cons.Length == 0)
cons = null;
if (cons == null)
{
throw new MissingMethodException(SR.Format(SR.MissingConstructor_Name, FullName));
}
MethodBase? invokeMethod;
object? state = null;
try
{
invokeMethod = binder.BindToMethod(bindingAttr, cons, ref args, null, culture, null, out state);

There is optional parameter binder
which is in-place re-assigned to DefaultBinder

      IL_0015:  brtrue.s   IL_001e
      IL_0017:  call       class System.Reflection.Binder System.Type::get_DefaultBinder()
      IL_001c:  starg.s    binder

The AOT will generate badly aligned shadow stack

  %vreg_loc_4 = alloca ptr, align 1


%76 = getelementptr i32, ptr %il_state, i32 1
  store i32 23, ptr %76, align 4
  %77 = load ptr, ptr @dummy_got, align 4
  %78 = invoke ptr @corlib_System_Type_get_DefaultBinder(ptr null)
          to label %CALL_NOEX_BB209 unwind label %LPAD1_BB8, !managed_name !800

CALL_NOEX_BB209:                                  ; preds = %BB11
  store volatile ptr %78, ptr %vreg_loc_4, align 4
  br label %BB10

Which will translate to

            local.get $var6
            i32.load offset=95
            i32.eqz
            if
              local.get $var6
              i32.const 23
              i32.store offset=300
              local.get $var6
              local.get $var6
              call $corlib_System_Type_get_DefaultBinder
              local.tee $var0
              i32.store offset=95
            end
            local.get $var6
            i32.load offset=88
            local.set $var0

Which is not rooting the managed pointer to default binder.

The allocation of new List<MethodBase> may trigger GC and move the managed objects.

Then the indirect_call (virtual method dispatch) will de-reference garbage data on the original address.

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-Codegen-AOT-mono os-browser Browser variant of arch-wasm labels Aug 9, 2024
@pavelsavara pavelsavara added this to the 9.0.0 milestone Aug 9, 2024
Copy link
Contributor

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

pavelsavara added a commit to pavelsavara/runtime that referenced this issue Aug 9, 2024
@pavelsavara
Copy link
Member Author

dnet build -bl:aot.binlog /t:Test src/libraries/System.Runtime/tests/System.Runtime.Tests/System.Runtime.Tests.csproj /p:TargetOS=browser /p:TargetArchitecture=wasm /p:Configuration=Release /p:RunAOTCompilation=true /p:Scenario=WasmTestOnV8 /p:EnableAggressiveTrimming=true /p:WasmTestAppArgs="-untilFailed" /p:TrimMode=partial

node --stack-trace-limit=1000 test-main.js --run WasmTestRunner.dll System.Runtime.Tests.dll -notrait category=AdditionalTimezoneChecks -notrait category=OuterLoop -notrait category=failing -untilFailed

The repro is sensitive the exact layout of the binaries, unfortunately.

@pavelsavara
Copy link
Member Author

repro.zip

@lewing
Copy link
Member

lewing commented Aug 12, 2024

Is the issue specific to AOT?

@kg
Copy link
Contributor

kg commented Aug 12, 2024

yeah jitcall trampolines are an AOT feature

@pavelsavara pavelsavara changed the title [browser][jiterp] possibly wrong code for jitcall trampolines [browser][AOT] some assignments are not properly rooted Aug 13, 2024
@pavelsavara
Copy link
Member Author

I updated the issue description, it's not jiterp's fault. It's un-rooted managed pointer in AOT'ed code.

@pavelsavara
Copy link
Member Author

i32.store offset=95 is badly aligned offset into shadow stack. #106313 may fix it

@pavelsavara
Copy link
Member Author

fixed by #106313

@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Codegen-AOT-mono os-browser Browser variant of arch-wasm
Projects
None yet
Development

No branches or pull requests

4 participants