Skip to content

Fix interpreter NULL_CHECK for pointers #122245

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.

Sign up for GitHub

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

Open
janvorli wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −17
Open

Fix interpreter NULL_CHECK for pointers #122245

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 18 additions & 17 deletions src/coreclr/vm/interpexec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@ static void InterpBreakpoint()
#define LOCAL_VAR_ADDR(offset,type) ((type*)(stack + (offset)))
#define LOCAL_VAR(offset,type) (*LOCAL_VAR_ADDR(offset, type))
#define NULL_CHECK(o) do { if ((o) == NULL) { COMPlusThrow(kNullReferenceException); } } while (0)
#define NULL_CHECK_PTR(o) do { if ((TADDR)(o) < NULL_AREA_SIZE) { COMPlusThrow(kNullReferenceException); } } while (0)
Copy link
Member

@am11 am11 Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas, can we align naot NULL_AREA_SIZE with coreclr for Unix:

runtime/src/coreclr/nativeaot/Runtime/Pal.h

Line 110 in 91509b1

#define NULL_AREA_SIZE (4*1024)

runtime/src/coreclr/vm/excep.h

Line 53 in 91509b1

#define NULL_AREA_SIZE GetOsPageSize()
it was added in dotnet/corert@99841b9.

Copy link
Member

@jkotas jkotas Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxUncheckedOffsetForNullObject used during the code generation must be in sync with NULL_AREA_SIZE used at runtime (there is 2x factor between these two constants).

For correctness, it is ok for maxUncheckedOffsetForNullObject to be less than NULL_AREA_SIZE, but it cannot be more than NULL_AREA_SIZE.

For security, it is best for NULL_AREA_SIZE to be as tight as possible to avoid mischaracterizing dirty segfauls as NullReferenceExceptions unnecessarily.

If we were to unify NULL_AREA_SIZE between NAOT and JIT, it would not be as tight as possible in all situations, and we would be leaving a bit of security on the table.



static OBJECTREF CreateMultiDimArray(MethodTable* arrayClass, int8_t* stack, int32_t dimsOffset, int numArgs)
Expand Down Expand Up @@ -2121,7 +2122,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
#define LDIND(dtype, ftype) \
do { \
char *src = LOCAL_VAR(ip[2], char*); \
NULL_CHECK(src); \
NULL_CHECK_PTR(src); \
LOCAL_VAR(ip[1], dtype) = *(ftype*)(src + ip[3]); \
ip += 4; \
} while (0)
Expand Down Expand Up @@ -2154,7 +2155,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
case INTOP_LDIND_VT:
{
char *src = LOCAL_VAR(ip[2], char*);
NULL_CHECK(src);
NULL_CHECK_PTR(src);
memcpy(LOCAL_VAR_ADDR(ip[1], void), (char*)src + ip[3], ip[4]);
ip += 5;
break;
Expand All @@ -2164,7 +2165,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
do \
{ \
char *dst = LOCAL_VAR(ip[1], char*); \
NULL_CHECK(dst); \
NULL_CHECK_PTR(dst); \
*(ftype*)(dst + ip[3]) = (ftype)(LOCAL_VAR(ip[2], dtype)); \
ip += 4; \
} while (0)
Expand Down Expand Up @@ -2198,15 +2199,15 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
{
char *dst = LOCAL_VAR(ip[1], char*);
OBJECTREF storeObj = LOCAL_VAR(ip[2], OBJECTREF);
NULL_CHECK(dst);
NULL_CHECK_PTR(dst);
SetObjectReferenceUnchecked((OBJECTREF*)(dst + ip[3]), storeObj);
ip += 4;
break;
}
case INTOP_STIND_VT_NOREF:
{
char *dest = LOCAL_VAR(ip[1], char*);
NULL_CHECK(dest);
NULL_CHECK_PTR(dest);
memcpyNoGCRefs(dest + ip[3], LOCAL_VAR_ADDR(ip[2], void), ip[4]);
ip += 5;
break;
Expand All @@ -2215,15 +2216,15 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
{
MethodTable *pMT = (MethodTable*)pMethod->pDataItems[ip[4]];
char *dest = LOCAL_VAR(ip[1], char*);
NULL_CHECK(dest);
NULL_CHECK_PTR(dest);
CopyValueClassUnchecked(dest + ip[3], LOCAL_VAR_ADDR(ip[2], void), pMT);
ip += 5;
break;
}
case INTOP_LDFLDA:
{
char *src = LOCAL_VAR(ip[2], char*);
NULL_CHECK(src);
NULL_CHECK_PTR(src);
LOCAL_VAR(ip[1], char*) = src + ip[3];
ip += 4;
break;
Expand Down Expand Up @@ -3022,7 +3023,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
case INTOP_ZEROBLK_IMM:
{
void* dst = LOCAL_VAR(ip[1], void*);
NULL_CHECK(dst);
NULL_CHECK_PTR(dst);
memset(dst, 0, ip[2]);
ip += 3;
break;
Expand Down Expand Up @@ -3140,7 +3141,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
MethodTable *pMT = (MethodTable*)pMethod->pDataItems[ip[3]];
void *dest = LOCAL_VAR_ADDR(ip[1], void);
void *src = LOCAL_VAR(ip[2], void*);
NULL_CHECK(dest);
NULL_CHECK_PTR(dest);
CopyValueClassUnchecked(dest, src, pMT);

ip += 4;
Expand Down Expand Up @@ -3185,7 +3186,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
MethodTable *pMT = (MethodTable*)DoGenericLookup(LOCAL_VAR(ip[2], void*), pLookup);
void *dest = LOCAL_VAR_ADDR(ip[1], void);
void *src = LOCAL_VAR(ip[3], void*);
NULL_CHECK(dest);
NULL_CHECK_PTR(dest);
CopyValueClassUnchecked(dest, src, pMT->IsNullable() ? pMT->GetInstantiation()[0].AsMethodTable() : pMT);

ip += 5;
Expand Down Expand Up @@ -3547,7 +3548,7 @@ do { \
case INTOP_COMPARE_EXCHANGE_U1:
{
uint8_t* dst = LOCAL_VAR(ip[2], uint8_t*);
NULL_CHECK(dst);
NULL_CHECK_PTR(dst);
uint8_t newValue = LOCAL_VAR(ip[3], uint8_t);
uint8_t comparand = LOCAL_VAR(ip[4], uint8_t);
// Since our Interlocked API doesn't support 8-bit exchange, we implement this via a loop and CompareExchange
Expand Down Expand Up @@ -3616,7 +3617,7 @@ do { \
do \
{ \
type* dst = (type*)LOCAL_VAR(ip[2], void*); \
NULL_CHECK(dst); \
NULL_CHECK_PTR(dst); \
type newValue = LOCAL_VAR(ip[3], type); \
type comparand = LOCAL_VAR(ip[4], type); \
type old = InterlockedCompareExchangeT(dst, newValue, comparand); \
Expand All @@ -3640,7 +3641,7 @@ do \
do \
{ \
type* dst = LOCAL_VAR(ip[2], type*); \
NULL_CHECK(dst); \
NULL_CHECK_PTR(dst); \
type newValue = LOCAL_VAR(ip[3], type); \
type old = InterlockedExchangeT(dst, newValue); \
LOCAL_VAR(ip[1], type) = old; \
Expand All @@ -3650,7 +3651,7 @@ do \
case INTOP_EXCHANGE_U1:
{
uint8_t* dst = LOCAL_VAR(ip[2], uint8_t*);
NULL_CHECK(dst);
NULL_CHECK_PTR(dst);
uint8_t newValue = LOCAL_VAR(ip[3], uint8_t);
// Since our Interlocked API doesn't support 8-bit exchange, we implement this via a loop and CompareExchange
// This is not optimal, but 8-bit exchange is not a common operation.
Expand All @@ -3674,7 +3675,7 @@ do \
case INTOP_EXCHANGE_U2:
{
uint16_t* dst = LOCAL_VAR(ip[2], uint16_t*);
NULL_CHECK(dst);
NULL_CHECK_PTR(dst);
uint16_t newValue = LOCAL_VAR(ip[3], uint16_t);
// Since our Interlocked API doesn't support 16-bit exchange, we implement this via a loop and CompareExchange
// This is not optimal, but 16-bit exchange is not a common operation.
Expand Down Expand Up @@ -3716,7 +3717,7 @@ do \
case INTOP_EXCHANGEADD_I4:
{
LONG* dst = LOCAL_VAR(ip[2], LONG*);
NULL_CHECK(dst);
NULL_CHECK_PTR(dst);
LONG newValue = LOCAL_VAR(ip[3], LONG);
LONG old = InterlockedExchangeAdd(dst, newValue);
LOCAL_VAR(ip[1], LONG) = old;
Expand All @@ -3727,7 +3728,7 @@ do \
case INTOP_EXCHANGEADD_I8:
{
LONG64* dst = LOCAL_VAR(ip[2], LONG64*);
NULL_CHECK(dst);
NULL_CHECK_PTR(dst);
LONG64 newValue = LOCAL_VAR(ip[3], LONG64);
LONG64 old = InterlockedExchangeAdd64(dst, newValue);
LOCAL_VAR(ip[1], LONG64) = old;
Expand Down
Loading