Everything

php

Home » Project » Newsgroups » php.internals » [PATCH] fix pcre global variable abuse »

[PATCH] fix pcre global variable abuse

php.internals

Joe Orton

22 years ago

This fixes #121454: the pcre extension should not change the global
allocation callbacks for pcre.

PCRE is used inside httpd and may be used by modules other than PHP too;
having these modules use PHP allocation functions doesn't seem at all
sensible, and could mess up the memory limit accounting presumably.

The cause of #121454 is that during a restart, libphp4.so is unloaded
from memory, but the global variable pcre_malloc is left pointing at
php_pcre_malloc; so when httpd uses pcre, it all goes boom.

Alternative fix might be to use a shutdown function in the extension
which does "pcre_malloc = malloc; pcre_free = free;" but I think it's
wiser just to stay well clear of the issue.

--- php-4.3.6/ext/pcre/php_pcre.c.pcrealloc
+++ php-4.3.6/ext/pcre/php_pcre.c
@@ -47,20 +47,6 @@

ZEND_DECLARE_MODULE_GLOBALS(pcre)

-
-static void *php_pcre_malloc(size_t size)
-{
-	return pemalloc(size, 1);
-}
-
-
-static void php_pcre_free(void *ptr)
-{
-	if (ptr)
-	pefree(ptr, 1);
-}
-
-
static void php_free_pcre_cache(void *data)
{
pcre_cache_entry *pce = (pcre_cache_entry *) data;
@@ -107,14 +93,6 @@
REGISTER_LONG_CONSTANT("PREG_SPLIT_OFFSET_CAPTURE", PREG_SPLIT_OFFSET_CAPTURE, CONST_CS | CONST_PERSISTENT);
REGISTER_LONG_CONSTANT("PREG_GREP_INVERT", PREG_GREP_INVERT, CONST_CS | CONST_PERSISTENT);

-	pcre_malloc = php_pcre_malloc;
-	pcre_free = php_pcre_free;
-
-#ifdef NO_RECURSE
-	pcre_stack_malloc = php_pcre_malloc;
-	pcre_stack_free = php_pcre_free;
-#endif
-
return SUCCESS;
}
/* }}} */
@@ -548,7 +526,7 @@
}
}

-				php_pcre_free((void *) stringlist);
+				pcre_free((void *) stringlist);
}
}
else { /* Failed to match */

Joe Orton

22 years ago

On Thu, Apr 22, 2004 at 11:02:18AM +0100, Joe Orton wrote:

> This fixes #121454: the pcre extension should not change the global
> allocation callbacks for pcre.

Oops, wrong bug and even wrong bug database.  I meant #27810.

Andi Gutmans

22 years ago

I think changing back to malloc/free on RSHUTDOWN is a better solution.
After all, we use our memory manager to prevent memory leaks and I think
this is one of PHP's most important features. I don't trust third party
extensions :)

Andi

At 11:02 AM 4/22/2004 +0100, Joe Orton wrote:

Ilia A.

22 years ago

On April 23, 2004 10:01 am, Andi Gutmans wrote:

> I think changing back to malloc/free on RSHUTDOWN is a better solution.
> After all, we use our memory manager to prevent memory leaks and I think
> this is one of PHP's most important features. I don't trust third party
> extensions :)

The php_pcre_(free|malloc) use persistent allocation routines with the
persistent flag set. Iif I am reading the code correctly, this is nothing
more then a wrapper around malloc() free() without any sort of ZE memory
management. If that is indeed the case, it would seem that Joe's solution
would be better, since besides fixing the problem it eliminates pointless
wrapper.

Ilia

Andi Gutmans

22 years ago

At 10:05 AM 4/23/2004 -0400, Ilia Alshanetsky wrote:

>On April 23, 2004 10:01 am, Andi Gutmans wrote:
> > I think changing back to malloc/free on RSHUTDOWN is a better solution.
> > After all, we use our memory manager to prevent memory leaks and I think
> > this is one of PHP's most important features. I don't trust third party
> > extensions :)
>
>The php_pcre_(free|malloc) use persistent allocation routines with the
>persistent flag set. Iif I am reading the code correctly, this is nothing
>more then a wrapper around malloc() free() without any sort of ZE memory
>management. If that is indeed the case, it would seem that Joe's solution
>would be better, since besides fixing the problem it eliminates pointless
>wrapper.

