Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: neutral (athena.apache.org: local policy)
Date: Sat, 11 Sep 2010 01:34:20 +0100
From: Nick Kew <niq@apache.org>
To: dev@apr.apache.org
Subject: Re: [PATCH] HUP on bug 45930 (CGI and signal masks)
Message-ID: <20100911013420.6cd67f68@baldur>
In-Reply-To: <20100909003732.GA13715@chiark.greenend.org.uk>
References: <20100909003732.GA13715@chiark.greenend.org.uk>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Thu, 9 Sep 2010 01:37:32 +0100
John Sullivan <johns@chiark.greenend.org.uk> wrote:


> I originally submitted against the specific httpd version I reproduced
> and tested the patch under. It was then reassigned to APR HEAD,
> presumably by a httpd reviewer because that's where the necessary
> code changes are.

OK, I've reviewed it in more detail.

Your basic point is well-made: the signal mask should be reset before calling
an external program.  However, this is not a threads vs no threads issue, and
if we're to fix it, we should reset signals in threaded and non-threaded builds
alike.  The fact that it only affects threaded MPMs in HTTPD is coincidental.

You've actually highlighted rather more issues in threadproc/unix/signals.c.
There's some rather confused use of constructs like
#if !APR_HAS_THREADS 
	sigprocmask
#else
	thread_sigmask
#endif 

within code that is entirely wrapped in #if APR_HAS_THREADS
Also some int values from system functions are being returned as apr_status_t
so I expect there are edge-case bugs lurking in the error handling.

Pending a more complete review of signals.c, (time permitting) I'm not going
to apply this patch.  Sorry.  Not your fault I'm not happy!

-- 
Nick Kew