[sr-dev] shm_lock in free_cell()

[Jan Janak]

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.