Received: (at submit) by bugs.debian.org; 13 Mar 2001 14:24:13 +0000
From viro@math.psu.edu Tue Mar 13 08:24:13 2001
Return-path: <viro@math.psu.edu>
Received: from leibniz.math.psu.edu (math.psu.edu) [146.186.130.2] 
	by master.debian.org with esmtp (Exim 3.12 1 (Debian))
	id 14cpie-0007Wo-00; Tue, 13 Mar 2001 08:24:12 -0600
Received: from weyl.math.psu.edu (weyl.math.psu.edu [146.186.130.226])
	by math.psu.edu (8.9.3/8.9.3) with ESMTP id JAA29665
	for <submit@bugs.debian.org>; Tue, 13 Mar 2001 09:24:07 -0500 (EST)
Received: from localhost (viro@localhost)
	by weyl.math.psu.edu (8.9.3/8.9.3) with ESMTP id JAA00617
	for <submit@bugs.debian.org>; Tue, 13 Mar 2001 09:24:07 -0500 (EST)
X-Authentication-Warning: weyl.math.psu.edu: viro owned process doing -bs
Date: Tue, 13 Mar 2001 09:24:07 -0500 (EST)
From: Alexander Viro <viro@math.psu.edu>
To: submit@bugs.debian.org
Subject: races galore in wmaker signal handling (segfaults)
Message-ID: <Pine.GSO.4.21.0103130840360.298-100000@weyl.math.psu.edu>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Delivered-To: submit@bugs.debian.org

Package: wmaker
Version: 0.61.1-4
Severity: normal

Summary: guys who had written wmaker signal handlers need to be
bashed with Stevens until the clue gets in.

	a) You can't call malloc(3) from the handler.
	b) You can't modify linked lists that are also modified from
the main code, unless main code blocks signals in critical areas.

Both rules are violated in wmaker. All versions, up to the current
one from upstream CVS. As the result, wmaker merrily segfaults if
signals happen in the wrong time (usually upon exit when we get a lot
of SIGCHLD, but it's just a matter of probability),

For 0.61.1-4:
* src/startup.c::handleSig() contains the following:
       if (!WDelayedActionSet) {
            WDelayedActionSet = 1;
            WMAddIdleHandler(delayedAction, NULL);
        }
WMAddIdleHandler() starts with calling malloc(). 'Nuff said. Instant
segfault if SIGCHLD comes in the middle of malloc(). Observed in the wild.

* Same code contains another race: WMAddIdleHandler() adds an entry into
the linked list. Quite possibly right in the middle of manipulations with
the same list (see WINGs/wevent.c). Our motto is "Segfaults Now"...

* handleSig() again, look at the WCHANGE_STATE(). Think how much good
it does if signal comes when main code is going to do WCHANGE_STATE()
itself.

	I don't see how to fix it without changes to wevent.c API.
We could preallocate structure for delayed action handler so that
handleSig() wouldn't have to allocate anything, but then we need a
new function (WMAddPreallocedHandler()). List manipulations outside
of the handleSig() probably need to be protected by blocking SIGCHLD, but
it can be too costly if we touch these lists often enough. WCHANGE_STATE()
either needs callers to block signals, or (more likely) should be
eliminated from handleSig() in favour of some sort of delayed handling
a-la SIGCHLD.

	Code is in bad need of audit for potential races - authors had
demonstrated complete lack of clue in writing async code. Sigh...