[Pidgin] #7902: The sighandler() function in pidgin/gtkmain.c doesn't restore a SIGALRM handler.
Pidgin
trac at pidgin.im
Fri Aug 21 12:36:27 EDT 2009
#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 |
--------------------------------------+-------------------------------------
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();
>
> }}}
>
> 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.
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
SIGCHLD or SIGALRM during its lifetime. (I.e. forking some child processes
in libraries, which terminate shortly, or something alike).
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 it was my pidgin's abnormal
termination that brought my attention to this piece of code.[[BR]]
I am running pidgin on on my Nokia N800 IT OS2007 and it began terminating
with an ALRM signal soon (about 1-2 seconds) after its startup. [[BR]]
I made some investigations and ended up grepping through pidgin sources
which brought me to this piece of code.
After I applied the patch, proposed in this ticket, the problem
disappeared.[[BR]]
To be accurate and to make sure that it's that very piece of code to
blame, I recompiled pidgin once more without my fix and back again I saw
it failing with a SIGALRM.
Best regards, K.S.
--
Comment(by quonsto):
[[BR]]
--
Ticket URL: <http://developer.pidgin.im/ticket/7902#comment:6>
Pidgin <http://pidgin.im>
Pidgin
More information about the Tracker
mailing list