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

oatpp::base::memory::MemoryPool::freeByEntryHeader()]: Invalid EntryHeader #308

Closed
pcapdump opened this issue Sep 24, 2020 · 20 comments
Closed

Comments

@pcapdump
Copy link
Contributor

pcapdump commented Sep 24, 2020

Hi!

I observe this exception when I'm stopping the application with OATPP:

terminate called after throwing an instance of 'std::runtime_error'
  what():  [oatpp::base::memory::MemoryPool::freeByEntryHeader()]: Invalid EntryHeader

I was able to get some more logs of it:

2020-09-16T13:31:24.530436Z DBG 322058:src/log/oatpp_logger.hpp:20 [oatpp::base::memory::MemoryPool::freeByEntryHeader()]:Error. Invalid EntryHeader. Expected poolId=9803840, entry poolId=-1610486112

Backtrace:

(gdb) bt
#0  0x00007f775e5e370f in raise () from /lib64/libc.so.6
#1  0x00007f775e5cdb25 in abort () from /lib64/libc.so.6
#2  0x00007f770a4fd06b in __gnu_cxx::__verbose_terminate_handler() [clone .cold.1] () from /lib64/libstdc++.so.6
#3  0x00007f770a50350c in __cxxabiv1::__terminate(void (*)()) () from /lib64/libstdc++.so.6
#4  0x00007f770a502529 in __cxa_call_terminate () from /lib64/libstdc++.so.6
#5  0x00007f770a502ea8 in __gxx_personality_v0 () from /lib64/libstdc++.so.6
#6  0x00007f770a265ad3 in _Unwind_RaiseException_Phase2 () from /lib64/libgcc_s.so.1
#7  0x00007f770a266041 in _Unwind_RaiseException () from /lib64/libgcc_s.so.1
#8  0x00007f770a5037bb in __cxa_throw () from /lib64/libstdc++.so.6
#9  0x00007f770a985b5e in oatpp::base::memory::MemoryPool::freeByEntryHeader(oatpp::base::memory::MemoryPool::EntryHeader*) [clone .cold.96] () 
#10 0x00007f770ab09aea in std::_Sp_counted_ptr_inplace<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >, oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > >, (__gnu_cxx::_Lock_policy)2>::_M_dispose() ()
#11 0x00007f770ab09ebf in oatpp::web::url::mapping::Pattern::~Pattern() ()
#12 0x00007f770aaff249 in oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler>::~Router() ()
#13 0x00007f770aaff04e in oatpp::web::server::HttpRouter::~HttpRouter() ()
#14 0x00007f770aa7e979 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x236b650) at /usr/include/c++/8/bits/shared_ptr_base.h:148
#15 std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x236b650) at /usr/include/c++/8/bits/shared_ptr_base.h:148
#16 std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=0x23a32c0, __in_chrg=<optimized out>) at /usr/include/c++/8/bits/shared_ptr_base.h:728
#17 std::__shared_ptr<oatpp::web::server::HttpRouter, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=0x23a32b8, __in_chrg=<optimized out>) at /usr/include/c++/8/bits/shared_ptr_base.h:1167
#18 std::shared_ptr<oatpp::web::server::HttpRouter>::~shared_ptr (this=0x23a32b8, __in_chrg=<optimized out>) at /usr/include/c++/8/bits/shared_ptr.h:103
#19 oatpp::base::Environment::Component<std::shared_ptr<oatpp::web::server::HttpRouter> >::~Component (this=0x23a3278, __in_chrg=<optimized out>)
    at /root/.conan/data/oatpp/1.1.0/_/_/package/56e0cf6d16ee57367a0661ab743f4e43b29223f8/include/oatpp-1.1.0/oatpp/oatpp/core/base/Environment.hpp:242
#20 zroute::ServiceComponent::~ServiceComponent (this=0x23a3170, __in_chrg=<optimized out>)
#21 std::default_delete<zroute::ServiceComponent>::operator() (this=0x23a4020, __ptr=0x23a3170) at /usr/include/c++/8/bits/unique_ptr.h:81
#22 std::unique_ptr<zroute::ServiceComponent, std::default_delete<zroute::ServiceComponent> >::~unique_ptr (this=0x23a4020, __in_chrg=<optimized out>) at /usr/include/c++/8/bits/unique_ptr.h:269
#23 zroute::HttpServer::~HttpServer (this=0x23a3f90, __in_chrg=<optimized out>)
#24 0x00007f770a9ac001 in std::default_delete<zroute::HttpServer>::operator() (this=0x7f770e5299c0, __ptr=0x23a3f90) at /usr/include/c++/8/bits/unique_ptr.h:342
#25 std::unique_ptr<zroute::HttpServer, std::default_delete<zroute::HttpServer> >::~unique_ptr (this=0x7f770e5299c0, __in_chrg=<optimized out>) at /usr/include/c++/8/bits/unique_ptr.h:269
#26 zroute::core::~core (this=0x7f770e529910, __in_chrg=<optimized out>)
#27 core_cleanup ()
#28 0x00007f770a9a2eff in destroy () at inroute_mod.c:137
#29 0x0000000000582a18 in destroy_modules () at core/sr_module.c:746
#30 0x000000000041d53d in cleanup (show_status=1) at main.c:563
#31 0x000000000041ed12 in shutdown_children (sig=15, show_status=1) at main.c:706
#32 0x000000000041f800 in handle_sigs () at main.c:737
#33 0x000000000042b465 in main_loop () at main.c:1817
#34 0x0000000000433137 in main (argc=13, argv=0x7ffd84f9ea78) at main.c:2856

