StanfordLegion / legion

The Legion Parallel Programming System
https://legion.stanford.edu
Apache License 2.0
689 stars 144 forks source link

Realm: `Realm::Logger`: `malloc()` allocates, `delete` deletes. #1748

Closed Jacobfaib closed 2 months ago

Jacobfaib commented 2 months ago
==21438==ERROR: AddressSanitizer: alloc-dealloc-mismatch (malloc vs operator delete) on 0x00010ed16f30
    #0 0x107955b8c in wrap__ZdlPv (/Library/Developer/CommandLineTools/usr/lib/clang/15.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:arm64e+0x61b8c)
    #1 0x13830f6e0 in Realm::DelayedMessage::operator delete(void*) legion/runtime/realm/logging.cc:499:7
    ...

0x00010ed16f30 is located 0 bytes inside of 74-byte region [0x00010ed16f30,0x00010ed16f7a)
allocated by thread T0 here:
    #0 0x107947124 in wrap_malloc (/Library/Developer/CommandLineTools/usr/lib/clang/15.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:arm64e+0x53124)
    #1 0x13830fba4 in Realm::Logger::log_msg(Realm::Logger::LoggingLevel, char const*, unsigned long) legion/runtime/realm/logging.cc:559:19
    #2 0x1351f3478 in Realm::LoggerMessage::~LoggerMessage() legion/runtime/realm/logging.inl:590:15
muraj commented 2 months ago

Yeah, this is bad. Apparently log_msg() malloc's some inline buffer with the object, then we do a placement new on it for delayed messages (messages that we want to buffer until after we're done configuring). Later we delete the object, which then deletes the pointer without a size. These are potentially two different allocators that can lead to memory corruption. An easy way to work around this is to replace malloc with ::operator new(sz), since we'll be calling ::operator delete(p) later anyway -- at least then it'll be consistent.

Jacobfaib commented 2 months ago

Apparently log_msg() malloc's some inline buffer with the object, then we do a placement new on it for delayed messages (messages that we want to buffer until after we're done configuring).

Yeah lol I saw this too. I have no idea why a simple

char *message;

member in the allocated struct was insufficient...

muraj commented 2 months ago

Yeah, I don't either, but I'd rather not change it for fear of regressing some corner performance case or something. Using a consistent allocator should do the trick.

muraj commented 2 months ago

Should be resolved in latest master.