[Pidgin] #7902: The sighandler() function in pidgin/gtkmain.c doesn't restore a SIGALRM handler.

[Pidgin]

#7902: The sighandler() function in pidgin/gtkmain.c doesn't restore a SIGALRM
handler.
--------------------------------------+-------------------------------------
 Reporter:  quonsto                   |        Owner:  elb         
     Type:  patch                     |       Status:  new         
Milestone:  Patches Needing Review    |    Component:  pidgin (gtk)
  Version:  2.5.3                     |   Resolution:              
 Keywords:  pidgin SIGALRM gstreamer  |  
--------------------------------------+-------------------------------------
Description changed by quonsto:

Old description:

> The sighandler() function in pidgin/gtkmain.c contains code, which looks
> like this:
>
> {{{
> #if defined(USE_GSTREAMER) && !defined(GST_CAN_DISABLE_FORKING)
> /* ... skipped long comment ... */
>         case SIGCHLD:
>                 /* Restore signal catching */
>                 signal(SIGCHLD, sighandler);
>                 alarm(1);
>                 break;
>         case SIGALRM:
> #else
>         case SIGCHLD:
> #endif
>                 clean_pid();
>                 /* Restore signal catching */
>                 signal(SIGCHLD, sighandler);
>                 break;
> }}}
>
> If this code is compiled with macro USE_GSTREAMER defined and
> GST_CAN_DISABLE_FORKING not defined, it may lead to an uncatched SIGALRM
> (and pidgin termination), if the pidgin process receives more than one
> SIGALRM during its lifetime.
>
> The problem is a SIGALRM handler which erroneously restores SIGCHLD
> handler instead of SIGALRM.
>
> The code after '#endif' in this piece of code is good for the case when
> being compiled without USE_GSTREAMER macro, or with
> GST_CAN_DISABLE_FORKING. [[BR]]
> But it doesn't suit otherwise.
>
> The proposed patch is to completely split the mentioned piece of code
> into two cases: one for "defined(USE_GSTREAMER) &&
> !defined(GST_CAN_DISABLE_FORKING)" and the other for the opposite case.
>
> A variant of the patch, implemeting this idea is given below:
>
> {{{
>
> --- gtkmain.c.orig      2008-12-21 03:38:16.000000000 +0300
> +++ gtkmain.c   2008-12-25 20:51:57.949286800 +0300
> @@ -207,13 +207,17 @@
>                 alarm(1);
>                 break;
>         case SIGALRM:
> +               clean_pid();
> +               /* Restore signal catching */
> +               signal(SIGALRM, sighandler);
> +               break;
>  #else
>         case SIGCHLD:
> -#endif
>                 clean_pid();
>                 /* Restore signal catching */
>                 signal(SIGCHLD, sighandler);
>                 break;
> +#endif
>         default:
>                 purple_debug_warning("sighandler", "Caught signal %d\n",
> sig);
>                 purple_core_quit();
>
> }}}
>

> Best regards, K.S.

New description:

 The sighandler() function in pidgin/gtkmain.c contains code, which looks
 like this:

 {{{
 #if defined(USE_GSTREAMER) && !defined(GST_CAN_DISABLE_FORKING)
 /* ... skipped long comment ... */
         case SIGCHLD:
                 /* Restore signal catching */
                 signal(SIGCHLD, sighandler);
                 alarm(1);
                 break;
         case SIGALRM:
 #else
         case SIGCHLD:
 #endif
                 clean_pid();
                 /* Restore signal catching */
                 signal(SIGCHLD, sighandler);
                 break;
 }}}

 If this code is compiled with macro USE_GSTREAMER defined and
 GST_CAN_DISABLE_FORKING not defined, it may lead to an uncatched SIGALRM
 (and pidgin termination), if the pidgin process receives more than one
 SIGALRM during its lifetime.

 The problem is a SIGALRM handler which erroneously restores SIGCHLD
 handler instead of SIGALRM.

 The code after '#endif' in this piece of code is good for the case when
 being compiled without USE_GSTREAMER macro, or with
 GST_CAN_DISABLE_FORKING. [[BR]]
 But it doesn't suit otherwise.

 The proposed patch is to completely split the mentioned piece of code into
 two cases: one for "defined(USE_GSTREAMER) &&
 !defined(GST_CAN_DISABLE_FORKING)" and the other for the opposite case.

 A variant of the patch, implemeting this idea is given below:

 {{{

 --- gtkmain.c.orig      2008-12-21 03:38:16.000000000 +0300
 +++ gtkmain.c   2008-12-25 20:51:57.949286800 +0300
 @@ -207,13 +207,17 @@
                 alarm(1);
                 break;
         case SIGALRM:
 +               clean_pid();
 +               /* Restore signal catching */
 +               signal(SIGALRM, sighandler);
 +               break;
  #else
         case SIGCHLD:
 -#endif
                 clean_pid();
                 /* Restore signal catching */
                 signal(SIGCHLD, sighandler);
                 break;
 +#endif
         default:
                 purple_debug_warning("sighandler", "Caught signal %d\n",
 sig);
                 purple_core_quit();

 }}}

 The case is quite simple and i guess no proof of concept is necessary.
 But to be more vivid, I will add, that the reason for me to pay attention
 to this piece of code was an abnormal termination of pidgin on my Nokia
 N800 IT OS2007.
 It terminated with an ALRM signal soon (about 1-2 seconds) after its
 startup.
 After I grep'd through pidgin sources and found this code and fixed it,
 the problem disappeared.
 To be accurate, i recompiled piding once more without this fix and back
 again it ended up with a SIGALRM.

 The proposed patch is just the one I used to fix pidgin for me.

 Best regards, K.S.

--

-- 
Ticket URL: <http://developer.pidgin.im/ticket/7902#comment:5>
Pidgin <http://pidgin.im>
Pidgin