The issue started to occur after I added 2nd controller.
There was no such issue with single controller.

OATPP version is 1.1.0

After I disabled pool allocation the issue doesn't reproduce anymore:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -26,7 +26,7 @@ option(OATPP_BUILD_TESTS "Create test target for oat++" ON)
 ###################################################################################################
.
 option(OATPP_DISABLE_ENV_OBJECT_COUNTERS "Disable object counting for Release builds for better performance" OFF)
-option(OATPP_DISABLE_POOL_ALLOCATIONS "This will make oatpp::base::memory::MemoryPool, method obtain and free call new and delete directly" OFF)
+option(OATPP_DISABLE_POOL_ALLOCATIONS "This will make oatpp::base::memory::MemoryPool, method obtain and free call new and delete directly" ON)
.
 set(OATPP_THREAD_HARDWARE_CONCURRENCY "AUTO" CACHE STRING "Predefined value for function oatpp::concurrency::Thread::getHardwareConcurrency()")
 set(OATPP_THREAD_DISTRIBUTED_MEM_POOL_SHARDS_COUNT "10" CACHE STRING "Number of shards of ThreadDistributedMemoryPool")

Looks to be connected with #173

Is this a bug at OATPP custom pool memory allocator?

Thanks in advance.

@lganzzzo
Copy link
Member

Hello @pcapdump ,

The memory pool error Invalid EntryHeader occurs when the pool object is returned to the pool and for some reason, it discovers that its entry-header was corrupted.- This check was added to detect memory leaks.

You may disable pool allocations with -DOATPP_DISABLE_POOL_ALLOCATIONS=ON cmake flag and silent this error, but
it doesn't mean that the reason was fixed and your app won't crash silently at some point.

The number of controllers doesn't really matter, you may have as many controllers as you like.
Most likely there is a memory leak in your code.

  • What oatpp API do you use, simple or async?
  • Is your code open-sourced, can I see it somewhere?

Regards,
Leonid

@pcapdump
Copy link
Contributor Author

pcapdump commented Sep 24, 2020

Thanks for answer, Leonid.

Hello @pcapdump ,

The memory pool error Invalid EntryHeader occurs when the pool object is returned to the pool and for some reason, it discovers that its entry-header was corrupted.- This check was added to detect memory leaks.

You may disable pool allocations with -DOATPP_DISABLE_POOL_ALLOCATIONS=ON cmake flag and silent this error, but
it doesn't mean that the reason was fixed and your app won't crash silently at some point.

The number of controllers doesn't really matter, you may have as many controllers as you like.
Most likely there is a memory leak in your code.

Aren't you mussing up memory leak and memory corruption?
These are different things.
Memory leak doesn't cause memory corruption.
And memory corruption doesn't cause memory leak.

Memory leak can crash application only when there is out-of-memory condition (when application consumes several Gigabytes of RAM), which is not our case.

  • What oatpp API do you use, simple or async?

simple

  • Is your code open-sourced, can I see it somewhere?

no, it's closed source.

@lganzzzo
Copy link
Member

lganzzzo commented Sep 24, 2020

Hey,

Memory leak can crash application only when there is out-of-memory condition (when application consumes several Gigabytes of RAM), which is not our case.

In your case, it seems that the application was still using a resource that was already deleted.
From the log, I can conclude that the underlying memory pool was deleted before the router was destructed. But it's hard to tell without seeing the code and a careful debugging.

Also, memory pool is static, so its kinda weird

@pcapdump
Copy link
Contributor Author

pcapdump commented Sep 24, 2020

Hey,

Memory leak can crash application only when there is out-of-memory condition (when application consumes several Gigabytes of RAM), which is not our case.

In your case, it seems that the application was still using a resource that was already deleted.
From the log, I can conclude that the underlying memory pool was deleted before the router was destructed. But it's hard to tell without seeing the code and a careful debugging.

Also, memory pool is static, so its kinda weird

Well, this could be the root of the issue.

https://github.com/oatpp/oatpp/blob/master/src/oatpp/core/data/stream/ChunkedBuffer.hpp#L55

Destruction order of static (=global) objects on application exit is undefined.
And we have crash at exit.
Likely static memory pool is destructed before the router.

BTW, your code violates Google coding conventions for C++:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
"Objects with static storage duration are forbidden unless they are trivially destructible."
due to exactly the same reason.

static complex objects should be allocated with new and never deleted:

  static oatpp::base::memory::ThreadDistributedMemoryPool& getSegemntPool(){
    static std::once_flag flag;
    static oatpp::base::memory::ThreadDistributedMemoryPool *pool = nullptr;
    std::call_once(flag, []() {
            if (pool == nullptr) {
                pool = new oatpp::base::memory::ThreadDistributedMemoryPool(CHUNK_POOL_NAME, CHUNK_ENTRY_SIZE,  CHUNK_CHUNK_SIZE);
            }
        });
    return *pool;
  }

Above is the proper Singleton pattern for your code as I transformed static object with undefined destruction time to static pointer which is never freed.
This matches Google's recommendations from Common patterns:
"If all else fails, you can create an object dynamically and never delete it by using a function-local static pointer or reference (e.g., static const auto& impl = *new T(args...);)."

@lganzzzo
Copy link
Member

Hey,

Thanks for the example code-snippet and for recommendations.
Oat++ doesn't aim to follow google coding conventions. However, some considerations may be helpful.

Destruction order of static (=global) objects on application exit is undefined.
And we have crash at exit.
Likely static memory pool is destructed before the router

