Memory leaks in H.323Plus library

Discussion related to implementation and use of the H.323 Plus H.323 stack at http://www.h323plus.org.
Sharath
Posts: 21
Joined: Sun Mar 02, 2014 4:41 pm
Contact:

Re: Memory leaks in H.323Plus library

Post by Sharath » Fri Mar 07, 2014 6:28 pm

Before I sign off for the day, I suspect the problem is in the following code. You are storing lots of objects of classes derived from H460_Feature in the dictionary Features. If GetPurpose() was a virtual function that returned different values for different objects, then the following code would make sense. However, since you are calling a static function that returns the same constant value for every object in the dictionary, this code will never delete a single object. Please review this function.

Image

shorne
Posts: 45
Joined: Thu Aug 27, 2009 4:17 am

Re: Memory leaks in H.323Plus library

Post by shorne » Fri Mar 14, 2014 10:51 am

I have conducted a full memory audit of h323plus using VLD and almost all the memory issues should now be resolved as of h323plus 1.26.3 (CVS). There is still an issue with a small leak in OpenSSL when securing the TCP transport which is still outstanding. Still trying to resolve that one.

Sharath
Posts: 21
Joined: Sun Mar 02, 2014 4:41 pm
Contact:

Re: Memory leaks in H.323Plus library

Post by Sharath » Tue Apr 22, 2014 8:00 am

Alright, I am back on this. Regarding the openssl related leaks, I suggest you take a look at this stackoverflow link.

Unrelated to memory leaks, if I set PTRACING=0, the h323plus library doesn't compile. It is nothing serious, the check for PTRACING flag is not rigorously done in the library.

Sharath
Posts: 21
Joined: Sun Mar 02, 2014 4:41 pm
Contact:

Re: Memory leaks in H.323Plus library

Post by Sharath » Tue Apr 22, 2014 6:13 pm

Got a break, I managed to get rid of all openssl related leaks.

Call CRYPTO_cleanup_all_ex_data() at the end of H323EndPoint::~H323EndPoint() function. It can also be done in the derived class destructor.

Call ERR_remove_state(0) at the end of H323ListenerTCP::Main() function. This is required while exiting every thread that uses openssl functions. If there are other such threads, do consider it.

The above two calls will clean up the openssl memory allocations.

User avatar
willamowius
Posts: 39
Joined: Tue Sep 01, 2009 7:25 am
Contact:

Re: Memory leaks in H.323Plus library

Post by willamowius » Tue Apr 22, 2014 7:20 pm

Thats an interestimng find. I did quite a bit of leak checking on the Linux version of GnuGk using Valgrind and I didn't see any OpenSSL leaks on Linux, even so I use similar code and never call CRYPTO_cleanup_all_ex_data() or ERR_remove_state(0).

Do the Windows and Linux version of OpenSSL behave differently ? Would you mind running GnuGk under your leak checker on Windows ?

Thanks!
Jan Willamowius
Founder of the GNU Gatekeeper Project
https://www.gnugk.org
https://www.willamowius.com (H.323 support)

Sharath
Posts: 21
Joined: Sun Mar 02, 2014 4:41 pm
Contact:

Re: Memory leaks in H.323Plus library

Post by Sharath » Wed Apr 23, 2014 4:27 am

I am not familiar with GnuGK yet. Still trying to get a hang of h323plus library, I use it with Avaya CM. I have not checked in Linux yet.

However, I have noticed that H323EndPoint::~H323EndPoint() does call CRYPTO_cleanup_all_ex_data() and ERR_remove_state(0) in case TLS is used. However, this needs to be called unconditionally.

According to openssl documention, ERR_remove_state() must be called at the end of every thread. Since I always openssl via BoostC++ (boost::asio::ssl) library, even I was not aware of it. But I looked up and discovered that even BoostC++ code calls ERR_remove_state() during the cleanup.

Sharath
Posts: 21
Joined: Sun Mar 02, 2014 4:41 pm
Contact:

Re: Memory leaks in H.323Plus library

Post by Sharath » Wed Apr 23, 2014 4:11 pm

Good News Gentlemen!

After chasing the memory leaks for 3 days using visual leak detector, I found three culprits.

I am using the latest h323plus files from the CVS as of 16th April, with ptlib 2.10.9

1) Call ERR_remove_state(0) at the end of H323ListenerTCP::Main() function.

2) In H323EndPoint destructor function move the OPENSSL cleanup outside of the check for m_transportContext. Cleanup is required even if m_transportContext is null.

Image

3) Finally, the BIG one, the leak in H460_FeatureSet. Some H460_Feature objects were being removed from the H460_FeatureSet, but not deleted. That was the cause of the leak. Just add the 3 lines highlighted in blue. The memory leak goes away.

Image

Final result from Visual Leak Detector:

Visual Leak Detector Version 2.4RC2 installed.
Outputting the report to C:\Log\memoryleak.log
No memory leaks detected.
Visual Leak Detector is now exiting.

User avatar
willamowius
Posts: 39
Joined: Tue Sep 01, 2009 7:25 am
Contact:

Re: Memory leaks in H.323Plus library

Post by willamowius » Wed Apr 23, 2014 5:19 pm

Thanks for the patches, I've put them into the CVS.

Maybe you can supply some more reasoning on the last fix:
Why do we have to delete some features but not the others ?
Jan Willamowius
Founder of the GNU Gatekeeper Project
https://www.gnugk.org
https://www.willamowius.com (H.323 support)

Sharath
Posts: 21
Joined: Sun Mar 02, 2014 4:41 pm
Contact:

Re: Memory leaks in H.323Plus library

Post by Sharath » Wed Apr 23, 2014 7:37 pm

willamowius wrote:Thanks for the patches, I've put them into the CVS.
Thanks. When is the next release planned, say 1.26.x ?
willamowius wrote:Maybe you can supply some more reasoning on the last fix:
Why do we have to delete some features but not the others ?
I had noticed that with every incoming call, a whole bunch of H460_Feature objects were being created, but not deleted when the call terminated. This sourcecode is huge (~1500 classes?), and my understanding of H323 protocols is shaky. And I am from SIP/TDM background. Therefore, understanding the code would take long. So I decided to look from C++ angle, and track the life cycle of every H460_Feature object, and H460_FeatureSet object. Just to see how they are created and destroyed, I wrote a small object tracking class and added it to both H460_Feature & H460_FeatureSet class. It would cout a message every time a object is created and destroyed.

After studying the messages, I noticed the following:
1) Some H460_Feature objects (permanent) are created during initialization and remain throughout the program lifecycle.
2) Some H460_Feature objects (temporary) are created and destroyed just before answering a call.
3) All H460_Feature objects are stored in H460_FeatureSet objects.
4) A H460_FeatureSet object can contain both permanent & temporary H460_Feature objects.
5) When a H460_FeatureSet object is deleted, it will delete all the temporary H460_Feature objects in it.
6) There are temporary and permanent H460_FeatureSet objects.

And here was the anomaly. Some temporary H460_Feature objects were not getting deleted when the call is answered. That means temporary H460_FeatureSet destructor is not deleting them. That could only happen if the temporary H460_Feature object is no more part of the temporary H460_FeatureSet object. Is somebody removing the temporary H460_Feature objects from the temporary H460_FeatureSet? Then I looked at functions that are removing the H460_Feature objects from H460_FeatureSet. That is how I zeroed in on DisableAllFeatures() function. This function removes both temporary and permanent H460_Feature objects from the temporary H460_FeatureSet object, but doesn't delete them. That is okay for permanent objects, but temporary object are effectively orphaned. So I added the code to delete only temporary objects, which are identified by the flag H460_Feature::FeatureSignal. That seems to have fixed the problem.

Keep in mind I used logic instead of domain knowledge of H323. My fix may not cover all situations. You guys have audit this fix when you find some time.

shorne
Posts: 45
Joined: Thu Aug 27, 2009 4:17 am

Re: Memory leaks in H.323Plus library

Post by shorne » Wed Apr 23, 2014 10:16 pm

Thanks for the patchs.

h4601.h ln 604 explains it

enum {
FeatureBaseAll =0, ///< AutoCreate Startup share instance with RAS and Signal
FeatureBaseClone =2, ///< AutoCreate Startup clone instance in RAS and Signal
FeatureBase =4, ///< AutoCreate Startup only (not used in RAS or Call Signalling)
FeatureRas =8, ///< AutoCreate RAS Only
FeatureSignal =16, ///< AutoCreate Call Signalling Only
FeaturePresence =64 ///< Manual Create in endpoint as required
} FeatureInstance;

Each H.460 feature operates in one of 6 ways. ALL features are instanced in different ways and different times however all are unique instances except FeatureBaseAll which are shared.

I have patched the CVS repository and removed the TODO.

Post Reply