Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:69230
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.212.41 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <523A047B.3040509@gmail.com>
References: <CAAPSnn21kq2_-4RGoMKW_8eWDNNv_0Aon30SRsp3U_v+2098AA@mail.gmail.com>
	<523A047B.3040509@gmail.com>
Date: Thu, 19 Sep 2013 11:51:11 +0200
Message-ID: <CAAPSnn3qn92oq+ra1X+7r4GBSjgpmKKroc7ZirmOhxko-1q4FQ@mail.gmail.com>
To: =?ISO-8859-1?Q?=C1ngel_Gonz=E1lez?= <keisial@gmail.com>
Cc: internals@lists.php.net
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] free deadlock in timeout signal handler
From: lazy404@gmail.com (Lazy)

2013/9/18 =C1ngel Gonz=E1lez <keisial@gmail.com>:
> 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=3Dphp-internals&m=3D121999390109071&w=3D2 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=3D31749
>
> 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.

time() refers to  https://bugs.php.net/bug.php?id=3D31749, You are right
this deadlock is related to free()

>> 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) =3D type;
>>      846         PG(last_error_message) =3D 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 duri=
ng
> the whole execution and extended with mmap(2) in the unlikely case it tur=
ns
> out to be too small. I suspect it would also make the error handler faste=
r,
> as it would avoid the free() + malloc()

thank You I didn't notice that it is used only there, i will try to
use a static buffer.

I managed to produce a segfault on current php version (php heap
corruption), bug report https://bugs.php.net/bug.php?id=3D65674
spprintf() allocating memory is also not safe. I will try to fix this
by using a static buffer.

Thanks,

Michal Grzedzicki