This doesn't look like the root of an issue to me.
The memory pool is static and will be destroyed in whatever random order with other static objects.
The point is that all other oatpp objects should NOT be static and all previously acquired memory pool entries must be returned to the pool BEFORE the pool is destroyed.

From the pools' point of view, it's a memory leak.

@lganzzzo
Copy link
Member

lganzzzo commented Sep 24, 2020

@pcapdump ,

About the above snippet:

"If all else fails, you can create an object dynamically and never delete it by using a function-local static pointer or reference (e.g., static const auto& impl = *new T(args...);)."

This sounds reasonable.
Also, why not just:

  static oatpp::base::memory::ThreadDistributedMemoryPool& getSegemntPool(){
    static auto pool = new oatpp::base::memory::ThreadDistributedMemoryPool(CHUNK_POOL_NAME, CHUNK_ENTRY_SIZE,  CHUNK_CHUNK_SIZE);
    return *pool;
  }

Also, please feel free to send PRs with changes you believe will improve oatpp.

Regards,
Leonid

@pcapdump
Copy link
Contributor Author

static oatpp::base::memory::ThreadDistributedMemoryPool& getSegemntPool(){
static std::once_flag flag;
static oatpp::base::memory::ThreadDistributedMemoryPool *pool = nullptr;
std::call_once(flag, {
if (pool == nullptr) {
pool = new oatpp::base::memory::ThreadDistributedMemoryPool(CHUNK_POOL_NAME, CHUNK_ENTRY_SIZE, CHUNK_CHUNK_SIZE);
}
});
return *pool;
}

For the history, I tried this and it didn't help.
Only completely disabling pool allocation helps at the moment.

@lganzzzo
Copy link
Member

Hey @pcapdump ,

Thanks for the update, and please let me know in case you find something else.

Disabling memory pools is not a big deal and won't affect performance. Pools were added in the very first release of oatpp, and since then a lot has changed. I plan to remove pools in future releases and reintroduce them after careful testing where it's needed.

However, I still suspect there must be some resource leaking in your code.
Also, the process of stopping the server is a bit tricky and unclear at the moment...

Can you please share your code where you create components, and run the server and where you stop your server?

@bamkrs
Copy link
Member

bamkrs commented Sep 29, 2020

Hey, do you have the possibility to run your application with Valgrind? And can you create a minimal working example of the error?

@pcapdump
Copy link
Contributor Author

pcapdump commented Sep 30, 2020

Hi!

I reproduced the issue with Google Asan.

Here's the result (I omitted some private info):

=================================================================
==25668==ERROR: AddressSanitizer: heap-use-after-free on address 0x61b0000106d0 at pc 0x000000b6c6e8 bp 0x7ffd026d59f0 sp 0x7ffd026d59e0
READ of size 8 at 0x61b0000106d0 thread T0
    #0 0xb6c6e7 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/include/c++/8/bits/shared_ptr_base.h:727
    #1 0x11ee77f in std::__shared_ptr<oatpp::web::url::mapping::Pattern::Part, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr()
    #2 0x11ee79b in std::shared_ptr<oatpp::web::url::mapping::Pattern::Part>::~shared_ptr()
    #3 0x11f1f39 in oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >::LinkedListNode::~LinkedListNode()
    #4 0x11f1f11 in oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >::destroyNode(oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >::LinkedListNode*)
    #5 0x11f1c53 in oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >::clear()
    #6 0x11f1ac5 in oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >::~LinkedList()
    #7 0x11f1e07 in void std::allocator_traits<oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > > >::_S_destroy<oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std
    #8 0x11f1b48 in void std::allocator_traits<oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > > >::destroy<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Pa
    #9 0x11f15a6 in std::_Sp_counted_ptr_inplace<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >, oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > >, (
    #10 0xbb7ce1 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()
    #11 0xb6c714 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/include/c++/8/bits/shared_ptr_base.h:728
    #12 0x11ee7eb in std::__shared_ptr<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr()
    #13 0x11ee807 in std::shared_ptr<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > >::~shared_ptr()
    #14 0x11f1481 in oatpp::web::url::mapping::Pattern::~Pattern()
    #15 0x11f1ebe in void __gnu_cxx::new_allocator<oatpp::web::url::mapping::Pattern>::destroy<oatpp::web::url::mapping::Pattern>(oatpp::web::url::mapping::Pattern*)
    #16 0x11f1bd2 in void std::allocator_traits<std::allocator<oatpp::web::url::mapping::Pattern> >::destroy<oatpp::web::url::mapping::Pattern>(std::allocator<oatpp::web::url::mapping::Pattern>&, oatpp::web::url::mapping::Pattern*)
    #17 0x11f185a in std::_Sp_counted_ptr_inplace<oatpp::web::url::mapping::Pattern, std::allocator<oatpp::web::url::mapping::Pattern>, (__gnu_cxx::_Lock_policy)2>::_M_dispose()
    #18 0xbb7ce1 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()
    #19 0xb6c714 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/include/c++/8/bits/shared_ptr_base.h:728
    #20 0x11e07bb in std::__shared_ptr<oatpp::web::url::mapping::Pattern, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr()
    #21 0x11e07d7 in std::shared_ptr<oatpp::web::url::mapping::Pattern>::~shared_ptr()
    #22 0x11e0d9f in std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std::shared_ptr<oatpp::web::server::HttpRequestHandler> >::~pair()
    #23 0x11e4525 in void __gnu_cxx::new_allocator<std::_List_node<std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std::shared_ptr<oatpp::web::server::HttpRequestHandler> > > >::destroy<std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std::shared_ptr<o
    #24 0x11e44d9 in void std::allocator_traits<std::allocator<std::_List_node<std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std::shared_ptr<oatpp::web::server::HttpRequestHandler> > > > >::destroy<std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std
    #25 0x11e4493 in std::__cxx11::_List_base<std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std::shared_ptr<oatpp::web::server::HttpRequestHandler> >, std::allocator<std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std::shared_ptr<oatpp::web::server:
    #26 0x11e43f9 in std::__cxx11::_List_base<std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std::shared_ptr<oatpp::web::server::HttpRequestHandler> >, std::allocator<std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std::shared_ptr<oatpp::web::server:
    #27 0x11e42a1 in std::__cxx11::list<std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std::shared_ptr<oatpp::web::server::HttpRequestHandler> >, std::allocator<std::pair<std::shared_ptr<oatpp::web::url::mapping::Pattern>, std::shared_ptr<oatpp::web::server::HttpR
    #28 0x11e45bb in oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler>::~Router()
    #29 0x11e497e in void __gnu_cxx::new_allocator<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> >::destroy<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> >(oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandl
    #30 0x11e490a in void std::allocator_traits<std::allocator<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> > >::destroy<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> >(std::allocator<oatpp::web::url::mapping::Router<oatpp::
    #31 0x11e468c in std::_Sp_counted_ptr_inplace<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler>, std::allocator<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> >, (__gnu_cxx::_Lock_policy)2>::_M_dispose()
    #32 0xbb7ce1 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()
    #33 0xb6c714 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/include/c++/8/bits/shared_ptr_base.h:728
    #34 0x11e0915 in std::__shared_ptr<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler>, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr()
    #35 0x11e0931 in std::shared_ptr<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> >::~shared_ptr()
    #36 0x11e09f5 in std::pair<oatpp::data::share::StringKeyLabel const, std::shared_ptr<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> > >::~pair()
    #37 0x11e3247 in void __gnu_cxx::new_allocator<std::__detail::_Hash_node<std::pair<oatpp::data::share::StringKeyLabel const, std::shared_ptr<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> > >, false> >::destroy<std::pair<oatpp::data::share::StringKe
    #38 0x11e2ba6 in void std::allocator_traits<std::allocator<std::__detail::_Hash_node<std::pair<oatpp::data::share::StringKeyLabel const, std::shared_ptr<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> > >, false> > >::destroy<std::pair<oatpp::data::s
    #39 0x11e2466 in std::__detail::_Hashtable_alloc<std::allocator<std::__detail::_Hash_node<std::pair<oatpp::data::share::StringKeyLabel const, std::shared_ptr<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> > >, false> > >::_M_deallocate_node(std::__d
    #40 0x11e1d0f in std::__detail::_Hashtable_alloc<std::allocator<std::__detail::_Hash_node<std::pair<oatpp::data::share::StringKeyLabel const, std::shared_ptr<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> > >, false> > >::_M_deallocate_nodes(std::__
    #41 0x11e1569 in std::_Hashtable<oatpp::data::share::StringKeyLabel, std::pair<oatpp::data::share::StringKeyLabel const, std::shared_ptr<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> > >, std::allocator<std::pair<oatpp::data::share::StringKeyLabel
    #42 0x11e0bc9 in std::_Hashtable<oatpp::data::share::StringKeyLabel, std::pair<oatpp::data::share::StringKeyLabel const, std::shared_ptr<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> > >, std::allocator<std::pair<oatpp::data::share::StringKeyLabel
    #43 0x11e08f5 in std::unordered_map<oatpp::data::share::StringKeyLabel, std::shared_ptr<oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler> >, std::hash<oatpp::data::share::StringKeyLabel>, std::equal_to<oatpp::data::share::StringKeyLabel>, std::allocato
    #44 0x11e4557 in oatpp::web::server::HttpRouter::~HttpRouter()
    #45 0x11e49b2 in void __gnu_cxx::new_allocator<oatpp::web::server::HttpRouter>::destroy<oatpp::web::server::HttpRouter>(oatpp::web::server::HttpRouter*)
    #46 0x11e494a in void std::allocator_traits<std::allocator<oatpp::web::server::HttpRouter> >::destroy<oatpp::web::server::HttpRouter>(std::allocator<oatpp::web::server::HttpRouter>&, oatpp::web::server::HttpRouter*)
    #47 0x11e47fa in std::_Sp_counted_ptr_inplace<oatpp::web::server::HttpRouter, std::allocator<oatpp::web::server::HttpRouter>, (__gnu_cxx::_Lock_policy)2>::_M_dispose()
    #48 0xbb7ce1 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()
    #49 0xb6c714 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/include/c++/8/bits/shared_ptr_base.h:728
    #50 0xf6586d in std::__shared_ptr<oatpp::web::server::HttpRouter, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() /usr/include/c++/8/bits/shared_ptr_base.h:1167
    #51 0xf658fc in std::shared_ptr<oatpp::web::server::HttpRouter>::~shared_ptr() /usr/include/c++/8/bits/shared_ptr.h:103
    #52 0xf659a2 in oatpp::base::Environment::Component<std::shared_ptr<oatpp::web::server::HttpRouter> >::~Component() /root/.conan/data/oatpp/1.1.0/_/_/package/56e0cf6d16ee57367a0661ab743f4e43b29223f8/include/oatpp-1.1.0/oatpp/oatpp/core/base/Environment.hpp:242
    #53 0xf65c58 in zroute::ServiceComponent::~ServiceComponent()
    #54 0xf65d0a in std::default_delete<zroute::ServiceComponent>::operator()(zroute::ServiceComponent*) const /usr/include/c++/8/bits/unique_ptr.h:81
    #55 0xf65de9 in std::unique_ptr<zroute::ServiceComponent, std::default_delete<zroute::ServiceComponent> >::~unique_ptr() /usr/include/c++/8/bits/unique_ptr.h:269
    #56 0xf5f728 in zroute::HttpServer::~HttpServer()
    #57 0xdf29da in std::default_delete<zroute::HttpServer>::operator()(zroute::HttpServer*) const /usr/include/c++/8/bits/unique_ptr.h:81
    #58 0xdf86e9 in std::unique_ptr<zroute::HttpServer, std::default_delete<zroute::HttpServer> >::~unique_ptr() /usr/include/c++/8/bits/unique_ptr.h:269
    #59 0xdf92d0 in zroute::core::~core()
    #60 0xdeb332 in core_cleanup
    #61 0xdd6ae0 in ____C_A_T_C_H____T_E_S_T____0
    #62 0xaa59d7 in Catch::TestInvokerAsFunction::invoke() const /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:14036
    #63 0xaa2bd7 in Catch::TestCase::invoke() const /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:13929
    #64 0xa8984f in Catch::RunContext::invokeActiveTestCase() /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:12791
    #65 0xa88a3a in Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4
    #66 0xa82c3c in Catch::RunContext::runTest(Catch::TestCase const&) /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:12525
    #67 0xa90e48 in execute /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:13119
    #68 0xa9635d in Catch::Session::runInternal() /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:13325
    #69 0xa95601 in Catch::Session::run() /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:13281
    #70 0xbb021a in int Catch::Session::run<char>(int, char const* const*)
    #71 0xaf77be in main /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:17164
    #72 0x7fc613d496a2 in __libc_start_main (/lib64/libc.so.6+0x236a2)
    #73 0xa3bc0d in _start

