Fix interpreter NULL_CHECK for pointers #122245
Conversation
The NULL_CHECK was checking strictly for NULL, but in some cases, we should be checking for values that are close to null. For example, some interop tests fail because various LDIND variants get pointer that is e.g. 2 or other low value.
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
There was a problem hiding this comment.
Pull request overview
This PR fixes the interpreter's null pointer checking to handle low pointer values (near-null pointers) correctly. The interpreter was checking strictly for NULL (0), but interop tests were failing when operations like LDIND received pointers with small values like 2, which should also be treated as null references.
Key Changes:
- Introduces
NULL_CHECK_PTRmacro that treats addresses belowNULL_AREA_SIZE(64KB on Windows, page size on Unix) as null references - Replaces
NULL_CHECKwithNULL_CHECK_PTRfor all pointer dereferencing operations in the interpreter - Aligns interpreter null checking behavior with the rest of the runtime
| #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) |
There was a problem hiding this comment.
@jkotas, can we align naot NULL_AREA_SIZE with coreclr for Unix:
runtime/src/coreclr/nativeaot/Runtime/Pal.h
Line 110 in 91509b1
runtime/src/coreclr/vm/excep.h
Line 53 in 91509b1
There was a problem hiding this comment.
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.
Are these tests valid, or are they just testing an unspecified behavior that the JIT happens to have? We do not need to have bug-for-bug compatibility for unspecified behavior. The check introduced in this PR is a bit more expensive than a simple null check. We do not want to be regressing interpreter executor perf unnecessarily. |
@jkotas A test failing without this fix was for example: |
|
This test does dirty write. The behavior for dirty write is unspecified - notice that the test is disabled on Mono for the same reason already. I think we should rather disable the test with interpreter. I expect that we will also want to disable the test on wasm since we are not going to simulate bug-for-bug behavior for dirty writes on wasm either. |
|
Ok. let me check if there are any meaningful tests failing due to this and disable the tests instead if there are none. |
|
@jkotas all of the failures are in the variant of this same test for various data width. So I am going to close this PR and create a PR for disabling those tests for the interpreter instead. |
5 participants
The NULL_CHECK was checking strictly for NULL, but in some cases, we should be checking for values that are close to null. For example, some interop tests fail because various LDIND variants get pointer that is e.g. 2 or other low value.