[asterisk-bugs] [JIRA] (ASTERISK-22960) [patch] Segfaults in res_musiconhold.c:moh_release

[Michael L. Young (JIRA)]

    [ https://issues.asterisk.org/jira/browse/ASTERISK-22960?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=212823#comment-212823 ] 

Michael L. Young commented on ASTERISK-22960:
---------------------------------------------

I would be curious to see that backtrace as well.  Especially, since in the attached patch I see the reference count being incremented on state->class which is something that was noticed when working on ASTERISK-21775.  There is a patch on that issue (ASTERISK-21775) which added a reference to this object but it didn't seem to solve that issue for the reporter.
                
> [patch] Segfaults in res_musiconhold.c:moh_release
> --------------------------------------------------
>
>                 Key: ASTERISK-22960
>                 URL: https://issues.asterisk.org/jira/browse/ASTERISK-22960
>             Project: Asterisk
>          Issue Type: Bug
>      Security Level: None
>          Components: Resources/res_musiconhold
>    Affects Versions: 11.6.0
>         Environment: Debian, using Realtime MusicOnHold with a MySQL database
>            Reporter: Ben Smithurst
>            Assignee: Ben Smithurst
>         Attachments: ASTERISK-22960-res_musiconhold_moh_release.diff
>
>
> We have been seeing a reasonably frequent segfault in res_musiconhold, in the moh_release() function.  This was with realtime musiconhold, stores in mysql, with cachertclasses disabled (potentially relevant to the fix).
> After a bit of debugging and testing we have come up with the attached patch.  There are two parts to this fix. 
> ast_activate_generator is passed a class and a channel, to activate MOH on that channel. If the channel already has MOH, it is disabled, before allocating the new instance. However what I think can happen is that if the channel's instances is the same as the one to activate for some reason, and the channel is the last reference to that class, the object will be destroyed, and allocating a new instance of it is then invalid.  Using MALLOC_DEBUG makes this more obvious, as the object is full of {{0xdeaddead}}.
> The fix changes the order to allocate a new instance first, then unallocate from the channel, before allocating the new instance to the channel. 
> local_ast_moh_start checks if the class already has a MOH class saved via state->class, and if so, it decrements the reference of the local variable mohclass, before assigning mohclass = state->class. However this function will then unref mohclass at the end, I think this can take the ref count of state->class (which was assigned to mohclass) too low, so I have changed it to increment the ref count of state->class when doing that assignment.  This fixes it for us but I may have misunderstood something to do with reference counting in this code.
> These appear to work, and running with REF_DEBUG enabled didn't show reference numbers going crazy, and when I terminated my test calls I saw MOH objects being destroyed as expected. 
> We have been able to reproduce this by creating a simple queue with a few extensions as members, and then using AMI to create a load of calls into the queue, and just dumping the other side of the call into something like echo test.  Normally with enough calls this will make Asterisk crash within a few minutes.
> We haven't yet tested with cachertclasses enabled, looking at the code, that may make a difference, but we decided to chase the bug rather than try to hide it by changing settings.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.asterisk.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira