[sr-dev] [tracker] Comment added: fixes for memory leak in TLS module

[sip-router]

THIS IS AN AUTOMATED MESSAGE, DO NOT REPLY.

The following task has a new comment added:

FS#380 - fixes for memory leak in TLS module
User who did this - Ding Ma (mading087)

----------
Reviewed the code and agreed with most of your changes:
- the removal of the history parts is fine
- using lock on the config updates is a lot better than locking around the loading of tls config file

However, think the lock around decrementing the reference count may be needed when the -- operation is not atomic.
Based on Intel document:
INC and DEC belong to the family of instructions that can read, modify, and write a data value in memory. Thus, their operation is not guaranteed to be atomic unless the LOCK prefix is used for these instructions (when referencing a location in memory). The XCHG instruction automatically causes the LOCK behavior to occur regardless of whether the prefix is used or not.

Here is an example of race condition for the ref_count decrement in tls_h_tcpconn_clean(). Assuming 2 threads are trying to tear down two connections, the correct way would be 
thread1 (ref_count 2->1), then thread2 (ref_count 1->0)
But if thread2 reads the ref_count before thread1 writes, you'd get
thread1 (ref_count 2->1) and thread2 (ref_count 2->1). The result would be wrong, and could cause dangling memory block. Realistically, this would be a rare case, but can still happen. 

Volatile just tells the compiler not to optimize the ref_count, won't guarantee atomicity. Think your suggestion of using atomic_t type and atomic ops for the ref_count is the absolute right fix. The potential issue with atomic_t is that it would cause some extra work when porting kamailio to non-linux or linux with older kernel. The other alternative would be to use lock around the ref_count decrement, which would have impact on performance when there are a large number of IP phones in the system. 

If you agree with my assessment, I can take a shoot at changing the ref_count to atomic_t, and provide that as another patch on top of your changes. Or you can make this change and give me a patch, which I can test for you in a real system. let me know whichever way works for you.

If possible, wonder if it would be better to merge the current patch plus your changes to the stable main line. This would address the original memory leak issue for 99.9% of the cases. The atomic_t change can come later. Thanks.

----------

More information can be found at the following URL:
https://sip-router.org/tracker/index.php?do=details&task_id=380#comment1292

You are receiving this message because you have requested it from the Flyspray bugtracking system.  If you did not expect this message or don't want to receive mails in future, you can change your notification settings at the URL shown above.