0x61b0000106d0 is located 848 bytes inside of 1536-byte region [0x61b000010380,0x61b000010980)
freed by thread T1 here:
    #0 0x7fc615136818 in operator delete[](void*) (/lib64/libasan.so.5+0xf2818)
    #1 0xed1a56 in oatpp::base::memory::MemoryPool::~MemoryPool()
    #2 0x7fc613d6062e in __call_tls_dtors (/lib64/libc.so.6+0x3a62e)

previously allocated by thread T1 here:
    #0 0x7fc615135960 in operator new[](unsigned long) (/lib64/libasan.so.5+0xf1960)
    #1 0xed1b2c in oatpp::base::memory::MemoryPool::allocChunk()
    #2 0xed1945 in oatpp::base::memory::MemoryPool::MemoryPool(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, long, long)
    #3 0x11f13ba in oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >::LinkedListNode::LinkedList_Node_Pool::getPool()
    #4 0x11f131c in oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >::LinkedList()
    #5 0x11f0f53 in std::enable_if<std::__and_<std::__and_<std::__not_<std::allocator_traits<oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > > >::__construct_helper<oatpp::collection::LinkedL
    #6 0x11f0bfe in decltype (_S_construct({parm#1}, {parm#2})) std::allocator_traits<oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > > >::construct<oatpp::collection::LinkedList<std::shared_
    #7 0x11f06af in std::_Sp_counted_ptr_inplace<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >, oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > >, (
    #8 0x11effa0 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >, oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::
    #9 0x11ef8fb in std::__shared_ptr<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url:
    #10 0x11ef5ae in std::shared_ptr<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > >::shared_ptr<oatpp::base::memory::PoolSharedObjectAllocator<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > >>(
    #11 0x11ef11f in std::shared_ptr<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > > std::allocate_shared<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >, oatpp::base::memory::PoolSharedObjectAll
    #12 0x11eeda4 in std::shared_ptr<oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> > > oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >::Shared_LinkedList_Pool::allocateShared<>()
    #13 0x11eeabb in oatpp::collection::LinkedList<std::shared_ptr<oatpp::web::url::mapping::Pattern::Part> >::createShared()
    #14 0x11ee846 in oatpp::web::url::mapping::Pattern::Pattern()
    #15 0x11f0cf0 in void __gnu_cxx::new_allocator<oatpp::web::url::mapping::Pattern>::construct<oatpp::web::url::mapping::Pattern>(oatpp::web::url::mapping::Pattern*)
    #16 0x11f0820 in void std::allocator_traits<std::allocator<oatpp::web::url::mapping::Pattern> >::construct<oatpp::web::url::mapping::Pattern>(std::allocator<oatpp::web::url::mapping::Pattern>&, oatpp::web::url::mapping::Pattern*)
    #17 0x11f01da in std::_Sp_counted_ptr_inplace<oatpp::web::url::mapping::Pattern, std::allocator<oatpp::web::url::mapping::Pattern>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<>(std::allocator<oatpp::web::url::mapping::Pattern>)
    #18 0x11ef99a in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<oatpp::web::url::mapping::Pattern, std::allocator<oatpp::web::url::mapping::Pattern>>(oatpp::web::url::mapping::Pattern*&, std::_Sp_alloc_shared_tag<std::allocator<oatpp::web::url::mapping::Patt
    #19 0x11ef5e7 in std::__shared_ptr<oatpp::web::url::mapping::Pattern, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<oatpp::web::url::mapping::Pattern>>(std::_Sp_alloc_shared_tag<std::allocator<oatpp::web::url::mapping::Pattern> >)
    #20 0x11ef160 in std::shared_ptr<oatpp::web::url::mapping::Pattern>::shared_ptr<std::allocator<oatpp::web::url::mapping::Pattern>>(std::_Sp_alloc_shared_tag<std::allocator<oatpp::web::url::mapping::Pattern> >)
    #21 0x11eee05 in std::shared_ptr<oatpp::web::url::mapping::Pattern> std::allocate_shared<oatpp::web::url::mapping::Pattern, std::allocator<oatpp::web::url::mapping::Pattern>>(std::allocator<oatpp::web::url::mapping::Pattern> const&)
    #22 0x11eeaed in std::shared_ptr<oatpp::web::url::mapping::Pattern> std::make_shared<oatpp::web::url::mapping::Pattern>()
    #23 0x11ee881 in oatpp::web::url::mapping::Pattern::createShared()
    #24 0x11ed4e2 in oatpp::web::url::mapping::Pattern::parse(unsigned char*, long)
    #25 0x11edb4c in oatpp::web::url::mapping::Pattern::parse(oatpp::data::mapping::type::String const&)
    #26 0x11e0dcb in oatpp::web::url::mapping::Router<oatpp::web::server::HttpRequestHandler>::route(oatpp::data::mapping::type::String const&, std::shared_ptr<oatpp::web::server::HttpRequestHandler> const&)
    #27 0x11e0592 in oatpp::web::server::HttpRouter::route(oatpp::data::mapping::type::String const&, oatpp::data::mapping::type::String const&, std::shared_ptr<oatpp::web::server::HttpRequestHandler> const&)
    #28 0x11e4ab1 in oatpp::web::server::api::ApiController::addEndpointsToRouter(std::shared_ptr<oatpp::web::server::HttpRouter> const&)
    #29 0xf5fe8a in operator()


