Skip to content

Commit f6e73fc

Browse files
AndyAyersMSCopilotjkotas
authored
Wasm: don't zero-pad the caller's return buffer in the interp-to-R2R thunk (#132257)
WasmInterpreterToR2RThunkNode zero-padded pRet out to an aligned size after calling the R2R body. pRet is the interpreter's own return slot only when the interpreter made the call; it is the compiled caller's return buffer when we arrive via WasmR2RToInterpreterThunkNode, and CallDescrData::pRetBuffArg when we arrive via CallDescrWorkerInternal. In those cases the buffer is exactly as large as the struct, so the padding wrote past its end. For a 12-byte struct that is four bytes into the next frame slot. In JsonDocument.Parse it zeroed the reference field of a live ReadOnlySpan and left its length intact, so the first Parse threw NullReferenceException from Utf8JsonReader.ReadSingleSegment. Fixes #131640 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com>
1 parent d00a0a2 commit f6e73fc

3 files changed

Lines changed: 207 additions & 18 deletions

File tree

‎src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmInterpreterToR2RThunkNode.cs‎

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -313,24 +313,8 @@ protected override void EmitCode(NodeFactory factory, ref Wasm.WasmEmitter instr
313313
}
314314
}
315315

316-
// For struct returns via retbuf: the R2R function has already written the struct
317-
// into pRet. Zero-pad to the appropriate alignment boundary.
318-
if (hasRetBuffArg)
319-
{
320-
TypeDesc returnType = methodSignature.ReturnType;
321-
int structSize = returnType.GetElementSize().AsInt;
322-
int alignment = structSize <= 4 ? 4 : 8;
323-
int padding = AlignmentHelper.AlignUp(structSize, alignment) - structSize;
324-
if (padding > 0)
325-
{
326-
expressions.Add(Local.Get(LocalPRet));
327-
expressions.Add(I32.Const(structSize));
328-
expressions.Add(I32.Add);
329-
expressions.Add(I32.Const(0));
330-
expressions.Add(I32.Const(padding));
331-
expressions.Add(Memory.Fill());
332-
}
333-
}
316+
// For struct returns via retbuf the R2R function has already written the struct into
317+
// pRet, and there is nothing more to do.
334318

335319
// Restore the stack pointer global
336320
expressions.Add(Local.Get(localSavedSp));

