Bug 3418 - Crash on cleanup when ACE_SSL_Context singleton is created from TAO_SSLIOP configured in ORB's local gestalt
Summary: Crash on cleanup when ACE_SSL_Context singleton is created from TAO_SSLIOP co...
Status: RESOLVED FIXED
Alias: None
Product: TAO
Classification: Unclassified
Component: SSLIOP Pluggable Protocol (show other bugs)
Version: 1.6.5
Hardware: All All
: P3 normal
Assignee: DOC Center Support List (internal)
URL:
Depends on:
Blocks: 2564
  Show dependency tree
 
Reported: 2008-09-04 02:51 CDT by Vladimir Zykov
Modified: 2011-03-07 04:01 CST (History)
0 users

See Also:


Attachments
This is a showcase for this report. It's a cleaned up version of the test for 1459. (2.94 KB, application/gzip)
2008-09-04 03:01 CDT, Vladimir Zykov
Details
Possible fix. (4.41 KB, patch)
2011-03-03 09:45 CST, Vladimir Zykov
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Vladimir Zykov 2008-09-04 02:51:21 CDT
In case we use TAO_SSLIOP through ACE Configuration framework and setup it in ORB with it's own local gestalt we have a crash upon process cleanup.

This happens because TAO_SSLIOP library is linked with ACE_SSL library and the later creates ACE_SSL_Context singleton which is registered for destruction with global ACE_Object_Manager singleton. When we destroy the orb where SSLIOP was configured the TAO_SSLIOP library gets unloaded and ACE_SSL is unloaded too. The above singletons remains dangling until end of the process execution and then we have a crash.
Comment 1 Vladimir Zykov 2008-09-04 03:01:58 CDT
Created attachment 1002 [details]
This is a showcase for this report. It's a cleaned up version of the test for 1459.
Comment 2 Johnny Willemsen 2008-09-04 03:18:44 CDT
seems you need to use a different singleton type which is tied to the dll 
Comment 3 Vladimir Zykov 2008-09-04 03:34:47 CDT
(In reply to comment #2)
> seems you need to use a different singleton type which is tied to the dll 
> 

I tried as a quick fix to use ACE_DLL_Singleton_Adapter_T but it didn't help and the documentation for it says that it only works when dll is loaded through ACE_DLL or ACE Service Configuration framework. But in this case ACE_SSL is loaded from TAO_SSLIOP and thus ACE_DLL_Singleton_Adapter_T is not applicable. My guess is that creation of ACE_SSL_Context singleton must increase a reference count of the TAO_SSLIOP dll in ACE_DLL_Manager but since ACE_SSL is loaded indirectly it's not easy to do. At least I don't see how to do it.
Comment 4 Johnny Willemsen 2008-11-30 03:16:51 CST
Any update on the regression in the repo and a fix?
Comment 5 Johnny Willemsen 2008-12-01 03:39:58 CST
what about making this an unmanaged singleton which we explicitly close when the dll gets unloaded?
Comment 6 Johnny Willemsen 2008-12-01 04:44:35 CST
Mon Dec  1 10:44:00 UTC 2008  Johnny Willemsen  <jwillemsen@remedy.nl>

        * orbsvcs/tests/Bug_3418_Regression/*:
          Added new regression test for bug 3418, this has not been
          fixed yet. Thanks to Vladimir Zykov
          <vladimir dot zykov at prismtech dot com> for creating this
          test
Comment 7 Johnny Willemsen 2008-12-01 07:47:28 CST
just committed the change below

        * ace/SSL/SSL_Context.cpp:
          Manage ourselves the life cycle of the SSL_Context singleton. Previously
          the SSL library registered a singleton with the ACE library. When
          the SSL library is unloaded expliclity before the ACE library the
          cleanup in ACE did crash because the SSL code wasn't there. Now when we
          cleanup the SSL library we also close our singleton
Comment 8 Johnny Willemsen 2008-12-02 03:22:49 CST
test results are increasing after the fix, I think this one if now fixed
Comment 9 Johnny Willemsen 2008-12-03 02:03:35 CST
test stats show that this now runs
Comment 10 Vladimir Zykov 2011-03-03 02:47:01 CST
Johnny, recently when looking at memory leaks in TAO_SSLIOP I found that the fix for this issue was not complete. It indeed fixed the crash but introduced a memory leak. It can be seen in Security/Big_Request test. The stack trace of that leak is the following.

==11234== 1,880 bytes in 1 blocks are possibly lost in loss record 450 of 459
==11234==    at 0x4C27E02: operator new[](unsigned long) (vg_replace_malloc.c:305)
==11234==    by 0x78CD71C: ACE_SSL_Context::ssl_library_init() (SSL_Context.cpp:148)
==11234==    by 0x78CD471: ACE_SSL_Context::ACE_SSL_Context() (SSL_Context.cpp:114)
==11234==    by 0x78CEF92: ACE_Singleton<ACE_SSL_Context, ACE_Thread_Mutex>::ACE_Singleton() (Singleton.inl:13)
==11234==    by 0x78CEC95: ACE_Unmanaged_Singleton<ACE_SSL_Context, ACE_Thread_Mutex>::ACE_Unmanaged_Singleton() (Singleton.inl:18)
==11234==    by 0x78CE95F: ACE_Unmanaged_Singleton<ACE_SSL_Context, ACE_Thread_Mutex>::instance() (Singleton.cpp:202)
==11234==    by 0x78CD69A: ACE_SSL_Context::instance() (SSL_Context.cpp:131)
==11234==    by 0x7459ABB: TAO::SSLIOP::Protocol_Factory::init(int, char**) (SSLIOP_Factory.cpp:145)
==11234==    by 0x592A482: ACE_Service_Object_Type::init(int, char**) const (Service_Types.cpp:103)
==11234==    by 0x592275F: ACE_Service_Gestalt::initialize_i(ACE_Service_Type const*, char const*) (Service_Gestalt.cpp:618)
==11234==    by 0x59224CE: ACE_Service_Gestalt::initialize(ACE_Service_Type_Factory const*, char const*) (Service_Gestalt.cpp:554)
==11234==    by 0x591AAE4: ACE_Dynamic_Node::apply(ACE_Service_Gestalt*, int&) (Parse_Node.cpp:326)
==11234==    by 0x592C746: ace_yyparse(void*) (Svc_Conf_y.cpp:1440)
==11234==    by 0x5922DB0: ACE_Service_Gestalt::process_directives_i(ACE_Svc_Conf_Param*) (Service_Gestalt.cpp:800)
==11234==    by 0x5922FE6: ACE_Service_Gestalt::process_file(char const*) (Service_Gestalt.cpp:905)
==11234==    by 0x5923EE2: ACE_Service_Gestalt::process_directives(bool) (Service_Gestalt.cpp:1271)
==11234==    by 0x592379B: ACE_Service_Gestalt::open_i(char const*, char const*, bool, bool, bool) (Service_Gestalt.cpp:1113)
==11234==    by 0x5924220: ACE_Service_Gestalt::open(int, char**, char const*, bool, bool, bool) (Service_Gestalt.inl:52)
==11234==    by 0x559CB22: (anonymous namespace)::open_private_services_i(ACE_Intrusive_Auto_Ptr<ACE_Service_Gestalt>, int&, char**, bool, bool) (TAO_Internal.cpp:586)
==11234==    by 0x559BF00: TAO::ORB::open_global_services(int, char**) (TAO_Internal.cpp:323)
==11234==    by 0x555E730: CORBA::ORB_init(int&, char**, char const*) (ORB.cpp:1254)
==11234==    by 0x40C63C: main (server.cpp:79)

ACE_Unmanaged_Singleton<ACE_SSL_Context, ACE_SYNCH_MUTEX>::close() is put to the function ACE_SSL_Context::ssl_library_fini() that is only called from the destructor of ACE_SSL_Context and which should ACE_Unmanaged_Singleton<ACE_SSL_Context, ACE_SYNCH_MUTEX>::close() actually call. So, this is circular.

We need a place where we can safely delete ACE_SSL_Context singleton. What about implementing initializer for the ACE_SSL library same as we have in a number of TAO libraries and destroying ACE_SSL_Context singleton in its fini() method?
Comment 11 Vladimir Zykov 2011-03-03 09:45:09 CST
Created attachment 1358 [details]
Possible fix.

Just to clarify the idea. Here is how ACE_SSL initializer can be implemented. It will create a service in ACE service config which will initialize and cleanup ACE_SSL_Context.
Comment 12 Johnny Willemsen 2011-03-03 13:31:49 CST
If this worksm please commit
Comment 13 Vladimir Zykov 2011-03-04 03:41:00 CST
(In reply to comment #12)
> If this worksm please commit
> 

Well, this requires a small change in TAO as well. Doing this line:

ACE_Service_Config::process_directive (ace_svc_desc_ACE_SSL_Initializer);

doesn't trigger calls to service's init/fini methods (it only creates the service in service config). For this to work we need to do static/dynamic service directive similar to those in svc.conf file but from TAO_SSLIOP code. The following line does the job:

ACE_Service_Config::process_directive (ACE_STATIC_SERVICE_DIRECTIVE ("ACE_SSL_Initializer", ""));
Comment 14 Vladimir Zykov 2011-03-07 04:01:08 CST
Made changes to ACE and TAO (revision 93498).

Mon Mar  7 09:38:57 UTC 2011  Vladimir Zykov  <vladimir.zykov@prismtech.com>

        * ace/SSL/SSL_Context.cpp:
        * ace/SSL/SSL_Initializer.cpp:
        * ace/SSL/SSL_Initializer.h:
          Added SSL initializer which is a part of the fix for bug#3418.
          The initializer if used does correct SSL cleanup. Moved SSL context
          singleton cleanup from ACE_SSL_Context::ssl_library_fini() to
          the initializer.


Mon Mar  7 09:44:39 UTC 2011  Vladimir Zykov  <vladimir.zykov@prismtech.com>

        * orbsvcs/orbsvcs/SSLIOP/SSLIOP_Factory.cpp:
          Fixed leaks in ACE_SSL library. This is a part of the fix
          for bug#3418.

        * orbsvcs/orbsvcs/SSLIOP/SSLIOP_Connector.cpp:
        * orbsvcs/orbsvcs/SSLIOP/SSLIOP_Accept_Strategy.cpp:
          Fixed other leaks in TAO_SSLIOP that I found during the work  
          on bug#3418.

        * orbsvcs/tests/Security/Bug_1107_Regression/Bug_1107_Regression.mpc:
        * orbsvcs/tests/Security/Big_Request/Big_Request.mpc:
          Removed unnecessary libraries from dependencies.

        * orbsvcs/tests/Security/InsecureReferences/test.cpp:
        * orbsvcs/tests/Security/MT_IIOP_SSL/server.cpp:
        * orbsvcs/tests/Security/Bug_1107_Regression/client.cpp:
        * orbsvcs/tests/Security/Crash_Test/server.cpp:
        * orbsvcs/tests/Security/ssliop_CSD/MessengerClient.cpp:
        * orbsvcs/tests/Security/BiDirectional/client.cpp:
        * orbsvcs/tests/Security/BiDirectional/server.cpp:
        * orbsvcs/tests/Security/MT_SSLIOP/server.cpp:
        * orbsvcs/tests/Security/Big_Request/client.cpp:
          Fixed memory leaks local to these tests.

        * orbsvcs/tests/Security/Crash_Test/run_test.pl:
        * bin/tao_other_tests.lst:
          Extended the test script so that it supports now QUICK config.
          This test could run for a very long time which is not necessary
          in setups like valgrind.