From 4dfdb8f6bccfdace4661a2cc6f4a31d4cba64fb0 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Fri, 28 Aug 2020 15:10:56 -0700 Subject: [PATCH] Fix a race condition in GCStress Consider the case of a partially interruptible function where we've replaced each call site with INTERRUPT_INSTR_CALL or INTERRUPT_INSTR_CALL_32. Even though the call site itself isn't a safe point, we put a GCStress instruction there so we can capture the call target at GC stress time, then set a specific GCStress instruction for the next instruction that will indicate how to GC protect any GC refs that are returned from the call. Multiple threads can enter `DoGcStress` at the same time. They all get just past the race check `if (!IsGcCoverageInterruptInstruction(instrPtr)) return;`. One goes ahead, and reads the instruction code for the address at which we're doing a GCStress. We set `atCall` to `true` if this matches one of our known GCStress instructions for partially-interruptible call sites. Later, if `atCall` is `true`, we write the original call instruction back to the code stream, write the distinguished next instruction GCStress instruction, and continue. If the other threads then read at the instruction address, they might read the actual call opcode, not the distinguished GCStress breakpoint instruction code. Then, they will set `atCall` to `false`. This will indicate that we are in a fully-interruptible location, or that no special call-site behavior is required. We go on, force a GC, and eventually get to the assert in `EECodeManager::EnumGcRefs` that we are at a GC safe point. The solution is to simply move the `if (!IsGcCoverageInterruptInstruction(instrPtr))` check below the read of `*instrPtr` to set `atCall`. Re-enable the GitHub_27924 test. Fixes #36681 --- src/coreclr/src/vm/gccover.cpp | 92 +++++++++++++++++++++----------- src/coreclr/tests/issues.targets | 3 -- 2 files changed, 60 insertions(+), 35 deletions(-) diff --git a/src/coreclr/src/vm/gccover.cpp b/src/coreclr/src/vm/gccover.cpp index cbe3873f364f40..3ad4f9203d64a1 100644 --- a/src/coreclr/src/vm/gccover.cpp +++ b/src/coreclr/src/vm/gccover.cpp @@ -744,23 +744,37 @@ void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID //Determine if instruction before the safe point is call using immediate (BLX Imm) or call by register (BLX Rm) BOOL instructionIsACallThroughRegister = FALSE; BOOL instructionIsACallThroughImmediate = FALSE; + #if defined(TARGET_ARM) + // POSSIBLE BUG: Note that we are looking backwards by 2 or 4 bytes, looking for particular call instruction encodings. + // However, we don't know if the previous instruction is 2 bytes or 4 bytes. Looking back 2 bytes could be looking into + // the middle of a 4-byte instruction. The only safe way to do this is by walking forward from the first instruction of + // the function. + // call by register instruction is two bytes (BL Reg T1 encoding) WORD instr = *((WORD*)savedInstrPtr - 1); - instr = instr & 0xff87; - if((instr ^ 0x4780) == 0) + if ((instr ^ 0x4780) == 0) + { // It is call by register instructionIsACallThroughRegister = TRUE; + } + else + { + // call using immediate instructions are 4 bytes (BL