‎src/tests/JIT/Regression/JitBlue/Runtime_131640/Runtime_131640.cs‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
namespace Runtime_131640;
5+
6+
using System;
7+
using System.Buffers;
8+
using System.Runtime.CompilerServices;
9+
using System.Runtime.InteropServices;
10+
using Xunit;
11+
12+
// On wasm32 these are 12, 20 and 28 bytes: sizes the interpreter-to-R2R thunk used to zero-pad
13+
// out to the next multiple of 8, writing past the end of the caller's return buffer.
14+
public struct Db12
15+
{
16+
public byte[] Data;
17+
public int Length;
18+
public bool Flag;
19+
20+
[MethodImpl(MethodImplOptions.NoInlining)]
21+
public static Db12 CreateRented(int n)
22+
{
23+
Db12 d = default;
24+
d.Data = ArrayPool<byte>.Shared.Rent(n < 16 ? 16 : n);
25+
d.Flag = true;
26+
return d;
27+
}
28+
29+
public void Dispose() => Return(ref Data);
30+
31+
internal static void Return(ref byte[] data)
32+
{
33+
byte[] d = data;
34+
data = null;
35+
if (d is not null)
36+
{
37+
ArrayPool<byte>.Shared.Return(d);
38+
}
39+
}
40+
}
41+
42+
public struct Db20
43+
{
44+
public byte[] Data;
45+
public int Length;
46+
public int A;
47+
public int B;
48+
public int C;
49+
50+
[MethodImpl(MethodImplOptions.NoInlining)]
51+
public static Db20 CreateRented(int n)
52+
{
53+
Db20 d = default;
54+
d.Data = ArrayPool<byte>.Shared.Rent(n < 16 ? 16 : n);
55+
d.C = 1;
56+
return d;
57+
}
58+
59+
public void Dispose() => Db12.Return(ref Data);
60+
}
61+
62+
public struct Db28
63+
{
64+
public byte[] Data;
65+
public int Length;
66+
public int A;
67+
public int B;
68+
public int C;
69+
public int D;
70+
public int E;
71+
72+
[MethodImpl(MethodImplOptions.NoInlining)]
73+
public static Db28 CreateRented(int n)
74+
{
75+
Db28 d = default;
76+
d.Data = ArrayPool<byte>.Shared.Rent(n < 16 ? 16 : n);
77+
d.E = 1;
78+
return d;
79+
}
80+
81+
public void Dispose() => Db12.Return(ref Data);
82+
}
83+
84+
public struct RowStack
85+
{
86+
public byte[] Buf;
87+
public int Len;
88+
public int Idx;
89+
90+
[MethodImpl(MethodImplOptions.NoInlining)]
91+
public RowStack(int n)
92+
{
93+
Buf = ArrayPool<byte>.Shared.Rent(n);
94+
Len = n;
95+
Idx = n;
96+
}
97+
98+
public void Dispose() => Db12.Return(ref Buf);
99+
}
100+
101+
public class Runtime_131640
102+
{
103+
// Two byref-like values are kept live across the struct-returning call so the repro does
104+
// not depend on a single frame slot landing above that call's return buffer.
105+
[MethodImpl(MethodImplOptions.NoInlining)]
106+
private static int Sum(ReadOnlySpan<byte> span, ReadOnlySpan<byte> other, ref RowStack stack)
107+
{
108+
Assert.False(Unsafe.IsNullRef(ref MemoryMarshal.GetReference(span)));
109+
Assert.False(Unsafe.IsNullRef(ref MemoryMarshal.GetReference(other)));
110+
111+
int total = 0;
112+
for (int i = 0; i < span.Length; i++)
113+
{
114+
total += span[i];
115+
}
116+
117+
stack.Idx = 0;
118+
return total;
119+
}
120+
121+
// The spans are materialized before the struct-returning call and stay live across it.
122+
[MethodImpl(MethodImplOptions.NoInlining)]
123+
private static int Run12(ReadOnlyMemory<byte> bytes)
124+
{
125+
ReadOnlySpan<byte> span = bytes.Span;
126+
ReadOnlySpan<byte> other = bytes.Span;
127+
Db12 db = Db12.CreateRented(bytes.Length);
128+
RowStack stack = new RowStack(512);
129+
try
130+
{
131+
return Sum(span, other, ref stack);
132+
}
133+
finally
134+
{
135+
stack.Dispose();
136+
db.Dispose();
137+
}
138+
}
139+
140+
[MethodImpl(MethodImplOptions.NoInlining)]
141+
private static int Run20(ReadOnlyMemory<byte> bytes)
142+
{
143+
ReadOnlySpan<byte> span = bytes.Span;
144+
ReadOnlySpan<byte> other = bytes.Span;
145+
Db20 db = Db20.CreateRented(bytes.Length);
146+
RowStack stack = new RowStack(512);
147+
try
148+
{
149+
return Sum(span, other, ref stack);
150+
}
151+
finally
152+
{
153+
stack.Dispose();
154+
db.Dispose();
155+
}
156+
}
157+
158+
[MethodImpl(MethodImplOptions.NoInlining)]
159+
private static int Run28(ReadOnlyMemory<byte> bytes)
160+
{
161+
ReadOnlySpan<byte> span = bytes.Span;
162+
ReadOnlySpan<byte> other = bytes.Span;
163+
Db28 db = Db28.CreateRented(bytes.Length);
164+
RowStack stack = new RowStack(512);
165+
try
166+
{
167+
return Sum(span, other, ref stack);
168+
}
169+
finally
170+
{
171+
stack.Dispose();
172+
db.Dispose();
173+
}
174+
}
175+
176+
[Theory]
177+
[InlineData(12)]
178+
[InlineData(20)]
179+
[InlineData(28)]
180+
public static void StructReturnDoesNotOverflowTheCallersReturnBuffer(int wasm32StructSize)
181+
{
182+
ReadOnlyMemory<byte> data = new byte[] { 1, 2, 3, 4, 5 };
183+
int actual = wasm32StructSize switch
184+
{
185+
12 => Run12(data),
186+
20 => Run20(data),
187+
_ => Run28(data),
188+
};
189+
190+
Assert.Equal(15, actual);
191+
}
192+
}

‎src/tests/JIT/Regression/JitBlue/Runtime_131640/Runtime_131640.csproj‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<PropertyGroup>
3+
<Optimize>True</Optimize>
4+
<!-- The bug only reproduces when the caller is optimized R2R code, so force crossgen on
5+
the browser leg. AlwaysUseCrossGen2 only takes effect through the project's own run
6+
script, which requires process isolation. -->
7+
<AlwaysUseCrossGen2 Condition="'$(TargetOS)' == 'browser'">true</AlwaysUseCrossGen2>
8+
<RequiresProcessIsolation>true</RequiresProcessIsolation>
9+
</PropertyGroup>
10+
<ItemGroup>
11+
<Compile Include="$(MSBuildProjectName).cs" />
12+
</ItemGroup>
13+
</Project>

0 commit comments

Comments
 (0)