Skip to content

Commit 340e3fa

Browse files
Fix OP_CHECK_THIS to read 1 byte instead of 4/8 on x86/x64/LLVM. (#74762)
Current implementation of OP_CHECK_THIS on x86/x64 and LLVM does a memory read of at least 4 bytes. This creates an issue when the target is a managed pointer, since that could point to the interior of a type, meaning it can read pass the allocated memory causing a crash. Fix change the size of the read to one byte since the only reason doing the read is to validate that the reference, managed pointer is not NULL. Reading only one byte is also inline with how it is implemented on arm/arm64, and it will reduce potential unaligned reads on x86/x64. Full fix for, #74179. Co-authored-by: lateralusX <[email protected]>
1 parent e56d52d commit 340e3fa

File tree

5 files changed

+12
-7
lines changed

5 files changed

+12
-7
lines changed

src/mono/mono/arch/amd64/amd64-codegen.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1206,6 +1206,7 @@ typedef union {
12061206
#define amd64_alu_membase8_imm_size(inst,opc,basereg,disp,imm,size) do { amd64_codegen_pre(inst); amd64_emit_rex ((inst),(size),0,0,(basereg)); x86_alu_membase8_imm((inst),(opc),((basereg)&0x7),(disp),(imm)); amd64_codegen_post(inst); } while (0)
12071207
#define amd64_alu_mem_reg_size(inst,opc,mem,reg,size) do { amd64_codegen_pre(inst); amd64_emit_rex ((inst),(size),0,0,(reg)); x86_alu_mem_reg((inst),(opc),(mem),((reg)&0x7)); amd64_codegen_post(inst); } while (0)
12081208
#define amd64_alu_membase_reg_size(inst,opc,basereg,disp,reg,size) do { amd64_codegen_pre(inst); amd64_emit_rex ((inst),(size),(reg),0,(basereg)); x86_alu_membase_reg((inst),(opc),((basereg)&0x7),(disp),((reg)&0x7)); amd64_codegen_post(inst); } while (0)
1209+
#define amd64_alu_membase8_reg_size(inst,opc,basereg,disp,reg,size) do { amd64_codegen_pre(inst); amd64_emit_rex ((inst),(size),(reg),0,(basereg)); x86_alu_membase8_reg((inst),(opc),((basereg)&0x7),(disp),((reg)&0x7)); amd64_codegen_post(inst); } while (0)
12091210
//#define amd64_alu_reg_reg_size(inst,opc,dreg,reg,size) do { amd64_codegen_pre(inst); amd64_emit_rex ((inst),(size),(dreg),0,(reg)); x86_alu_reg_reg((inst),(opc),((dreg)&0x7),((reg)&0x7)); amd64_codegen_post(inst); } while (0)
12101211
#define amd64_alu_reg8_reg8_size(inst,opc,dreg,reg,is_dreg_h,is_reg_h,size) do { amd64_codegen_pre(inst); amd64_emit_rex ((inst),(size),(dreg),0,(reg)); x86_alu_reg8_reg8((inst),(opc),((dreg)&0x7),((reg)&0x7),(is_dreg_h),(is_reg_h)); amd64_codegen_post(inst); } while (0)
12111212
#define amd64_alu_reg_mem_size(inst,opc,reg,mem,size) do { amd64_codegen_pre(inst); amd64_emit_rex ((inst),(size),0,0,(reg)); x86_alu_reg_mem((inst),(opc),((reg)&0x7),(mem)); amd64_codegen_post(inst); } while (0)

src/mono/mono/arch/x86/x86-codegen.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -682,6 +682,13 @@ mono_x86_patch_inline (guchar* code, gpointer target)
682682
x86_membase_emit ((inst), (reg), (basereg), (disp)); \
683683
} while (0)
684684

685+
#define x86_alu_membase8_reg(inst,opc,basereg,disp,reg) \
686+
do { \
687+
x86_codegen_pre(&(inst), 1 + kMaxMembaseEmitPadding); \
688+
x86_byte (inst, (((unsigned char)(opc)) << 3)); \
689+
x86_membase_emit ((inst), (reg), (basereg), (disp)); \
690+
} while (0)
691+
685692
#define x86_alu_reg_reg(inst,opc,dreg,reg) \
686693
do { \
687694
x86_codegen_pre(&(inst), 2); \

src/mono/mono/mini/mini-amd64.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5511,7 +5511,7 @@ mono_arch_output_basic_block (MonoCompile *cfg, MonoBasicBlock *bb)
55115511
}
55125512
case OP_CHECK_THIS:
55135513
/* ensure ins->sreg1 is not NULL */
5514-
amd64_alu_membase_imm_size (code, X86_CMP, ins->sreg1, 0, 0, 4);
5514+
amd64_alu_membase8_reg_size (code, X86_CMP, ins->sreg1, 0, ins->sreg1, 1);
55155515
break;
55165516
case OP_ARGLIST: {
55175517
amd64_lea_membase (code, AMD64_R11, cfg->frame_reg, cfg->sig_cookie);

src/mono/mono/mini/mini-llvm.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6856,7 +6856,7 @@ MONO_RESTORE_WARNING
68566856
}
68576857

68586858
case OP_CHECK_THIS:
6859-
LLVMBuildLoad2 (builder, IntPtrType (), convert (ctx, lhs, pointer_type (IntPtrType ())), "");
6859+
LLVMBuildLoad2 (builder, LLVMInt8Type (), convert (ctx, lhs, pointer_type (LLVMInt8Type ())), "");
68606860
break;
68616861
case OP_OUTARG_VTRETADDR:
68626862
break;

src/mono/mono/mini/mini-x86.c

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3192,11 +3192,8 @@ mono_arch_output_basic_block (MonoCompile *cfg, MonoBasicBlock *bb)
31923192
break;
31933193
}
31943194
case OP_CHECK_THIS:
3195-
/* ensure ins->sreg1 is not NULL
3196-
* note that cmp DWORD PTR [eax], eax is one byte shorter than
3197-
* cmp DWORD PTR [eax], 0
3198-
*/
3199-
x86_alu_membase_reg (code, X86_CMP, ins->sreg1, 0, ins->sreg1);
3195+
/* ensure ins->sreg1 is not NULL */
3196+
x86_alu_membase8_reg (code, X86_CMP, ins->sreg1, 0, ins->sreg1);
32003197
break;
32013198
case OP_ARGLIST: {
32023199
int hreg = ins->sreg1 == X86_EAX? X86_ECX: X86_EAX;

0 commit comments

Comments
 (0)