I was sure it was using emalloc/efree. Is there any good reason it isn't?
Does PCRE require persistent memory?
If it does, then I agree with you that malloc/free is the right way to go.
Andi

Sterling Hughes

22 years ago

i'm pretty sure it does, it has a compiled regex cache that uses it, i
think.

-sterling

On Apr 23, 2004, at 7:11 AM, Andi Gutmans wrote:

Rasmus Lerdorf

22 years ago

I don't understand why this would have to be in RSHUTDOWN.  We are talking
about an Apache restart here so MSHUTDOWN should be sufficient.

-Rasmus

On Fri, 23 Apr 2004, Andi Gutmans wrote:

Andi Gutmans

22 years ago

If we were to use emalloc/efree and PHP would leave these at the end of the
request, then some other Apache module which uses PCRE would be using PHP's
emalloc/efree. I don't think it has anything to do with Apache restart.

Andi

At 07:37 AM 4/23/2004 -0700, Rasmus Lerdorf wrote:

Rasmus Lerdorf

22 years ago

On Fri, 23 Apr 2004, Andi Gutmans wrote:

> If we were to use emalloc/efree and PHP would leave these at the end of the
> request, then some other Apache module which uses PCRE would be using PHP's
> emalloc/efree. I don't think it has anything to do with Apache restart.

Which should be fine since the PHP function is there and just maps to
malloc/free anyway.  The problem on a reload is that PHP gets unloaded so
any other module that uses pcre while PHP is unloaded will crash.  So I
think the specific bug we are talking about is very much related to an
Apache restart.  Of course, the way it is written, it is silly for it to
use the pemalloc wrapper.

-Rasmus

Andi Gutmans

22 years ago

At 07:43 AM 4/23/2004 -0700, Rasmus Lerdorf wrote:

>On Fri, 23 Apr 2004, Andi Gutmans wrote:
> > If we were to use emalloc/efree and PHP would leave these at the end of the
> > request, then some other Apache module which uses PCRE would be using PHP's
> > emalloc/efree. I don't think it has anything to do with Apache restart.
>
>Which should be fine since the PHP function is there and just maps to
>malloc/free anyway.  The problem on a reload is that PHP gets unloaded so
>any other module that uses pcre while PHP is unloaded will crash.  So I
>think the specific bug we are talking about is very much related to an
>Apache restart.  Of course, the way it is written, it is silly for it to
>use the pemalloc wrapper.

