fix: missing destructor call in Pistache::Queue

[tyler92]

The Pistache::Queue uses placement new which means destructor must be called explicitly but there is one place in the implementation where it's missing. As a result, we might have memory leaks but I got it only with Clang 18.1.8:

==9==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x55cd164e111d in operator new(unsigned long) /src/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:86:3
    #1 0x55cd16686bf3 in std::__1::shared_ptr<Pistache::Http::Experimental::Connection>::shared_ptr[abi:ne180100]<Pistache::Http::Experimental::Connection, void>(Pistache::Http::Experimental::Connection*) /usr/local/bin/../include/c++/v1/__memory/shared_ptr.h:446:16
    #2 0x55cd1668688e in construct<std::__1::shared_ptr<Pistache::Http::Experimental::Connection>, Pistache::Http::Experimental::Connection *> /usr/local/bin/../include/c++/v1/__memory/allocator.h:173:24
    #3 0x55cd1668688e in construct<std::__1::shared_ptr<Pistache::Http::Experimental::Connection>, Pistache::Http::Experimental::Connection *, void> /usr/local/bin/../include/c++/v1/__memory/allocator_traits.h:296:9
    #4 0x55cd1668688e in std::__1::shared_ptr<Pistache::Http::Experimental::Connection>* std::__1::vector<std::__1::shared_ptr<Pistache::Http::Experimental::Connection>, std::__1::allocator<std::__1::shared_ptr<Pistache::Http::Experimental::Connection>>>::__emplace_back_slow_path<Pistache::Http::Experimental::Connection*>(Pistache::Http::Experimental::Connection*&&) /usr/local/bin/../include/c++/v1/vector:1491:3
    #5 0x55cd16672b70 in emplace_back<Pistache::Http::Experimental::Connection *> /usr/local/bin/../include/c++/v1/vector:1511:13
    #6 0x55cd16672b70 in Pistache::Http::Experimental::ConnectionPool::pickConnection(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /src/pistache/src/client/client.cc:1020:33
    #7 0x55cd1667477e in Pistache::Http::Experimental::Client::doRequest(Pistache::Http::Request) /src/pistache/src/client/client.cc:1329:26
    #8 0x55cd166741e6 in Pistache::Http::Experimental::RequestBuilder::send() /src/pistache/src/client/client.cc:1172:25
    #9 0x55cd164e3839 in sendPingRequest(Pistache::Address const&) /src/pistache/build/../tests/fuzzers/fuzz_server.cpp:38:96
    #10 0x55cd164e65ad in LLVMFuzzerTestOneInput /src/pistache/build/../tests/fuzzers/fuzz_server.cpp:86:5
    #11 0x55cd16397f80 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
    #12 0x55cd163831f5 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
    #13 0x55cd16388c8f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
    #14 0x55cd163b3f32 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #15 0x7f55f11d8082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)

I also modified the unit test and added one more. Tests output without the fix:

[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 2 tests from QueueTest
[ RUN      ] QueueTest.destructor_test
[       OK ] QueueTest.destructor_test (0 ms)
[ RUN      ] QueueTest.push_pop
pistache/tests/mailbox_test.cc:80: Failure
Expected equality of these values:
  Data::num_instances
    Which is: 5
  0
[  FAILED  ] QueueTest.push_pop (0 ms)
[----------] 2 tests from QueueTest (0 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test suite ran. (1 ms total)
[  PASSED  ] 1 test.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] QueueTest.push_pop

 1 FAILED TEST

=================================================================
==1573154==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1010 byte(s) in 10 object(s) allocated from:
    #0 0x7f2cf48341e7 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x7f2cf4359cbb in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct(unsigned long, char) (/lib/x86_64-linux-gnu/libstdc++.so.6+0x158cbb)

SUMMARY: AddressSanitizer: 1010 byte(s) leaked in 10 allocation(s).

[dgreatwood]

Hi @tyler92 -

Can you explain a bit more about how this is deallocation is supposed to work?

Of course, directly calling the destructor (as per this change) doesn't of itself free the object being destroyed.

Then again, since res->storage is just a member of Entry, we wouldn't need to deallocate ("operator delete") that.

In popSafe, "entry" was already being deallocated (because we create a std::unique_ptr to it). In pop(), on the other hand, I guess it is up to the caller somehow to deallocate the returned entry.

At the change itself, calling the destructor for the defunct "storage" seems right. Given we've done std::move, though, I wouldn't expect that destructor calling to deallocate any memory (e.g. if there's a pointer to a string in storage, presumably the std::move would already have shifted that pointer elsewhere, leaving nothing for storage to take care of?). I think.

Is there in fact a deallocate that would happen now that wasn't happening before? Or is it simply a matter of ensuring the destructor be called just in case the destructor is performing some needed cleanup?

[tyler92]

Hi @dgreatwood

Given we've done std::move, though, I wouldn't expect that destructor calling to deallocate any memory (e.g. if there's a pointer to a string in storage, presumably the std::move would already have shifted that pointer elsewhere, leaving nothing for storage to take care of?).

Yes, I totally agree, and it works this way for GCC and some versions of Clang. But for some reason, ASAN with Clang 18.1.8 gives this memory leak warning. I'm not sure if it's a false positive or a real issue that's reproduced only with a specific compiler (and maybe even compile flags). I can go deeply and investigate it, but we need to call the destructor anyway, even if it doesn't lead to a memory leak. So...

Is there in fact a deallocate that would happen now that wasn't happening before? Or is it simply a matter of ensuring the destructor be called just in case the destructor is performing some needed cleanup?

... I think it's mostly for correctness, just to avoid potential issues

[tyler92]

And I intentionally made a "wrong" move constructor in unit tests that doesn't clean up the moving object completely. It leads to a memory leak in case of a missing destructor call. Maybe this is not a real case, so please let me know if you think we need to remove it. We have EXPECT_EQ(Data::num_instances, 0); assertion anyway.

[dgreatwood]

That makes sense. The change looks good to me. 👍

…

On Wed, Oct 16, 2024 at 1:02 PM tyler92 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ On include/pistache/mailbox.h <#1255 (comment)>: And I intentionally made a "wrong" move constructor in unit tests that doesn't clean up the moving object completely. It leads to a memory leak in case of a missing destructor call. Maybe this is not a real case, so please let me know if you think we need to remove it. We have EXPECT_EQ(Data::num_instances, 0); assertion anyway. — Reply to this email directly, view it on GitHub <#1255 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFMA236QCXMJLERK6HOKSDZ33A5XAVCNFSM6AAAAABP74DOTOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNZTGU2TKMRXGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- NOTICE: This email and its attachments may contain privileged and confidential information, only for the viewing and use of the intended recipient. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, acting upon, or use of the information contained in this email and its attachments is strictly prohibited and that this email and its attachments must be immediately returned to the sender and deleted from your system. If you received this email erroneously, please notify the sender immediately.  Xage Security, Inc. and its affiliates will never request personal information (e.g., passwords, Social Security numbers) via email.  Report suspicious emails to ***@***.*** ***@***.***>

[kiplingw]

@Tachi107, LGTM. Let us know if you're fine with it.

[kiplingw]

@tyler92, can you bump the patch version? You've changed the internal implementation without changing any public interfaces, so that should normally be done.

[tyler92]

Done, 0.4.10.20241017