Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:69213
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.178 as permitted sender)
Message-ID: <523A047B.3040509@gmail.com>
Date: Wed, 18 Sep 2013 21:52:27 +0200
User-Agent: Thunderbird
MIME-Version: 1.0
To: Lazy <lazy404@gmail.com>
CC: internals@lists.php.net
References: <CAAPSnn21kq2_-4RGoMKW_8eWDNNv_0Aon30SRsp3U_v+2098AA@mail.gmail.com>
In-Reply-To: <CAAPSnn21kq2_-4RGoMKW_8eWDNNv_0Aon30SRsp3U_v+2098AA@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] free deadlock in timeout signal handler
From: keisial@gmail.com (=?ISO-8859-1?Q?=C1ngel_Gonz=E1lez?=)

On 13/09/13 22:10, Lazy wrote:
> Hello internals,
>
> I'm trying to fix deadlock in an ancient php 5.2.17, php hangs on
> internal libc lock.
>  From my understanding free is not safe to use in a signal handler, and
> this seems to be the issue here.
No, it's not.

> http://marc.info/?l=php-internals&m=121999390109071&w=2 seems to
> address this issue but it's not present in 5.3 or later releases.
>
> Very similar deadlock but with time() instead of free(),
> https://bugs.php.net/bug.php?id=31749
Are you sure it's with time() ? I do see a free() in that call stack 
(and no time),
as I would expect, as time() is required by POSIX.1-2004 to be 
Async-signal-safe.

> Current php also uses free() in php_error_cb() and external
> destructors in list_entry_destructor() aren't protected by
> HANDLE_BLOCK_INTERRUPTIONS() (which seems to be a noop in fastcgi
> mode), so I suspect 5.5 may also contain this deadlock.
>
> main/main.c
> php_error_cb()
>      835     if (display) {
>      836         if (PG(last_error_message)) {
>      837             free(PG(last_error_message));
> ...                    ^^^^^^ deadlock if previous free was
> interrupted by a timeout signal
>      845         PG(last_error_type) = type;
>      846         PG(last_error_message) = strdup(buffer);
>
> I'm thinking about "fixing" this by leaking memory pointed by
> PG(last_error_message) if php called when a timeout is pending
> (timeouts are usually very rare, php processes will eventually be
> restarted so this little memory waste won't have time to make any
> impact.
>
> Is this issue fixed in modern php ? If so I would be grateful for some
> information about the way it was done. This would save me a lot of
> time trying to trigger
> a non existing confition.
>
> I will try to reproduce this in a modern version of php.

It probably isn't. PG(last_error_message) is only modified in main.c, I 
would try to use a separate arena for PG(last_error_message) and 
PG(last_error_file). For instance it could be a static buffer reused 
during the whole execution and extended with mmap(2) in the unlikely 
case it turns out to be too small. I suspect it would also make the 
error handler faster, as it would avoid the free() + malloc()