You didn't understand what I'm saying. If we were to use emalloc/efree
(which we don't but we should if possible) and if we wouldn't replace them
with malloc/free on RSHUTDOWN, then some other Apache module which handles
the next request (mod_perl for example) would call our emalloc/efree. As
the memory manager wouldn't be initialized this would most likely crash.

Andi

Rasmus Lerdorf

22 years ago

On Fri, 23 Apr 2004, Andi Gutmans wrote:

> At 07:43 AM 4/23/2004 -0700, Rasmus Lerdorf wrote:
> >On Fri, 23 Apr 2004, Andi Gutmans wrote:
> > > If we were to use emalloc/efree and PHP would leave these at the end of the
> > > request, then some other Apache module which uses PCRE would be using PHP's
> > > emalloc/efree. I don't think it has anything to do with Apache restart.
> >
> >Which should be fine since the PHP function is there and just maps to
> >malloc/free anyway.  The problem on a reload is that PHP gets unloaded so
> >any other module that uses pcre while PHP is unloaded will crash.  So I
> >think the specific bug we are talking about is very much related to an
> >Apache restart.  Of course, the way it is written, it is silly for it to
> >use the pemalloc wrapper.
>
> You didn't understand what I'm saying. If we were to use emalloc/efree
> (which we don't but we should if possible) and if we wouldn't replace them
> with malloc/free on RSHUTDOWN, then some other Apache module which handles
> the next request (mod_perl for example) would call our emalloc/efree. As
> the memory manager wouldn't be initialized this would most likely crash.

But it always uses the persistent flag, so where is the memory manager
issue?  pemalloc(foo,1) is going to map straight to malloc(foo) is it not?

The way ext/pcre works is that it uses emalloc/efree for the
non-persistent allocs directly but it registers pemalloc/pefree for the
persistent memory handling.  The correct fix here is to just use
malloc/free for the persistent stuff.

-Rasmus

Andi Gutmans

22 years ago

At 07:49 AM 4/23/2004 -0700, Rasmus Lerdorf wrote:

> > You didn't understand what I'm saying. If we were to use emalloc/efree
> > (which we don't but we should if possible) and if we wouldn't replace them
> > with malloc/free on RSHUTDOWN, then some other Apache module which handles
> > the next request (mod_perl for example) would call our emalloc/efree. As
> > the memory manager wouldn't be initialized this would most likely crash.
>
>But it always uses the persistent flag, so where is the memory manager
>issue?  pemalloc(foo,1) is going to map straight to malloc(foo) is it not?
>
>The way ext/pcre works is that it uses emalloc/efree for the
>non-persistent allocs directly but it registers pemalloc/pefree for the
>persistent memory handling.  The correct fix here is to just use
>malloc/free for the persistent stuff.

Never mind. I don't feel like explaining myself a thousand times :) If
you'd have read what I wrote you would have seen that I wasn't saying that
we're using emalloc/efree (and thus the memory manager) today.
I was talking about the PCRE library and not the PCRE extension. But in any
case, as PCRE is caching compiled regexes in between requests I don't think
we can make the library use emalloc/efree.
Thus, it seems that the only option is to set the callbacks to malloc/free.

Andi

Rasmus Lerdorf

22 years ago

On Fri, 23 Apr 2004, Andi Gutmans wrote:

> At 07:49 AM 4/23/2004 -0700, Rasmus Lerdorf wrote:
>
> > > You didn't understand what I'm saying. If we were to use emalloc/efree
> > > (which we don't but we should if possible) and if we wouldn't replace them
> > > with malloc/free on RSHUTDOWN, then some other Apache module which handles
> > > the next request (mod_perl for example) would call our emalloc/efree. As
> > > the memory manager wouldn't be initialized this would most likely crash.
> >
> >But it always uses the persistent flag, so where is the memory manager
> >issue?  pemalloc(foo,1) is going to map straight to malloc(foo) is it not?
> >
> >The way ext/pcre works is that it uses emalloc/efree for the
> >non-persistent allocs directly but it registers pemalloc/pefree for the
> >persistent memory handling.  The correct fix here is to just use
> >malloc/free for the persistent stuff.
>
> Never mind. I don't feel like explaining myself a thousand times :) If
> you'd have read what I wrote you would have seen that I wasn't saying that
> we're using emalloc/efree (and thus the memory manager) today.
> I was talking about the PCRE library and not the PCRE extension. But in any
> case, as PCRE is caching compiled regexes in between requests I don't think
> we can make the library use emalloc/efree.
> Thus, it seems that the only option is to set the callbacks to malloc/free.

I guess I have no idea what you are talking about.  The bug we are
discussing is that the PCRE extension registers pemalloc/pefree as the
cache malloc/free functions.  On a restart these PHP wrappers go away and
we crash when another module hits PCRE while PHP is unloaded.  It is as
simple as that.  There is no memory manager issue anywhere in this bug.

As far as I can tell you suggested to reset the pemalloc/pefree hook in
the RSHUTDOWN and I simply pointed out that MSHUTDOWN would be sufficient
if we wanted to keep using the wrapper.  Granted, that would mean other
modules using PCRE would be using PHP's wrappers for some reason which
would technically work.

-Rasmus

Andrei Zmievski

22 years ago

On Fri, 23 Apr 2004, Rasmus Lerdorf wrote:

> I guess I have no idea what you are talking about.  The bug we are
> discussing is that the PCRE extension registers pemalloc/pefree as the
> cache malloc/free functions.  On a restart these PHP wrappers go away and
> we crash when another module hits PCRE while PHP is unloaded.  It is as
> simple as that.  There is no memory manager issue anywhere in this bug.

They are _not_ cache malloc/free functions. They are used during the
request to obtain additional memory for computation.

- Andrei

Rasmus Lerdorf

22 years ago

On Fri, 23 Apr 2004, Andrei Zmievski wrote:

> On Fri, 23 Apr 2004, Rasmus Lerdorf wrote:
> > I guess I have no idea what you are talking about.  The bug we are
> > discussing is that the PCRE extension registers pemalloc/pefree as the
> > cache malloc/free functions.  On a restart these PHP wrappers go away and
> > we crash when another module hits PCRE while PHP is unloaded.  It is as
> > simple as that.  There is no memory manager issue anywhere in this bug.
>
> They are _not_ cache malloc/free functions. They are used during the
> request to obtain additional memory for computation.

You mean they are not generic memory alloc functions?  I know that.  But
they map to malloc/free which is the important point here.

-Rasmus

Andrei Zmievski

22 years ago

On Fri, 23 Apr 2004, Rasmus Lerdorf wrote:

> You mean they are not generic memory alloc functions?  I know that.  But
> they map to malloc/free which is the important point here.

The global variables  pcre_malloc  and  pcre_free  initially
contain the entry points of the standard malloc() and free()
functions respectively. PCRE  calls  the  memory  management
functions  via  these  variables,  so  a calling program can
replace them if it  wishes  to  intercept  the  calls.  This
should be done before calling any PCRE functions.

pcre_malloc() is then called at various points to obtain additional
memory.

- Andrei

Rasmus Lerdorf

22 years ago

On Fri, 23 Apr 2004, Andrei Zmievski wrote:

> On Fri, 23 Apr 2004, Rasmus Lerdorf wrote:
> > You mean they are not generic memory alloc functions?  I know that.  But
> > they map to malloc/free which is the important point here.
>
>      The global variables  pcre_malloc  and  pcre_free  initially
>      contain the entry points of the standard malloc() and free()
>      functions respectively. PCRE  calls  the  memory  management
>      functions  via  these  variables,  so  a calling program can
>      replace them if it  wishes  to  intercept  the  calls.  This
>      should be done before calling any PCRE functions.
>
> pcre_malloc() is then called at various points to obtain additional
> memory.

Right, sorry, more precisely, by setting them to pemalloc/pefree they
result in calling malloc/free anyway.  Not a direct mapping of course.
But I think we have beaten this one to death.  Since PHP just calls
malloc/free anyway and the wrapper layer is causing problems, simply
removing the php layer is the obvious answer.

-Rasmus

Andi Gutmans

22 years ago

I know what you were talking about. I was contemplating of making the PCRE
library use emalloc/efree (a completely different issue), but arrived at
the conclusion that it wouldn't work because of the compiled regex caching
we do.
*A completely different issue*!!!

Andi

At 08:09 AM 4/23/2004 -0700, Rasmus Lerdorf wrote:

Rasmus Lerdorf

22 years ago

On Fri, 23 Apr 2004, Andi Gutmans wrote:

> I know what you were talking about. I was contemplating of making the PCRE
> library use emalloc/efree (a completely different issue), but arrived at
> the conclusion that it wouldn't work because of the compiled regex caching
> we do.
> *A completely different issue*!!!

Ok, I was confused then.  I have re-read the message I replied to a few
times now and don't see where you allude to that, but ok.

http://news.php.net/article.php?group=php.internals&article=9498

-Rasmus

Andrei Zmievski

22 years ago

On Fri, 23 Apr 2004, Andi Gutmans wrote:

> Never mind. I don't feel like explaining myself a thousand times :) If
> you'd have read what I wrote you would have seen that I wasn't saying that
> we're using emalloc/efree (and thus the memory manager) today.
> I was talking about the PCRE library and not the PCRE extension. But in any
> case, as PCRE is caching compiled regexes in between requests I don't think
> we can make the library use emalloc/efree.
> Thus, it seems that the only option is to set the callbacks to malloc/free.

Or omit them at all. They are set to malloc/free by default anyway.
These callbacks are used only when PCRE library needs additional memory
during a long/large computation.

The regex cache is implemented via direct calls to pemalloc()/pefree(),
without using those callbacks.

- Andrei