From d9d50a340241ff96c9876bc8472d474adb989a6d Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 5 Jun 2022 01:20:21 +0300 Subject: [PATCH 1/3] Add a test --- .../JIT/Directed/StructABI/MisSizedStructs.cs | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/src/tests/JIT/Directed/StructABI/MisSizedStructs.cs b/src/tests/JIT/Directed/StructABI/MisSizedStructs.cs index 3fb55a909960f6..5ba7f6a537a64b 100644 --- a/src/tests/JIT/Directed/StructABI/MisSizedStructs.cs +++ b/src/tests/JIT/Directed/StructABI/MisSizedStructs.cs @@ -60,6 +60,11 @@ public static int Main() return 109; } + if (ProblemWithOutOfBoundsLoads(out int result)) + { + return result; + } + return 100; } @@ -149,6 +154,89 @@ struct ForceStackUsage public fixed byte Bytes[40]; } + // Test that we do not load of bounds for split arguments on ARM. + // + static bool ProblemWithOutOfBoundsLoads(out int result) + { + result = 100; + + // TODO: enable for x64 once https://github.com/dotnet/runtime/issues/65937 has been fixed. + if (!OperatingSystem.IsLinux() || (RuntimeInformation.ProcessArchitecture == Architecture.X64)) + { + return false; + } + + const int PROT_NONE = 0x0; + const int PROT_READ = 0x1; + const int PROT_WRITE = 0x2; + const int MAP_PRIVATE = 0x02; + const int MAP_ANONYMOUS = 0x20; + const int PAGE_SIZE = 0x1000; + + byte* pages = (byte*)mmap(null, 2 * PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + + if (pages == (byte*)-1) + { + Console.WriteLine("Failed to allocate two pages, errno is {0}, giving up on the test", Marshal.GetLastSystemError()); + return false; + } + + if (mprotect(pages + PAGE_SIZE, PAGE_SIZE, PROT_NONE) != 0) + { + Console.WriteLine("Failed to protect the second page, errno is {0}, giving up on the test", Marshal.GetLastSystemError()); + munmap(pages, 2 * PAGE_SIZE); + return false; + } + + pages[PAGE_SIZE - 1] = ByteValue; + + if (CallForSplitStructWithSixteenBytes(0, *(StructWithSixteenBytes*)(pages + PAGE_SIZE - sizeof(StructWithSixteenBytes))) != ByteValue) + { + result = 200; + return true; + } + if (CallForSplitStructWithSeventeenBytes(0, *(StructWithSeventeenBytes*)(pages + PAGE_SIZE - sizeof(StructWithSeventeenBytes))) != ByteValue) + { + result = 201; + return true; + } + if (CallForSplitStructWithEighteenBytes(0, *(StructWithEighteenBytes*)(pages + PAGE_SIZE - sizeof(StructWithEighteenBytes))) != ByteValue) + { + result = 202; + return true; + } + if (CallForSplitStructWithNineteenBytes(0, *(StructWithNineteenBytes*)(pages + PAGE_SIZE - sizeof(StructWithNineteenBytes))) != ByteValue) + { + result = 203; + return true; + } + + munmap(pages, 2 * PAGE_SIZE); + + return false; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static byte CallForSplitStructWithSixteenBytes(long arg0, StructWithSixteenBytes splitArg) => splitArg.Bytes[15]; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static byte CallForSplitStructWithSeventeenBytes(long arg0, StructWithSeventeenBytes splitArg) => splitArg.Bytes[16]; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static byte CallForSplitStructWithEighteenBytes(long arg0, StructWithEighteenBytes splitArg) => splitArg.Bytes[17]; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static byte CallForSplitStructWithNineteenBytes(long arg0, StructWithNineteenBytes splitArg) => splitArg.Bytes[18]; + + [DllImport("libc")] + private static extern void* mmap(void* addr, nuint length, int prot, int flags, int fd, nuint offset); + + [DllImport("libc")] + private static extern int mprotect(void* addr, nuint len, int prot); + + [DllImport("libc")] + private static extern int munmap(void* addr, nuint length); + struct StructWithThreeBytes { public fixed byte Bytes[3]; @@ -189,6 +277,21 @@ struct StructWithFifteenBytes public fixed byte Bytes[15]; } + struct StructWithSixteenBytes + { + public fixed byte Bytes[16]; + } + + struct StructWithSeventeenBytes + { + public fixed byte Bytes[17]; + } + + struct StructWithEighteenBytes + { + public fixed byte Bytes[18]; + } + struct StructWithNineteenBytes { public fixed byte Bytes[19]; From a4ba00ddd002da797270626ed0bb8ecb718c620e Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 5 Jun 2022 00:54:39 +0300 Subject: [PATCH 2/3] Fix out-of-bounds loads in ARM/64's "genPutArgSplit" --- src/coreclr/jit/codegenarmarch.cpp | 61 ++++++++++++++++++------------ 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index f5de40823259e4..bbaba6051a0f33 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -1306,14 +1306,8 @@ void CodeGen::genPutArgSplit(GenTreePutArgSplit* treeNode) } else // addrNode is used { - assert(addrNode != nullptr); - // TODO-Cleanup: `Lowering::NewPutArg` marks only `LCL_VAR_ADDR` as contained nowadays, - // but we use `genConsumeAddress` as a precaution, use `genConsumeReg()` instead. - assert(!addrNode->isContained()); - // Generate code to load the address that we need into a register - genConsumeAddress(addrNode); - addrReg = addrNode->GetRegNum(); + addrReg = genConsumeReg(addrNode); // If addrReg equal to baseReg, we use the last target register as alternative baseReg. // Because the candidate mask for the internal baseReg does not include any of the target register, @@ -1327,21 +1321,40 @@ void CodeGen::genPutArgSplit(GenTreePutArgSplit* treeNode) ClassLayout* layout = source->AsObj()->GetLayout(); // Put on stack first - unsigned nextIndex = treeNode->gtNumRegs; - unsigned structOffset = nextIndex * TARGET_POINTER_SIZE; - int remainingSize = treeNode->GetStackByteSize(); + unsigned structOffset = treeNode->gtNumRegs * TARGET_POINTER_SIZE; + unsigned remainingSize = layout->GetSize() - structOffset; unsigned argOffsetOut = treeNode->getArgOffset(); - // remainingSize is always multiple of TARGET_POINTER_SIZE - assert(remainingSize % TARGET_POINTER_SIZE == 0); + assert((remainingSize > 0) && (roundUp(remainingSize, TARGET_POINTER_SIZE) == treeNode->GetStackByteSize())); while (remainingSize > 0) { - var_types type = layout->GetGCPtrType(nextIndex); + var_types type; + if (remainingSize >= TARGET_POINTER_SIZE) + { + type = layout->GetGCPtrType(structOffset / TARGET_POINTER_SIZE); + } + else if (remainingSize >= 4) + { + type = TYP_INT; + } + else if (remainingSize >= 2) + { + type = TYP_USHORT; + } + else + { + assert(remainingSize == 1); + type = TYP_UBYTE; + } + + emitAttr attr = emitActualTypeSize(type); + unsigned moveSize = genTypeSize(type); + instruction loadIns = ins_Load(type); if (varNode != nullptr) { - // Load from our varNumImp source - emit->emitIns_R_S(INS_ldr, emitTypeSize(type), baseReg, srcVarNum, structOffset); + // Load from our local source + emit->emitIns_R_S(loadIns, attr, baseReg, srcVarNum, structOffset); } else { @@ -1349,16 +1362,16 @@ void CodeGen::genPutArgSplit(GenTreePutArgSplit* treeNode) assert(baseReg != addrReg); // Load from our address expression source - emit->emitIns_R_R_I(INS_ldr, emitTypeSize(type), baseReg, addrReg, structOffset); + emit->emitIns_R_R_I(loadIns, attr, baseReg, addrReg, structOffset); } - // Emit str instruction to store the register into the outgoing argument area - emit->emitIns_S_R(INS_str, emitTypeSize(type), baseReg, varNumOut, argOffsetOut); - argOffsetOut += TARGET_POINTER_SIZE; // We stored 4-bytes of the struct - assert(argOffsetOut <= argOffsetMax); // We can't write beyond the outgoing arg area - remainingSize -= TARGET_POINTER_SIZE; // We loaded 4-bytes of the struct - structOffset += TARGET_POINTER_SIZE; - nextIndex += 1; + // Emit the instruction to store the register into the outgoing argument area + emit->emitIns_S_R(ins_Store(type), attr, baseReg, varNumOut, argOffsetOut); + argOffsetOut += moveSize; + assert(argOffsetOut <= argOffsetMax); + + remainingSize -= moveSize; + structOffset += moveSize; } // We set up the registers in order, so that we assign the last target register `baseReg` is no longer in use, @@ -1371,7 +1384,7 @@ void CodeGen::genPutArgSplit(GenTreePutArgSplit* treeNode) if (varNode != nullptr) { - // Load from our varNumImp source + // Load from our local source emit->emitIns_R_S(INS_ldr, emitTypeSize(type), targetReg, srcVarNum, structOffset); } else From 7513cfac350400fe430f1def080a080017fd10bc Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Tue, 7 Jun 2022 11:47:37 +0300 Subject: [PATCH 3/3] Split the test out --- .../JIT/Directed/StructABI/MisSizedStructs.cs | 103 --------------- .../StructABI/MisSizedStructs_ArmSplit.cs | 124 ++++++++++++++++++ .../StructABI/MisSizedStructs_ArmSplit.csproj | 13 ++ 3 files changed, 137 insertions(+), 103 deletions(-) create mode 100644 src/tests/JIT/Directed/StructABI/MisSizedStructs_ArmSplit.cs create mode 100644 src/tests/JIT/Directed/StructABI/MisSizedStructs_ArmSplit.csproj diff --git a/src/tests/JIT/Directed/StructABI/MisSizedStructs.cs b/src/tests/JIT/Directed/StructABI/MisSizedStructs.cs index 5ba7f6a537a64b..3fb55a909960f6 100644 --- a/src/tests/JIT/Directed/StructABI/MisSizedStructs.cs +++ b/src/tests/JIT/Directed/StructABI/MisSizedStructs.cs @@ -60,11 +60,6 @@ public static int Main() return 109; } - if (ProblemWithOutOfBoundsLoads(out int result)) - { - return result; - } - return 100; } @@ -154,89 +149,6 @@ struct ForceStackUsage public fixed byte Bytes[40]; } - // Test that we do not load of bounds for split arguments on ARM. - // - static bool ProblemWithOutOfBoundsLoads(out int result) - { - result = 100; - - // TODO: enable for x64 once https://github.com/dotnet/runtime/issues/65937 has been fixed. - if (!OperatingSystem.IsLinux() || (RuntimeInformation.ProcessArchitecture == Architecture.X64)) - { - return false; - } - - const int PROT_NONE = 0x0; - const int PROT_READ = 0x1; - const int PROT_WRITE = 0x2; - const int MAP_PRIVATE = 0x02; - const int MAP_ANONYMOUS = 0x20; - const int PAGE_SIZE = 0x1000; - - byte* pages = (byte*)mmap(null, 2 * PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); - - if (pages == (byte*)-1) - { - Console.WriteLine("Failed to allocate two pages, errno is {0}, giving up on the test", Marshal.GetLastSystemError()); - return false; - } - - if (mprotect(pages + PAGE_SIZE, PAGE_SIZE, PROT_NONE) != 0) - { - Console.WriteLine("Failed to protect the second page, errno is {0}, giving up on the test", Marshal.GetLastSystemError()); - munmap(pages, 2 * PAGE_SIZE); - return false; - } - - pages[PAGE_SIZE - 1] = ByteValue; - - if (CallForSplitStructWithSixteenBytes(0, *(StructWithSixteenBytes*)(pages + PAGE_SIZE - sizeof(StructWithSixteenBytes))) != ByteValue) - { - result = 200; - return true; - } - if (CallForSplitStructWithSeventeenBytes(0, *(StructWithSeventeenBytes*)(pages + PAGE_SIZE - sizeof(StructWithSeventeenBytes))) != ByteValue) - { - result = 201; - return true; - } - if (CallForSplitStructWithEighteenBytes(0, *(StructWithEighteenBytes*)(pages + PAGE_SIZE - sizeof(StructWithEighteenBytes))) != ByteValue) - { - result = 202; - return true; - } - if (CallForSplitStructWithNineteenBytes(0, *(StructWithNineteenBytes*)(pages + PAGE_SIZE - sizeof(StructWithNineteenBytes))) != ByteValue) - { - result = 203; - return true; - } - - munmap(pages, 2 * PAGE_SIZE); - - return false; - } - - [MethodImpl(MethodImplOptions.NoInlining)] - private static byte CallForSplitStructWithSixteenBytes(long arg0, StructWithSixteenBytes splitArg) => splitArg.Bytes[15]; - - [MethodImpl(MethodImplOptions.NoInlining)] - private static byte CallForSplitStructWithSeventeenBytes(long arg0, StructWithSeventeenBytes splitArg) => splitArg.Bytes[16]; - - [MethodImpl(MethodImplOptions.NoInlining)] - private static byte CallForSplitStructWithEighteenBytes(long arg0, StructWithEighteenBytes splitArg) => splitArg.Bytes[17]; - - [MethodImpl(MethodImplOptions.NoInlining)] - private static byte CallForSplitStructWithNineteenBytes(long arg0, StructWithNineteenBytes splitArg) => splitArg.Bytes[18]; - - [DllImport("libc")] - private static extern void* mmap(void* addr, nuint length, int prot, int flags, int fd, nuint offset); - - [DllImport("libc")] - private static extern int mprotect(void* addr, nuint len, int prot); - - [DllImport("libc")] - private static extern int munmap(void* addr, nuint length); - struct StructWithThreeBytes { public fixed byte Bytes[3]; @@ -277,21 +189,6 @@ struct StructWithFifteenBytes public fixed byte Bytes[15]; } - struct StructWithSixteenBytes - { - public fixed byte Bytes[16]; - } - - struct StructWithSeventeenBytes - { - public fixed byte Bytes[17]; - } - - struct StructWithEighteenBytes - { - public fixed byte Bytes[18]; - } - struct StructWithNineteenBytes { public fixed byte Bytes[19]; diff --git a/src/tests/JIT/Directed/StructABI/MisSizedStructs_ArmSplit.cs b/src/tests/JIT/Directed/StructABI/MisSizedStructs_ArmSplit.cs new file mode 100644 index 00000000000000..2f83aa0685b7da --- /dev/null +++ b/src/tests/JIT/Directed/StructABI/MisSizedStructs_ArmSplit.cs @@ -0,0 +1,124 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.InteropServices; +using System.Runtime.CompilerServices; + +public unsafe class MisSizedStructs_ArmSplit +{ + public const byte ByteValue = 0xC1; + + public static int Main() + { + if (ProblemWithOutOfBoundsLoads(out int result)) + { + return result; + } + + return 100; + } + + // Test that we do not load of bounds for split arguments on ARM. + // + static bool ProblemWithOutOfBoundsLoads(out int result) + { + result = 100; + + // TODO: enable for x64 once https://github.com/dotnet/runtime/issues/65937 has been fixed. + if (!OperatingSystem.IsLinux() || (RuntimeInformation.ProcessArchitecture == Architecture.X64)) + { + return false; + } + + const int PROT_NONE = 0x0; + const int PROT_READ = 0x1; + const int PROT_WRITE = 0x2; + const int MAP_PRIVATE = 0x02; + const int MAP_ANONYMOUS = 0x20; + const int PAGE_SIZE = 0x1000; + + byte* pages = (byte*)mmap(null, 2 * PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + + if (pages == (byte*)-1) + { + Console.WriteLine("Failed to allocate two pages, errno is {0}, giving up on the test", Marshal.GetLastSystemError()); + return false; + } + + if (mprotect(pages + PAGE_SIZE, PAGE_SIZE, PROT_NONE) != 0) + { + Console.WriteLine("Failed to protect the second page, errno is {0}, giving up on the test", Marshal.GetLastSystemError()); + munmap(pages, 2 * PAGE_SIZE); + return false; + } + + pages[PAGE_SIZE - 1] = ByteValue; + + if (CallForSplitStructWithSixteenBytes(0, *(StructWithSixteenBytes*)(pages + PAGE_SIZE - sizeof(StructWithSixteenBytes))) != ByteValue) + { + result = 200; + return true; + } + if (CallForSplitStructWithSeventeenBytes(0, *(StructWithSeventeenBytes*)(pages + PAGE_SIZE - sizeof(StructWithSeventeenBytes))) != ByteValue) + { + result = 201; + return true; + } + if (CallForSplitStructWithEighteenBytes(0, *(StructWithEighteenBytes*)(pages + PAGE_SIZE - sizeof(StructWithEighteenBytes))) != ByteValue) + { + result = 202; + return true; + } + if (CallForSplitStructWithNineteenBytes(0, *(StructWithNineteenBytes*)(pages + PAGE_SIZE - sizeof(StructWithNineteenBytes))) != ByteValue) + { + result = 203; + return true; + } + + munmap(pages, 2 * PAGE_SIZE); + + return false; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static byte CallForSplitStructWithSixteenBytes(long arg0, StructWithSixteenBytes splitArg) => splitArg.Bytes[15]; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static byte CallForSplitStructWithSeventeenBytes(long arg0, StructWithSeventeenBytes splitArg) => splitArg.Bytes[16]; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static byte CallForSplitStructWithEighteenBytes(long arg0, StructWithEighteenBytes splitArg) => splitArg.Bytes[17]; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static byte CallForSplitStructWithNineteenBytes(long arg0, StructWithNineteenBytes splitArg) => splitArg.Bytes[18]; + + [DllImport("libc")] + private static extern void* mmap(void* addr, nuint length, int prot, int flags, int fd, nuint offset); + + [DllImport("libc")] + private static extern int mprotect(void* addr, nuint len, int prot); + + [DllImport("libc")] + private static extern int munmap(void* addr, nuint length); + + struct StructWithSixteenBytes + { + public fixed byte Bytes[16]; + } + + struct StructWithSeventeenBytes + { + public fixed byte Bytes[17]; + } + + struct StructWithEighteenBytes + { + public fixed byte Bytes[18]; + } + + struct StructWithNineteenBytes + { + public fixed byte Bytes[19]; + } +} diff --git a/src/tests/JIT/Directed/StructABI/MisSizedStructs_ArmSplit.csproj b/src/tests/JIT/Directed/StructABI/MisSizedStructs_ArmSplit.csproj new file mode 100644 index 00000000000000..de62e29fa3e187 --- /dev/null +++ b/src/tests/JIT/Directed/StructABI/MisSizedStructs_ArmSplit.csproj @@ -0,0 +1,13 @@ + + + Exe + true + + + PdbOnly + True + + + + +