Thread T1 created by T0 here:
    #0 0x7fc615096e73 in __interceptor_pthread_create (/lib64/libasan.so.5+0x52e73)
    #1 0x7fc614744e58 in std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) (/lib64/libstdc++.so.6+0xc2e58)
    #2 0xf5f512 in zroute::HttpServer::start()
    #3 0xdef206 in zroute::core::start()
    #4 0xdef6f2 in core_start
    #5 0xdd65d2 in ____C_A_T_C_H____T_E_S_T____0
    #6 0xaa59d7 in Catch::TestInvokerAsFunction::invoke() const /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:14036
    #7 0xaa2bd7 in Catch::TestCase::invoke() const /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:13929
    #8 0xa8984f in Catch::RunContext::invokeActiveTestCase() /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:12791
    #9 0xa88a3a in Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4f
    #10 0xa82c3c in Catch::RunContext::runTest(Catch::TestCase const&) /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:12525
    #11 0xa90e48 in execute /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:13119
    #12 0xa9635d in Catch::Session::runInternal() /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:13325
    #13 0xa95601 in Catch::Session::run() /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:13281
    #14 0xbb021a in int Catch::Session::run<char>(int, char const* const*)
    #15 0xaf77be in main /root/.conan/data/catch2/2.11.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/catch2/catch.hpp:17164
    #16 0x7fc613d496a2 in __libc_start_main (/lib64/libc.so.6+0x236a2)

SUMMARY: AddressSanitizer: heap-use-after-free /usr/include/c++/8/bits/shared_ptr_base.h:727 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count()
Shadow bytes around the buggy address:
  0x0c367fffa080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c367fffa090: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c367fffa0a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c367fffa0b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c367fffa0c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c367fffa0d0: fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd
  0x0c367fffa0e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c367fffa0f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c367fffa100: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c367fffa110: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c367fffa120: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07.
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==25668==

I interpret it as follows:

  1. static MemoryPool has been destructed due to compiler-specific cleanup sequence (see __call_tls_dtors above) - this is the automatic code generated by compiler and we don't control it
  2. Then we destroy oatpp::web::server::HttpRouter as part of our cleanup sequence.
  3. ~HttpRouter indirectly appeals to the destroyed MemoryPool.
  4. heap-use-after-free error seen at Asan. And without Asan it leads to Invalid EntryHeader exception.

Then I was a bit frustrated why do I get a static MemoryPool object cleanup if I changed it to be a static pointer which should be never deleted (see my patch above).
I started digging oatpp sources and found out that MemoryPool is declared a static object not only in the place I fixed, but in many other places as well.

