Hi Miklos,
On 10-09 18:42, Miklos Tirpak wrote:
Hi Jan,
when free_cell() frees the memory of a transaction the shm memory lock is already held:
shm_lock(); ... /* callbacks */ for( cbs=(struct tm_callback*)dead_cell->tmcb_hl.first ; cbs ; ) { cbs_tmp = cbs; cbs = cbs->next; if (cbs_tmp->release) { cbs_tmp->release(cbs_tmp->param); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I think this can cause a dead-lock because the release function is not aware of the state of the shm mem lock.
I am not sure I understand. The release function assumes that it is called from within the lock, so it knows about it, right?
} shm_free_unsafe( cbs_tmp ); }
I saw that you have added this function call, but I cannot found any customer of this function in the repository, so I do not know whether the cb functions use safe or unsafe shm_free(). Do you know anything about this?
The release function was added there on request. It was needed by the kamailio version of dialog module. See modules_k/dialog, function unref_new_dialog is the handle that is called from the code above.
Unfortunately I do not know much about the dialog module.
I recently added another place where this cb is called from (without locking), hence I think it would be better to move this outside of the shm mem lock to be on the safe side.
Why is the shm_lock there? Is this some sort of performance optimization to ensure that as little as possible is done with the cell lock being?
I think you are right that the release callabacks should not be executed with the shm_lock being held, because the callbacks can execute arbitrary complex functions and those functions might not be aware that they need to use the non-locking version of shm related functions.
So feel free to change it it is safe.
Jan.