Skip to content
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.

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

Fix clang alloc-dealloc-mismatch #318

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

fboemer
Copy link
Contributor

@fboemer fboemer commented Apr 12, 2021

When compiling with clang-10, the recent aligned malloc changes seem to have a mismatch, using malloc / delete[], as evidenced by the AddressSanitizer (Asan) output below. This PR fixes the issue.

This PR also an option to enable Asan. Now, the tests pass with -DSEAL_USE_ADDRESS_SANITIZER=ON. Note, Asan adds a significant performance penalty, so any benchmarking results should be run with -DSEAL_USE_ADDRESS_SANITIZER=OFF. I'm happy to remove this option, just figured I'd point it out as a useful tool in case you'd like to add it to SEAL.

==437185==ERROR: AddressSanitizer: alloc-dealloc-mismatch (malloc vs operator delete []) on 0x7f6559275800
    #0 0x52cffd in operator delete[](void*) (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x52cffd)
    #1 0xef326d in seal::util::MemoryPoolHeadMT::~MemoryPoolHeadMT() (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0xef326d)
    #2 0xef33c8 in seal::util::MemoryPoolHeadMT::~MemoryPoolHeadMT() (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0xef33c8)
    #3 0xef4aa1 in seal::util::MemoryPoolMT::~MemoryPoolMT() (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0xef4aa1)
    #4 0xed661b in seal::Serialization::Save(std::function<void (std::ostream&)>, long, std::ostream&, seal::compr_mode_type, bool) (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0xed661b)
    #5 0x53e37f in sealtest::CiphertextTest_SaveLoadCiphertext_Test::TestBody() (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x53e37f)
    #6 0x108ff01 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x108ff01)
    #7 0x101cd5b in testing::Test::Run() (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x101cd5b)
    #8 0x1020952 in testing::TestInfo::Run() (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x1020952)
    #9 0x1022200 in testing::TestSuite::Run() (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x1022200)
    #10 0x105058c in testing::internal::UnitTestImpl::RunAllTests() (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x105058c)
    #11 0x10925c7 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x10925c7)
    #12 0x104f5a6 in testing::UnitTest::Run() (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x104f5a6)
    #13 0xac091a in main (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0xac091a)
    #14 0x7f655bfb50b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #15 0x4847ed in _start (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x4847ed)

0x7f6559275800 is located 0 bytes inside of 262144-byte region [0x7f6559275800,0x7f65592b5800)
allocated by thread T0 here:
    #0 0x4fd852 in aligned_alloc (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x4fd852)
    #1 0xef2d4d in seal::util::MemoryPoolHeadMT::MemoryPoolHeadMT(unsigned long, bool) (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0xef2d4d)
    #2 0xef50c5 in seal::util::MemoryPoolMT::get_for_byte_count(unsigned long) (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0xef50c5)
    #3 0x9955df in seal::DynArray<std::byte>::resize(unsigned long, bool) (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x9955df)
    #4 0x994c17 in seal::DynArray<std::byte>::DynArray(unsigned long, seal::MemoryPoolHandle) (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x994c17)

SUMMARY: AddressSanitizer: alloc-dealloc-mismatch (/home/fboemer/repos/XXXX/intel-seal/build/bin/sealtest+0x52cffd) in operator delete[](void*)
==437185==HINT: if you don't care about these errors you may set ASAN_OPTIONS=alloc_dealloc_mismatch=0
==437185==ABORTING

@WeiDaiWD
Copy link
Contributor

I do care about alloc-dealloc-mismatch. :) It seems to be caused by pointer.h where delete was used regardless of SEAL_USE_ALIGNED_ALLOC. I need to verify that thought. Also Visual Studio has an address sanitizer. So I might force SEAL_USE_ADDRESS_SANITIZER to OFF. No need to change anything yet. If I make changes, I'll create a PR to your branch. Thanks again!

@fboemer
Copy link
Contributor Author

fboemer commented Apr 12, 2021

Sure thing; on my side, the error came from clang not defining SEAL_FREE in clang.h, resulting in using the default delete[] ptr rather than std::free.

@WeiDaiWD
Copy link
Contributor

Right, clang.h is a mistake. It requires a new patch version. I'm trying to include the following changes in the new patch:

  • pointer.h can also cause this kind of warnings, which is separate from recent aligned allocation changes. This needs to be prevented.
  • Sanitizers can be enabled by default in Debug mode. While in Release mode, if sanitizers are enabled, there should a warning message at CMake's configuration time.
  • Leak sanitizer is not yet available in Visual Studio. Need to make -fsanitize=leak depend on NOT MSVC.

@WeiDaiWD
Copy link
Contributor

@fboemer Do you see the need to enable sanitizers in release build? I'm thinking about enabling it by default in debug and not providing an option.

@fboemer
Copy link
Contributor Author

fboemer commented Apr 13, 2021

@WeiDaiWD , sanitizers should definitely should not be in Release build, since it adds a significant performance penalty. Default in debug sounds reasonable, given SEAL_DEBUG is already a performance penalty.

@WeiDaiWD WeiDaiWD merged commit c289ac4 into microsoft:contrib Apr 13, 2021
@WeiDaiWD
Copy link
Contributor

I'll add more edits before release 3.6.4 soon. Thanks a lot!

@fboemer fboemer deleted the fboemer/fix-free branch November 3, 2021 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants