res_musiconhold.c - Race Condition

18 views
Skip to first unread message

Gustavo B

unread,
Nov 11, 2010, 1:05:07 PM11/11/10
to asteris...@googlegroups.com
Estimados,
                Estuvimos teniendo unas caídas en nuestro Asterisk RSP y pudimos ver que el problema que se estaba generando era un race condition en res_musiconhold.c el patch lo tengo publicado en mi Blog (http://asteriskhelp.blogspot.com/) con una brebe explicación de lo que estaba sucediendo. De todos modos, aquí tienen un link al DIFF donde se puede ver el código que lo soluciona: http://svnview.digium.com/svn/asterisk/branches/1.4/res/res_musiconhold.c?view=diff&r1=260344&r2=260345&pathrev=260345

Saludos

Gustavo Borgoni
http://asteriskhelp.blogspot.com

Saúl Ibarra Corretgé

unread,
Nov 11, 2010, 3:52:38 PM11/11/10
to asteris...@googlegroups.com
Hola,

2010/11/11 Gustavo B <ngb...@gmail.com>:


> Estimados,
>                 Estuvimos teniendo unas caídas en nuestro Asterisk RSP y
> pudimos ver que el problema que se estaba generando era un race condition en
> res_musiconhold.c el patch lo tengo publicado en mi Blog
> (http://asteriskhelp.blogspot.com/) con una brebe explicación de lo que
> estaba sucediendo. De todos modos, aquí tienen un link al DIFF donde se
> puede ver el código que lo soluciona:
> http://svnview.digium.com/svn/asterisk/branches/1.4/res/res_musiconhold.c?view=diff&r1=260344&r2=260345&pathrev=260345
>

Gracias por el análisis! Podrías mandar el parche en formato diff en
un mail a la lista?

Saludos,

--
/Saúl
http://saghul.net | http://sipdoc.net

Gustavo B

unread,
Nov 11, 2010, 3:55:44 PM11/11/10
to asteris...@googlegroups.com
Saúl,
       Aquí va el parche.

--- res/res_musiconhold.c       2010/04/30 19:53:36     260344
+++ res/res_musiconhold.c       2010/04/30 20:08:15     260345
@@ -288,7 +288,15 @@
        state->sample_queue += samples;

        while (state->sample_queue > 0) {
+               ast_channel_lock(chan);
                if ((f = moh_files_readframe(chan))) {
+                       /* We need to be sure that we unlock
+                        * the channel prior to calling
+                        * ast_write. Otherwise, the recursive locking
+                        * that occurs can cause deadlocks when using
+                        * indirect channels, like local channels
+                        */
+                       ast_channel_unlock(chan);
                        state->samples += f->samples;
                        state->sample_queue -= f->samples;
                        res = ast_write(chan, f);
@@ -297,8 +305,10 @@
                                ast_log(LOG_WARNING, "Failed to write frame to '%s': %s\n", chan->name, strerror(errno));
                                return -1;
                        }
-               } else
+               } else {
+                       ast_channel_unlock(chan);
                        return -1;
+               }
        }
        return res;
 }
@@ -1054,12 +1064,14 @@
        ast_clear_flag(chan, AST_FLAG_MOH);
        ast_deactivate_generator(chan);

+       ast_channel_lock(chan);
        if (chan->music_state) {
                if (chan->stream) {
                        ast_closestream(chan->stream);
                        chan->stream = NULL;
                }
        }
+       ast_channel_unlock(chan);
 }

 static void moh_class_destructor(void *obj)

Saludos



--
Has recibido este mensaje porque estás suscrito al grupo "asterisk-es-rsp" de Grupos de Google.
Para publicar una entrada en este grupo, envía un correo electrónico a asteris...@googlegroups.com.
Para anular tu suscripción a este grupo, envía un correo electrónico a asterisk-es-r...@googlegroups.com
Para tener acceso a más opciones, visita el grupo en http://groups.google.com/group/asterisk-es-rsp?hl=es.




--
Gustavo Borgoni
http://asteriskhelp.blogspot.com

Saúl Ibarra Corretgé

unread,
Nov 11, 2010, 3:59:38 PM11/11/10
to asteris...@googlegroups.com
Attach please :-)

2010/11/11 Gustavo B <ngb...@gmail.com>:

Gustavo B

unread,
Nov 11, 2010, 4:04:29 PM11/11/10
to asteris...@googlegroups.com
Perdón! ahí va adjunto

Saludos

2010/11/11 Saúl Ibarra Corretgé <sag...@gmail.com>
patch res_musiconhold.diff

Saúl Ibarra Corretgé

unread,
Nov 12, 2010, 3:22:23 AM11/12/10
to asteris...@googlegroups.com
2010/11/11 Gustavo B <ngb...@gmail.com>:
> Perdón! ahí va adjunto
>

Gracias Gustavo. He mirado el svn log y el reporte de este bug lo hizo
un cliente de Digium para la Asterisk Business Edition. Pinta bien así
que lo añadiré a lo largo del fin de semana.

saghul@hal:~/work/asterisk-branch-1.4$ svn log -c 260345
------------------------------------------------------------------------
r260345 | mmichelson | 2010-04-30 22:08:15 +0200 (Fri, 30 Apr 2010) | 18 lines

Fix potential crash from race condition due to accessing channel data
without the channel locked.

In res_musiconhold.c, there are several places where a channel's
stream's existence is checked prior to calling ast_closestream on it. The issue
here is that in several cases, the channel was not locked while checking the
stream. The result was that if two threads checked the state of the channel's
stream at approximately the same time, then there could be a situation where
both threads attempt to call ast_closestream on the channel's stream. The result
here is that the refcount for the stream would go below 0, resulting in a crash.

I have added proper channel locking to res_musiconhold.c to ensure that
we do not try to check chan->stream without the channel locked. A
Digium customer
has been using this patch for several weeks and has not had any crashes since
applying the patch.

ABE-2147

Gustavo B

unread,
Nov 12, 2010, 8:39:54 AM11/12/10
to asteris...@googlegroups.com
Excelente!!! espero que salga todo bien. Suerte!

2010/11/12 Saúl Ibarra Corretgé <sag...@gmail.com>
--
Has recibido este mensaje porque estás suscrito al grupo "asterisk-es-rsp" de Grupos de Google.
Para publicar una entrada en este grupo, envía un correo electrónico a asteris...@googlegroups.com.
Para anular tu suscripción a este grupo, envía un correo electrónico a asterisk-es-r...@googlegroups.com
Para tener acceso a más opciones, visita el grupo en http://groups.google.com/group/asterisk-es-rsp?hl=es.

Saúl Ibarra Corretgé

unread,
Nov 14, 2010, 2:41:19 PM11/14/10
to asteris...@googlegroups.com
On Fri, Nov 12, 2010 at 2:39 PM, Gustavo B <ngb...@gmail.com> wrote:
> Excelente!!! espero que salga todo bien. Suerte!
>

Probado y commiteado en la revisión 312, gracias!

Gustavo B

unread,
Nov 14, 2010, 8:11:16 PM11/14/10
to asteris...@googlegroups.com
excelente!! =)

--
Has recibido este mensaje porque estás suscrito al grupo "asterisk-es-rsp" de Grupos de Google.
Para publicar una entrada en este grupo, envía un correo electrónico a asteris...@googlegroups.com.
Para anular tu suscripción a este grupo, envía un correo electrónico a asterisk-es-r...@googlegroups.com
Para tener acceso a más opciones, visita el grupo en http://groups.google.com/group/asterisk-es-rsp?hl=es.

Reply all
Reply to author
Forward
0 new messages