Then I changed the code in all the places and the issue was fixed.
We get no Invalid EntryHeader` exception anymore.

Please see patch attached.
Sorry, I don't have permissions to branch oatpp at your repository and I don't know how to deliver PR without this permission.

Sorry, it's not a final patch as it contains a mixture of different ideas and workarounds which need to be cleaned.
The main idea is to replace static MemoryPool pool with static MemoryPool *pool.
I advice to adapt it to the pattern using std::once, which is a thread-safe version of Singleton pattern (I'm not sure if it applies to oatpp however as I'm not a big expert of oatpp and I don't know if oatpp needs to support multithreading when allocating MemoryPools or not).

308.txt

@lganzzzo
Copy link
Member

Hey @pcapdump ,

Thanks for investigating this issue and for the patch!

Sorry, I don't have permissions to branch oatpp at your repository and I don't know how to deliver PR without this permission.

It's possible to do with forks:

  • Fork the repo - click on the "fork" button in the right-top (next to watch and star).
    It will make a copy of the repo in your github account.
  • Make changes in your copy of the repo.
  • Send PR directly from you repo

You can do everything on Github UI with just button-clicking.

Sorry, it's not a final patch as it contains a mixture of different ideas and workarounds which need to be cleaned.
The main idea is to replace static MemoryPool pool with static MemoryPool *pool.

I've reviewed the Patch, and it looks good to me.
The only place we should pay more attention is here (with thread locals):

static oatpp::base::memory::MemoryPool& getPool(const AllocatorPoolInfo& info){
#ifndef OATPP_COMPAT_BUILD_NO_THREAD_LOCAL
  /* memory leak because of thread_local */
  static thread_local oatpp::base::memory::MemoryPool* pool = new oatpp::base::memory::MemoryPool(info.poolName, sizeof(T), info.poolChunkSize);
#else
  static auto pool = new oatpp::base::memory::MemoryPool(info.poolName, sizeof(T), info.poolChunkSize);
#endif
  return *pool;
}

I advice to adapt it to the pattern using std::once, which is a thread-safe version of Singleton pattern (I'm not sure if it applies to oatpp however as I'm not a big expert of oatpp and I don't know if oatpp needs to support multithreading when allocating MemoryPools or not).

Oat++ min C++ version is 11, and starting from C++11 such singleton is guaranteed to be thread safe
(well, unless there are some crippled compilers out there)

static MemoryPool& getPool() {
  static auto pool = new MemoryPool();
  return *pool;
}

So I'm not sure if we should introduce additional safety constructions. Well, again, unless there is a known case where it doesn't work as expected by the standard.


Please feel free to send the PR.

Best Regards,
Leonid

@pcapdump
Copy link
Contributor Author

pcapdump commented Oct 1, 2020

Leonid, please review PR #314

Alas, it doesn't work without std::call_once.
It was my 1st attempt to try without it: 6974e70
And unit tests failed: https://github.com/oatpp/oatpp/runs/1192140265
Looks like there is a concurrency issue.

With std::call_once all checks pass.

@pcapdump
Copy link
Contributor Author

pcapdump commented Oct 1, 2020

Still having issue with PR #314 :

0x61b000010618 is located 664 bytes inside of 1536-byte region [0x61b000010380,0x61b000010980)
freed by thread T1 here:
    #0 0x7fe1ca87d818 in operator delete[](void*) (/lib64/libasan.so.5+0xf2818)
    #1 0x9d6f1c in oatpp::base::memory::MemoryPool::~MemoryPool() (/Volumes/data/work/zaleos/git/softin-inesrp/src/modules/inroute/libzroute/build/bin/core_run_test_test+0x9d6f1c)
    #2 0x7fe1c94a762e in __call_tls_dtors (/lib64/libc.so.6+0x3a62e)

Still ~MemoryPool() is called on cleanup, although it seems I replaced all static MemoryPool pool objects with static MemoryPool *pool with no delete...

@lganzzzo
Copy link
Member

lganzzzo commented Oct 1, 2020

@pcapdump , hey
It may be that thread local pool - it was the only pool you didnt change to a new pattern

I'll be able to take a look closer in 3 hours.

@pcapdump
Copy link
Contributor Author

pcapdump commented Oct 1, 2020

@pcapdump , hey
It may be that thread local pool - it was the only pool you didnt change to a new pattern

I'll be able to take a look closer in 3 hours.

It looks like you're right.
As soon as I switched OATPP_COMPAT_BUILD_NO_THREAD_LOCAL to ON, the issue has gone away.

However I don't know how to express static thread_local MemoryPool pool to make it safe for stopping the application with CTRL+C and keeping the original oatpp design at the same time.
It looks like it can't be replaced with static thread_local MemoryPool *pool as it will produce memory leak in any new thread.
IMO it's safe to use this construction only for trivial objects like static thread_local int.
If I were you, I wouldn't have used this expression in the code at all, as it seems way too unsafe.
Thus, I don't know how to fix it in my PR and preserve original design at the same time.

Do you think you could fix it somehow at next release?
We use a binary oatpp package from Conan and they build package from your releases.
We don't have a way to apply our own patches or config options to oatpp,
and the application crash at CTRL+C makes issues for us...

Please advice what's the best way to proceed.
Maybe you could set OATPP_DISABLE_POOL_ALLOCATIONS to ON by default in next release?
You mentioned you planned to do this and this would fix our issue...

lganzzzo added a commit that referenced this issue Oct 1, 2020
Issue #308: oatpp::base::memory::MemoryPool::freeByEntryHeader()]: Invalid EntryHeader
@lganzzzo
Copy link
Member

lganzzzo commented Oct 2, 2020

Hey @pcapdump ,

I like the changes in the PR so it's merged now.

However I don't know how to express static thread_local MemoryPool pool to make it safe for stopping the application with CTRL+C and keeping the original oatpp design at the same time.

Ok, now I got what was the original issue :)
All you need is to add the interrupt signal handler - here is the example I've made for you

You are stopping the application with an interrupt signal thus it doesn't go through the proper cleanup process and memory pools report a leak.

With a signal handler, you can gracefully stop the server like here and no error should be reported.

$ ./my-project-exe                                                           
 I |2020-10-02 02:55:04 1601596504860169| MyApp:Server running on port 8000
^C I |2020-10-02 02:55:06 1601596506422256| MyApp:Sig received!
$ _

Please advice what's the best way to proceed.
Maybe you could set OATPP_DISABLE_POOL_ALLOCATIONS to ON by default in next release?
You mentioned you planned to do this and this would fix our issue...

From my side, I would recommend having your own fork and a build pipeline above it. Thus you'll be able:

  • To fine-adapt oatpp for your needs.
  • Get the latest updates from oatpp source when it's needed.
  • Contribute back to oatpp source.

This approach will give the best synergy.

Yes, the pools will be addressed in the next releases. We have some code I want to clean up. Pools also should be tunned a bit.

Regards,
Leonid

@lganzzzo
Copy link
Member

lganzzzo commented Oct 2, 2020

Hey @pcapdump ,

I'm curious if you could describe your oatpp use-case - it helps to better understand our users.

Also, please feel free to join the devs chat on Gitter - https://gitter.im/oatpp-framework/Lobby

Regards

@pcapdump
Copy link
Contributor Author

pcapdump commented Oct 2, 2020

Thanks for the answer, @lganzzzo

Hey @pcapdump ,

I like the changes in the PR so it's merged now.

However I don't know how to express static thread_local MemoryPool pool to make it safe for stopping the application with CTRL+C and keeping the original oatpp design at the same time.

Ok, now I got what was the original issue :)
All you need is to add the interrupt signal handler - here is the example I've made for you

You are stopping the application with an interrupt signal thus it doesn't go through the proper cleanup process and memory pools report a leak.

Sorry, but I thought that you understood that the issue here is not a leak in our application, but a heap-use-after-free condition caused by using of static MemoryPool objects inside oatpp.
We can't control when the static objects inside oatpp are destroyed.
Therefore it's strange from your side to blame the application for the incorrect cleanup order inside oatpp, which we don't control.
Don't you agree on that basing on the Asan output I gave you above?

With a signal handler, you can gracefully stop the server like here and no error should be reported.

$ ./my-project-exe                                                           
 I |2020-10-02 02:55:04 1601596504860169| MyApp:Server running on port 8000
^C I |2020-10-02 02:55:06 1601596506422256| MyApp:Sig received!
$ _

This solution doesn't fit us well, because:

  1. We are writing the shared library.
    The shared library is linked with oatpp to run a REST service to control the main application.
    The shared library is dynamically loaded by the main application using dlopen().
    We don't control source code of the main application.
    It's very hard for us to make changes to it.
    The main application already has its own signal handler installed.
    We can't install different signal handler from our shared library as it will overwrite main application's signal handler.

  2. Stopping the server inside signal handler is not safe.
    See https://www.man7.org/linux/man-pages/man7/signal-safety.7.html
    If server->stop() indirectly calls any of the non-async-signal-safe functions, whole application may sometimes deadlock or crash at exit.
    And I guess it's near to impossible to audit all of the functions called by server->stop() internally, including the stdlib functions for the async-signal-safety.

Please advice what's the best way to proceed.
Maybe you could set OATPP_DISABLE_POOL_ALLOCATIONS to ON by default in next release?
You mentioned you planned to do this and this would fix our issue...

From my side, I would recommend having your own fork and a build pipeline above it. Thus you'll be able:

  • To fine-adapt oatpp for your needs.
  • Get the latest updates from oatpp source when it's needed.
  • Contribute back to oatpp source.

So the outcome of this conversation is that you don't plan to fix it?
And we should fork and fix it at our side by disabling OATPP_DISABLE_POOL_ALLOCATIONS?

@lganzzzo
Copy link
Member

lganzzzo commented Oct 2, 2020

Hey @pcapdump ,

Sorry, but I thought that you understood that the issue here is not a leak in our application, but a heap-use-after-free condition caused by using of static MemoryPool objects inside oatpp.
We can't control when the static objects inside oatpp are destroyed.
Therefore it's strange from your side to blame the application for the incorrect cleanup order inside oatpp, which we don't control.
Don't you agree on that basing on the Asan output I gave you above?

From oatpp perspective, it's exactly a memory leak. And Asan's output shows that you are using oatpp incorrectly.
Here's why:

How resource life-cycle should look like in a typical oatpp application

App start

  1. Static resource allocation
  2. Environment init
  3. Dynamic resource allocation <-- Router should be here

App exit

  1. Dynamic resource de-allocation <-- Router should be here
  2. Environment destroy
  3. Static resource deallocation

Whatever reason that leads to the Router being destroyed in uncontrolled order during static resource deallocation -
is treated as memory-leak - because it has not been destroyed at the dynamic resource de-allocation stage.

Therefore it's strange from your side to blame the application for the incorrect cleanup order inside oatpp, which we don't control.

All user-created oatpp objects including the Router must not be static - it's a restriction of oatpp.
If you have the Router being static - it's an incorrect usage of a framework.

Don't you agree on that basing on the Asan output I gave you above?

The output of Asan looks correct and was expected in this case.


The correct way of doing custom oatpp based library

  • Your library provides ApiController classes.
  • User code chooses what ApiControllers to use and creates them if needed. Also, the user code is responsible for oatpp Environment initialization and deinitialization.

The shared library is linked with oatpp to run a REST service to control the main application.

IMHO libraries should provide tools and resources for the user application.
Isn't it strange when the library controls the main application?

The corporate world

The shared library is linked with oatpp to run a REST service to control the main application.

In the corporate world, this is totally understandable and considered to be "normal".

However, what is strange in the corporate world - is when an open-source library --> which is community Conan-packed --> which is wrapped by your library --> shipped to control user application without an intermediate fork maintained by the vendor.

The Solution

  • Fork oatpp
  • -DOATPP_DISABLE_POOL_ALLOCATIONS=ON

Little hack - little blood.

So the outcome of this conversation is that you don't plan to fix it?
And we should fork and fix it at our side by disabling OATPP_DISABLE_POOL_ALLOCATIONS?

Yes.
Some changes to mem-pools will be done because we want to improve it.
But we won't do changes to oatpp origin just because your company has a legacy delivery pipeline that needs another hack.

@lganzzzo
Copy link
Member

I'm closing this.
@pcapdump , please feel free to reopen or create a new issue in case of any updates

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

No branches or pull requests

3 participants