fixed VirtualMemoryLogging::logRecords overflow with negative indexing #27907
Conversation
tqinli
force-pushed
the
fix_vmlogging_underflow
branch
2 times, most recently
from
5938f5e to
8281f09
Compare
tqinli
changed the title
src/pal/src/map/virtual.cpp
Outdated
| @@ -137,7 +137,9 @@ namespace VirtualMemoryLogging | |||
| IN LPVOID returnedAddress, | |||
| IN BOOL result) | |||
| { | |||
| LONG i = InterlockedIncrement(&recordNumber) - 1; | |||
| ULONG i = InterlockedIncrement((LONG *)&recordNumber) - 1; | |||
There was a problem hiding this comment.
Can you please put cast to ULONG before the InterlockedIncrement too? Clang is probably ok with it, but some people spent quite an effort trying to keep the codebase buildable with GCC too and it is picky on these.
when VirtualMemoryLogging::recordNumber increments from LONG_MAX, it became negative number, and the result of i % MaxRecords became a number from -127 to 0. When that happens we will ovewrite CRITICAL_SECTION virtual_critsec which are stored in bss right before logRecords with garbage data. Then most likely the process will have a GC hang with one or more GC threads stuck trying to enter or leave critical section. The fix is to ensure ULONG value are passed to modulo operation.
tqinli
force-pushed
the
fix_vmlogging_underflow
branch
from
8281f09 to
c7fee2b
Compare
janvorli
added
the
* NO MERGE *
|
@tqinli the PR will need to be ported and merged into the new runtime repo once it is open (should happen soon). This repo is closed for new PRs. |
|
@janvorli thanks for letting me know. yeah, I will port and re-send once the new repo is set up. thanks for your help! |
|
@tqinli the dotnet/runtime repo is open now, could you please port your change to it? |
|
thanks for letting me know. Yeah I will work on this today and send the new one |
|
Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime. |
when VirtualMemoryLogging::recordNumber increments from LONG_MAX,
it became negative number, and the result of i % MaxRecords became
a number from -127 to 0.
When that happens we will ovewrite CRITICAL_SECTION virtual_critsec
which are stored in bss right before logRecords with garbage data.
Then most likely the process will have a GC hang with one or more
GC threads stuck trying to enter or leave critical section.
The fix is to ensure ULONG value are passed to modulo operation.