You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by dave b <db...@gmail.com> on 2010/09/01 06:07:25 UTC
rational behind not checking the return value of apr_palloc and apr_pcalloc
What is the rational behind not checking the return value of
apr_palloc and apr_pcalloc?
code memory/unix/apr_pools.c from apr-1.4.2
APR_DECLARE(void *) apr_pcalloc(apr_pool_t *pool, apr_size_t size);
APR_DECLARE(void *) apr_pcalloc(apr_pool_t *pool, apr_size_t size)
{
void *mem;
if ((mem = apr_palloc(pool, size)) != NULL) {
memset(mem, 0, size);
}
return mem;
}
and
apr_palloc can return NULL.
So I modified the code and the testdir test failed in one place ->
" node = active->next;
if (size <= node_free_space(node)) {
list_remove(node);
}
else {
if ((node = allocator_alloc(pool->allocator, size)) == NULL) {
if (pool->abort_fn)
pool->abort_fn(APR_ENOMEM); /* HERE */
return NULL;
}
}
When you run the testdir (test). If you change the above to be:
.....
if ((node = allocator_alloc(pool->allocator, size)) == NULL) {
if (! pool->abort_fn) /* note the ! added */
pool->abort_fn(APR_ENOMEM);
return NULL; /* you end up here */
}
}
and you will fail one of the tests. This to me suggests that this scenario is
possible if the pool is like that one failed test *but* pool->abort_fn is not
true :)
"
So what is the rational behind most users of these method *not*
checking the return code - because from what I have seen / know it is
possible return NULL.
Also see: https://issues.apache.org/bugzilla/show_bug.cgi?id=49847
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by dave b <db...@gmail.com>.
On 2 September 2010 13:29, William A. Rowe Jr. <wr...@rowe-clan.net> wrote:
> On 9/1/2010 10:17 PM, dave b wrote:
>>
>> Why not just fix it now and not worry? ...
>
> It will help if you can provide a specific use case for graceful failure.
>
> A segfault/dereference of NULL pointer provides a very specific exception
> for this general case, and 'recovers' neatly from a process which has simply
> consumed far too much memory. There are very few alloc exceptions which are
> recoverable a multi-client application.
>
> But if you can illustrate a few, the community is happy to evaluate your
> examples, which is what Jeff has politely suggested to you.
>
>
http://blog.ksplice.com/2010/03/null-pointers-part-i/
So what if NULL doesn't crash the app ^^ ?
https://www.securecoding.cert.org/confluence/display/seccode/EXP34-C.+Do+not+dereference+null+pointers
I can't see MAP_FIXED anywhere in APR or apache code though.
I am just suggesting it would be wise to prevent this from occurring.
Not every platform / linux has /proc/sys/vm/mmap_min_addr .
Sure it is not likely to be null, but it could be :)
--
It is a wise father that knows his own child. -- William Shakespeare,
"The Merchant of Venice"
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by HyperHacker <hy...@gmail.com>.
On Fri, Sep 3, 2010 at 13:24, dave b <db...@gmail.com> wrote:
>> "first the attacker has to find a way to reduce system memory to an
>> almost oom condition"
>> Say, by attacking several httpd threads and/or unrelated processes to
>> get them to eat up memory.
>>
>> --
>> Sent from my toaster.
>>
>
> If you know something why not share it ;) ?
> imho Apache is pretty good - so perhaps you could find a commonly used
> module that leaks memory?
>
> Also, I hope your toaster is running netbsd with apache ^^
>
> --
> As flies to wanton boys are we to the gods; they kill us for their
> sport. -- Shakespeare, "King Lear"
>
Just tossing around ideas. What's the threshold for killing these
child processes? What prevents someone from bringing several to just
below that threshold?
--
Sent from my toaster.
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by dave b <db...@gmail.com>.
> "first the attacker has to find a way to reduce system memory to an
> almost oom condition"
> Say, by attacking several httpd threads and/or unrelated processes to
> get them to eat up memory.
>
> --
> Sent from my toaster.
>
If you know something why not share it ;) ?
imho Apache is pretty good - so perhaps you could find a commonly used
module that leaks memory?
Also, I hope your toaster is running netbsd with apache ^^
--
As flies to wanton boys are we to the gods; they kill us for their
sport. -- Shakespeare, "King Lear"
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by Graham Leggett <mi...@sharp.fm>.
On 03 Sep 2010, at 3:58 PM, HyperHacker wrote:
> "first the attacker has to find a way to reduce system memory to an
> almost oom condition"
> Say, by attacking several httpd threads and/or unrelated processes to
> get them to eat up memory.
At which point the child processes are terminated, and httpd spawns
new children to replace them.
If an attacker has a way to trigger an OOM condition, that is a
separate problem completely unrelated to the behavior of apr_pcalloc().
Regards,
Graham
--
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by HyperHacker <hy...@gmail.com>.
On Fri, Sep 3, 2010 at 07:12, Graham Leggett <mi...@sharp.fm> wrote:
> On 03 Sep 2010, at 2:37 PM, HyperHacker wrote:
>
>> ...assuming he attacks a single httpd thread, as opposed to say a
>> distributed attack or attack on an unrelated process.
>
> How would a distributed attack be different?
>
> Obviously an attack on an unrelated process would have nothing to do with
> checking the return value of apr_pcalloc().
>
> Regards,
> Graham
> --
>
>
"first the attacker has to find a way to reduce system memory to an
almost oom condition"
Say, by attacking several httpd threads and/or unrelated processes to
get them to eat up memory.
--
Sent from my toaster.
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by Graham Leggett <mi...@sharp.fm>.
On 03 Sep 2010, at 2:37 PM, HyperHacker wrote:
> ...assuming he attacks a single httpd thread, as opposed to say a
> distributed attack or attack on an unrelated process.
How would a distributed attack be different?
Obviously an attack on an unrelated process would have nothing to do
with checking the return value of apr_pcalloc().
Regards,
Graham
--
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by HyperHacker <hy...@gmail.com>.
On Fri, Sep 3, 2010 at 03:49, Graham Leggett <mi...@sharp.fm> wrote:
> On 03 Sep 2010, at 5:31 AM, dave b wrote:
>
>> Sure ok :)
>> You have no complains from me really here. Just this could be an issue
>> on some platform with some mods potentially :)
>
> In order to understand why it isn't an issue for httpd, you need to
> understand how httpd works.
>
> httpd has a thin parent process, which is responsible for spawning children
> as required to do the actual work. Those children doing the actual work are
> expendable, and if the child process dies for any reason, the parent process
> will spawn new child processes to replace them.
>
> This is the fundamental reason why it is so difficult to crash an httpd
> server, because your module code only has the power to crash one child
> process. If a single child process goes bananas and tries to allocate all
> the RAM, that child will be terminated and replaced.
>
>> I only asked this list because the mod_wsgi guy wasn't checking the
>> result of memory allocation. The rational as I see it is: there is
>> only a few cases where this can happen 1: and 2: first the attacker
>> has to find a way to reduce system memory to an almost oom condition
>> by the looks of it.
>
> If the attacker has found a way to create an OOM condition, that child
> process will terminate and be replaced, and the attacker would have caused
> no lasting damage.
>
> Regards,
> Graham
> --
>
>
...assuming he attacks a single httpd thread, as opposed to say a
distributed attack or attack on an unrelated process.
--
Sent from my toaster.
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by Graham Leggett <mi...@sharp.fm>.
On 03 Sep 2010, at 5:31 AM, dave b wrote:
> Sure ok :)
> You have no complains from me really here. Just this could be an issue
> on some platform with some mods potentially :)
In order to understand why it isn't an issue for httpd, you need to
understand how httpd works.
httpd has a thin parent process, which is responsible for spawning
children as required to do the actual work. Those children doing the
actual work are expendable, and if the child process dies for any
reason, the parent process will spawn new child processes to replace
them.
This is the fundamental reason why it is so difficult to crash an
httpd server, because your module code only has the power to crash one
child process. If a single child process goes bananas and tries to
allocate all the RAM, that child will be terminated and replaced.
> I only asked this list because the mod_wsgi guy wasn't checking the
> result of memory allocation. The rational as I see it is: there is
> only a few cases where this can happen 1: and 2: first the attacker
> has to find a way to reduce system memory to an almost oom condition
> by the looks of it.
If the attacker has found a way to create an OOM condition, that child
process will terminate and be replaced, and the attacker would have
caused no lasting damage.
Regards,
Graham
--
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by dave b <db...@gmail.com>.
> And if you can't illustrate a few explicit cases, further abstract arguments
> are likely to be politely, but firmly, ignored. There are good C language
> forums for folks to carry on such religious arguments.
>
> Or to put it another way, the dev@ group here is most certainly not worried
> about the general case, as the current design is effective at terminating
> httpd when faced with runaway allocations.
Sure ok :)
You have no complains from me really here. Just this could be an issue
on some platform with some mods potentially :)
I only asked this list because the mod_wsgi guy wasn't checking the
result of memory allocation. The rational as I see it is: there is
only a few cases where this can happen 1: and 2: first the attacker
has to find a way to reduce system memory to an almost oom condition
by the looks of it.
--
I dote on his very absence. -- William Shakespeare, "The Merchant of Venice"
Re: rational behind not checking the return value of apr_palloc and
apr_pcalloc
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 9/1/2010 10:29 PM, William A. Rowe Jr. wrote:
> On 9/1/2010 10:17 PM, dave b wrote:
>>
>> Why not just fix it now and not worry? ...
>
> But if you can illustrate a few, the community is happy to evaluate your
> examples, which is what Jeff has politely suggested to you.
And if you can't illustrate a few explicit cases, further abstract arguments
are likely to be politely, but firmly, ignored. There are good C language
forums for folks to carry on such religious arguments.
Or to put it another way, the dev@ group here is most certainly not worried
about the general case, as the current design is effective at terminating
httpd when faced with runaway allocations.
Re: rational behind not checking the return value of apr_palloc and
apr_pcalloc
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 9/1/2010 10:17 PM, dave b wrote:
>
> Why not just fix it now and not worry? ...
It will help if you can provide a specific use case for graceful failure.
A segfault/dereference of NULL pointer provides a very specific exception
for this general case, and 'recovers' neatly from a process which has simply
consumed far too much memory. There are very few alloc exceptions which are
recoverable a multi-client application.
But if you can illustrate a few, the community is happy to evaluate your
examples, which is what Jeff has politely suggested to you.
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by dave b <db...@gmail.com>.
> no, I don't want you to do anything for me; I'm just sharing my educated
> guess at what it takes to make progress on this topic you're apparently very
> interested in
>
> with a little luck you'll be able to find somebody here to analyze the code
> you pointed out to see which cases actually matter, and to what extent
>
Why not just fix it now and not worry? ...
--
For a light heart lives long. -- Shakespeare, "Love's Labour's Lost"
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by Jeff Trawick <tr...@gmail.com>.
On Wed, Sep 1, 2010 at 4:09 PM, dave b <db...@gmail.com> wrote:
> >
> > My 2 cents:
> >
> > I doubt that any of the core devs are going to match you for devotion to
> > this topic, but I'm sure we will review patches to trunk to fix somewhat
> > practical scenarios, such as ensuring that memory allocation failures
> during
> > request processing go through the common abort function, if the existing
> > code doesn't already handle that. (OTOH, I wouldn't think many would
> find
> > addressing un-aborted alloc failures during initialization so
> interesting.)
>
> Sorry I have a habit of measuring things twice and cutting once ;)
>
>
> > That requires some investigation/understanding of the context of the code
> > that passes NULL for the parent pool, which hopefully you can make an
> > attempt at without dumping the grep output and selected source code to
> the
> > mailing list ;)
> > (I know that sounds rude, but its my best advice to pursuing that thread
> of
> > investigation.)
>
> It doesn't sound rude. It sounds like you want me to do the work for you.
>
no, I don't want you to do anything for me; I'm just sharing my educated
guess at what it takes to make progress on this topic you're apparently very
interested in
with a little luck you'll be able to find somebody here to analyze the code
you pointed out to see which cases actually matter, and to what extent
> The code dump and grep was to give some examples.
> I think without the code dump some would have problems following that
> the global_pool abort_fn is not to abort when it would otherwise
> return NULL ( I believe this to be the case atm at least :) ).
>
>
> > What's the big picture here from your standpoint? Why is 2.2 noticeably
> > impaired without such changes? Are there really many users who need that
> > abort function to tell them that httpd can't alloc more heap? Anyway, if
> > 2.3 doesn't really have this suitably addressed it would be better to get
> > that finished first.
>
> I would need to do a lot more testing and I don't have much time for
> that presently. I will have a poke around later in a vm after making
> some modifications to the code base to try and trigger somethings :)
> IMHO the apr init function, which inits the global_pool, could take in
> an argument for the default abort_fn.
>
>
> >>
> >> Also, note I hit that code path via apr testdir (in one of the tests
> >> ;) ) - and I was not out of memory - I haven't checked which test it
> >> is yet (it would be nice if the apr tests told me this...).
> >> If the caller has hit that code path and they haven't defined an
> >> abort_fn, I think it best to exit / abort for them - If a user wants
> >> not to have that happen then they can just simply ensure they are
> >> providing their own abort_fn :)
> >
> > dev@apr.apache.org, but first try gdb or testall -v or anything else
> that
> > you need to find exactly what/where the failure is
>
> ah there is a -v !
>
> --
> The ripest fruit falls first. -- William Shakespeare, "Richard
> II"
>
--
Born in Roswell... married an alien...
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by dave b <db...@gmail.com>.
>
> My 2 cents:
>
> I doubt that any of the core devs are going to match you for devotion to
> this topic, but I'm sure we will review patches to trunk to fix somewhat
> practical scenarios, such as ensuring that memory allocation failures during
> request processing go through the common abort function, if the existing
> code doesn't already handle that. (OTOH, I wouldn't think many would find
> addressing un-aborted alloc failures during initialization so interesting.)
Sorry I have a habit of measuring things twice and cutting once ;)
> That requires some investigation/understanding of the context of the code
> that passes NULL for the parent pool, which hopefully you can make an
> attempt at without dumping the grep output and selected source code to the
> mailing list ;)
> (I know that sounds rude, but its my best advice to pursuing that thread of
> investigation.)
It doesn't sound rude. It sounds like you want me to do the work for you.
The code dump and grep was to give some examples.
I think without the code dump some would have problems following that
the global_pool abort_fn is not to abort when it would otherwise
return NULL ( I believe this to be the case atm at least :) ).
> What's the big picture here from your standpoint? Why is 2.2 noticeably
> impaired without such changes? Are there really many users who need that
> abort function to tell them that httpd can't alloc more heap? Anyway, if
> 2.3 doesn't really have this suitably addressed it would be better to get
> that finished first.
I would need to do a lot more testing and I don't have much time for
that presently. I will have a poke around later in a vm after making
some modifications to the code base to try and trigger somethings :)
IMHO the apr init function, which inits the global_pool, could take in
an argument for the default abort_fn.
>>
>> Also, note I hit that code path via apr testdir (in one of the tests
>> ;) ) - and I was not out of memory - I haven't checked which test it
>> is yet (it would be nice if the apr tests told me this...).
>> If the caller has hit that code path and they haven't defined an
>> abort_fn, I think it best to exit / abort for them - If a user wants
>> not to have that happen then they can just simply ensure they are
>> providing their own abort_fn :)
>
> dev@apr.apache.org, but first try gdb or testall -v or anything else that
> you need to find exactly what/where the failure is
ah there is a -v !
--
The ripest fruit falls first. -- William Shakespeare, "Richard II"
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by Jeff Trawick <tr...@gmail.com>.
On Wed, Sep 1, 2010 at 2:49 PM, dave b <db...@gmail.com> wrote:
> On 1 September 2010 22:08, Jeff Trawick <tr...@gmail.com> wrote:
> > On Wed, Sep 1, 2010 at 6:37 AM, Graham Dumpleton
> > <gr...@gmail.com> wrote:
> >>
> >> On 1 September 2010 20:15, Graham Leggett <mi...@sharp.fm> wrote:
> >> > On 01 Sep 2010, at 6:07 AM, dave b wrote:
> >> >
> >> >> What is the rational behind not checking the return value of
> >> >> apr_palloc and apr_pcalloc?
> >> >
> >> > The rationale is to not be forced to check for and handle hundreds of
> >> > potential failure cases when you're probably doomed anyway.
>
> Yes I agree :)
> Hence, why the patch I put in the bug report used assert :)
>
>
> >> > The APR pools API gives you the apr_pool_abort_set() function, which
> >> > specifies a function to call if the memory allocation fails. In the
> case
> >> > of
> >> > httpd, a function is registered which gracefully shuts down that
> >> > particular
> >> > server process if the allocation fails, and apr_palloc() is in the
> >> > process
> >> > guaranteed to never return NULL.
> >>
> >> Noting that apr_pool_abort_set() is only setup in Apache 2.3 and not
> >> in Apache 2.2.16 or earlier. Not being in 2.X explains why I couldn't
> >> find it before.
> >>
> >> Any reason why setting up apr_pool_abort_set() wasn't back ported to
> >> Apache 2.2?
> >
> > dunno, but here's the commit and original discussion in case anyone wants
> to
> > read
> > http://svn.apache.org/viewvc?view=revision&revision=406953 (dunno if it
> was
> > subsequently fixed)
> > http://www.mail-archive.com/dev@httpd.apache.org/msg32257.html
>
>
> Interesting patch, um but... I think there maybe a problem here:
>
> from apr :)
> include/apr_pools.h
>
>
> #if defined(DOXYGEN)
> APR_DECLARE(apr_status_t) apr_pool_create(apr_pool_t **newpool,
> apr_pool_t *parent);
> #else
> #if APR_POOL_DEBUG
> #define apr_pool_create(newpool, parent) \
> apr_pool_create_ex_debug(newpool, parent, NULL, NULL, \
> APR_POOL__FILE_LINE__)
> #else
> #define apr_pool_create(newpool, parent) \
> apr_pool_create_ex(newpool, parent, NULL, NULL)
> #endif
> #endif
>
> And apr_pool_create_ex is:
>
>
> APR_DECLARE(apr_status_t) apr_pool_create_ex(apr_pool_t **newpool,
> apr_pool_t *parent,
> apr_abortfunc_t abort_fn,
> apr_allocator_t *allocator);
>
>
>
> First thing to note is that in main apr_app_initialize[1] in
> server/main.c via init_process. When this happens the global_pool will
> not have a abort_fn. *However*, cntx will (it is set :) ),
> process->pconf will.
>
> Now we look in the actual code :)
> See [0]
> Right so if apr_pool_create gets called with null as the parent from
> now on - there will be no default abort_fn.
> Lets go hunting for this happens ;) see [2]
>
> Ok so lets pull out the scoreboard now ?
>
> /* ToDo: This function should be made to handle setting up
> * a scoreboard shared between processes using any IPC technique,
> * not just a shared memory segment
> */
> static apr_status_t open_scoreboard(apr_pool_t *pconf)
> {
> #if APR_HAS_SHARED_MEMORY
> apr_status_t rv;
> char *fname = NULL;
> apr_pool_t *global_pool;
>
> /* We don't want to have to recreate the scoreboard after
> * restarts, so we'll create a global pool and never clean it.
> */
> rv = apr_pool_create(&global_pool, NULL);
>
>
> MMM I wonder where else httpd does these kind of things ;)
> Remember my simple grep can *easily* miss some :)
>
My 2 cents:
I doubt that any of the core devs are going to match you for devotion to
this topic, but I'm sure we will review patches to trunk to fix somewhat
practical scenarios, such as ensuring that memory allocation failures during
request processing go through the common abort function, if the existing
code doesn't already handle that. (OTOH, I wouldn't think many would find
addressing un-aborted alloc failures during initialization so interesting.)
That requires some investigation/understanding of the context of the code
that passes NULL for the parent pool, which hopefully you can make an
attempt at without dumping the grep output and selected source code to the
mailing list ;)
(I know that sounds rude, but its my best advice to pursuing that thread of
investigation.)
> Ok great just get the rest of world to use apache 2.3 which has had
> this patch for over 4 years now. Just one problem: I don't see anyone
> running apache 2.3 alpha 8 on their production servers.
>
> So perhaps apache 2.2 should get a backport of this 'fix' :-)
> Apparently my fix that I suggested on the bug referenced in my
> original email is also not going to work :) (it will not be enough :-)
> )
>
What's the big picture here from your standpoint? Why is 2.2 noticeably
impaired without such changes? Are there really many users who need that
abort function to tell them that httpd can't alloc more heap? Anyway, if
2.3 doesn't really have this suitably addressed it would be better to get
that finished first.
>
> Also, note I hit that code path via apr testdir (in one of the tests
> ;) ) - and I was not out of memory - I haven't checked which test it
> is yet (it would be nice if the apr tests told me this...).
> If the caller has hit that code path and they haven't defined an
> abort_fn, I think it best to exit / abort for them - If a user wants
> not to have that happen then they can just simply ensure they are
> providing their own abort_fn :)
>
dev@apr.apache.org, but first try gdb or testall -v or anything else that
you need to find exactly what/where the failure is
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by dave b <db...@gmail.com>.
On 1 September 2010 22:08, Jeff Trawick <tr...@gmail.com> wrote:
> On Wed, Sep 1, 2010 at 6:37 AM, Graham Dumpleton
> <gr...@gmail.com> wrote:
>>
>> On 1 September 2010 20:15, Graham Leggett <mi...@sharp.fm> wrote:
>> > On 01 Sep 2010, at 6:07 AM, dave b wrote:
>> >
>> >> What is the rational behind not checking the return value of
>> >> apr_palloc and apr_pcalloc?
>> >
>> > The rationale is to not be forced to check for and handle hundreds of
>> > potential failure cases when you're probably doomed anyway.
Yes I agree :)
Hence, why the patch I put in the bug report used assert :)
>> > The APR pools API gives you the apr_pool_abort_set() function, which
>> > specifies a function to call if the memory allocation fails. In the case
>> > of
>> > httpd, a function is registered which gracefully shuts down that
>> > particular
>> > server process if the allocation fails, and apr_palloc() is in the
>> > process
>> > guaranteed to never return NULL.
>>
>> Noting that apr_pool_abort_set() is only setup in Apache 2.3 and not
>> in Apache 2.2.16 or earlier. Not being in 2.X explains why I couldn't
>> find it before.
>>
>> Any reason why setting up apr_pool_abort_set() wasn't back ported to
>> Apache 2.2?
>
> dunno, but here's the commit and original discussion in case anyone wants to
> read
> http://svn.apache.org/viewvc?view=revision&revision=406953 (dunno if it was
> subsequently fixed)
> http://www.mail-archive.com/dev@httpd.apache.org/msg32257.html
Interesting patch, um but... I think there maybe a problem here:
from apr :)
include/apr_pools.h
#if defined(DOXYGEN)
APR_DECLARE(apr_status_t) apr_pool_create(apr_pool_t **newpool,
apr_pool_t *parent);
#else
#if APR_POOL_DEBUG
#define apr_pool_create(newpool, parent) \
apr_pool_create_ex_debug(newpool, parent, NULL, NULL, \
APR_POOL__FILE_LINE__)
#else
#define apr_pool_create(newpool, parent) \
apr_pool_create_ex(newpool, parent, NULL, NULL)
#endif
#endif
And apr_pool_create_ex is:
APR_DECLARE(apr_status_t) apr_pool_create_ex(apr_pool_t **newpool,
apr_pool_t *parent,
apr_abortfunc_t abort_fn,
apr_allocator_t *allocator);
First thing to note is that in main apr_app_initialize[1] in
server/main.c via init_process. When this happens the global_pool will
not have a abort_fn. *However*, cntx will (it is set :) ),
process->pconf will.
Now we look in the actual code :)
See [0]
Right so if apr_pool_create gets called with null as the parent from
now on - there will be no default abort_fn.
Lets go hunting for this happens ;) see [2]
Ok so lets pull out the scoreboard now ?
/* ToDo: This function should be made to handle setting up
* a scoreboard shared between processes using any IPC technique,
* not just a shared memory segment
*/
static apr_status_t open_scoreboard(apr_pool_t *pconf)
{
#if APR_HAS_SHARED_MEMORY
apr_status_t rv;
char *fname = NULL;
apr_pool_t *global_pool;
/* We don't want to have to recreate the scoreboard after
* restarts, so we'll create a global pool and never clean it.
*/
rv = apr_pool_create(&global_pool, NULL);
MMM I wonder where else httpd does these kind of things ;)
Remember my simple grep can *easily* miss some :)
Ok great just get the rest of world to use apache 2.3 which has had
this patch for over 4 years now. Just one problem: I don't see anyone
running apache 2.3 alpha 8 on their production servers.
So perhaps apache 2.2 should get a backport of this 'fix' :-)
Apparently my fix that I suggested on the bug referenced in my
original email is also not going to work :) (it will not be enough :-)
)
Also, note I hit that code path via apr testdir (in one of the tests
;) ) - and I was not out of memory - I haven't checked which test it
is yet (it would be nice if the apr tests told me this...).
If the caller has hit that code path and they haven't defined an
abort_fn, I think it best to exit / abort for them - If a user wants
not to have that happen then they can just simply ensure they are
providing their own abort_fn :)
"A little less conversation, a little more action please."
[0]
APR_DECLARE(apr_status_t) apr_pool_create_ex(apr_pool_t **newpool,
apr_pool_t *parent,
apr_abortfunc_t abort_fn,
apr_allocator_t *allocator)
{
apr_pool_t *pool;
apr_memnode_t *node;
*newpool = NULL;
if (!parent)
parent = global_pool;
/* parent will always be non-NULL here except the first time a
* pool is created, in which case allocator is guaranteed to be
* non-NULL. */
if (!abort_fn && parent)
abort_fn = parent->abort_fn;
if (allocator == NULL)
allocator = parent->allocator;
if ((node = allocator_alloc(allocator,
MIN_ALLOC - APR_MEMNODE_T_SIZE)) == NULL) {
if (abort_fn)
abort_fn(APR_ENOMEM);
return APR_ENOMEM;
}
node->next = node;
node->ref = &node->next;
pool = (apr_pool_t *)node->first_avail;
node->first_avail = pool->self_first_avail = (char *)pool + SIZEOF_POOL_T;
pool->allocator = allocator;
pool->active = pool->self = node;
pool->abort_fn = abort_fn;
pool->child = NULL;
pool->cleanups = NULL;
pool->free_cleanups = NULL;
pool->pre_cleanups = NULL;
pool->subprocesses = NULL;
pool->user_data = NULL;
pool->tag = NULL;
#ifdef NETWARE
pool->owner_proc = (apr_os_proc_t)getnlmhandle();
#endif /* defined(NETWARE) */
if ((pool->parent = parent) != NULL) {
#if APR_HAS_THREADS
apr_thread_mutex_t *mutex;
if ((mutex = apr_allocator_mutex_get(parent->allocator)) != NULL)
apr_thread_mutex_lock(mutex);
#endif /* APR_HAS_THREADS */
if ((pool->sibling = parent->child) != NULL)
pool->sibling->ref = &pool->sibling;
parent->child = pool;
pool->ref = &parent->child;
#if APR_HAS_THREADS
if (mutex)
apr_thread_mutex_unlock(mutex);
#endif /* APR_HAS_THREADS */
}
else {
pool->sibling = NULL;
pool->ref = NULL;
}
*newpool = pool;
return APR_SUCCESS;
}
[1]
static int initialized = 0;
APR_DECLARE(apr_status_t) apr_initialize(void)
{
apr_pool_t *pool;
apr_status_t status;
if (initialized++) {
return APR_SUCCESS;
}
#if !defined(BEOS) && !defined(OS2)
apr_proc_mutex_unix_setup_lock();
apr_unix_setup_time();
#endif
if ((status = apr_pool_initialize()) != APR_SUCCESS)
return status;
if (apr_pool_create(&pool, NULL) != APR_SUCCESS) {
return APR_ENOPOOL;
}
apr_pool_tag(pool, "apr_initialize");
/* apr_atomic_init() used to be called from here aswell.
* Pools rely on mutexes though, which can be backed by
* atomics. Due to this circular dependency
* apr_pool_initialize() is taking care of calling
* apr_atomic_init() at the correct time.
*/
apr_signal_init(pool);
return APR_SUCCESS;
}
[2]
(obviously not all of these are hits and probably there are lots more
around ;) )
grep "apr_pool_create" * -R |grep "NULL" -
httpd/server/main.c: stat = apr_pool_create(&cntx, NULL);
httpd/server/mpm/event/event.c:
apr_pool_create_ex(&ptrans, pconf, NULL, allocator);
httpd/server/mpm/simple/simple_io.c: apr_pool_create(&ptrans, NULL);
httpd/server/mpm/winnt/child.c: rv =
apr_pool_create_ex(&context->ptrans, pchild, NULL,
httpd/server/mpm/winnt/nt_eventlog.c: apr_pool_create_ex(&p, NULL,
NULL, NULL);
httpd/server/mpm/winnt/mpm_winnt.c: apr_pool_create_ex(&ptemp, p,
NULL, NULL);
httpd/server/mpm/worker/worker.c:
apr_pool_create_ex(&ptrans, pconf, NULL, allocator);
httpd/server/mpm/netware/mpm_netware.c: apr_pool_create_ex(&ptrans,
pmain, NULL, allocator);
httpd/server/mpm/prefork/prefork.c: apr_pool_create_ex(&pchild,
pconf, NULL, allocator);
httpd/server/scoreboard.c: rv = apr_pool_create(&global_pool, NULL);
httpd/support/ab.c: apr_pool_create(&cntxt, NULL);
httpd/support/httxt2dbm.c: apr_pool_create(&pool, NULL);
httpd/support/rotatelogs.c: apr_pool_create(&status.pool, NULL);
httpd/support/htcacheclean.c: if (apr_pool_create(&pool, NULL) !=
APR_SUCCESS) {
httpd/support/htdigest.c: apr_pool_create(&cntxt, NULL);
httpd/support/htpasswd.c: apr_pool_create(&pool, NULL);
httpd/support/htdbm.c: apr_pool_create( pool, NULL);
httpd/support/fcgistarter.c: apr_pool_create(&pool, NULL);
httpd/support/logresolve.c: if (apr_pool_create(&pool, NULL) !=
APR_SUCCESS) {
httpd/modules/slotmem/mod_slotmem_shm.c: rv =
apr_pool_create(&global_pool, NULL);
httpd/modules/arch/win32/mod_isapi.c:
apr_pool_create_ex(&loaded.pool, pconf, NULL, NULL);
httpd/modules/ldap/util_ldap.c: if (apr_pool_create(&newpool,
NULL) != APR_SUCCESS) {
--
Every cloud engenders not a storm. -- William Shakespeare, "Henry VI"
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by Jeff Trawick <tr...@gmail.com>.
On Wed, Sep 1, 2010 at 6:37 AM, Graham Dumpleton <graham.dumpleton@gmail.com
> wrote:
> On 1 September 2010 20:15, Graham Leggett <mi...@sharp.fm> wrote:
> > On 01 Sep 2010, at 6:07 AM, dave b wrote:
> >
> >> What is the rational behind not checking the return value of
> >> apr_palloc and apr_pcalloc?
> >
> > The rationale is to not be forced to check for and handle hundreds of
> > potential failure cases when you're probably doomed anyway.
> >
> > The APR pools API gives you the apr_pool_abort_set() function, which
> > specifies a function to call if the memory allocation fails. In the case
> of
> > httpd, a function is registered which gracefully shuts down that
> particular
> > server process if the allocation fails, and apr_palloc() is in the
> process
> > guaranteed to never return NULL.
>
> Noting that apr_pool_abort_set() is only setup in Apache 2.3 and not
> in Apache 2.2.16 or earlier. Not being in 2.X explains why I couldn't
> find it before.
>
> Any reason why setting up apr_pool_abort_set() wasn't back ported to Apache
> 2.2?
>
dunno, but here's the commit and original discussion in case anyone wants to
read
http://svn.apache.org/viewvc?view=revision&revision=406953 (dunno if it was
subsequently fixed)
http://www.mail-archive.com/dev@httpd.apache.org/msg32257.html
>
> Graham
>
> > Obviously if you're not using APR from httpd, or if you're writing a
> library
> > that depends on APR, and you haven't set an abort function, NULL will
> > potentially be returned and you should check for and handle that case.
> >
> > Regards,
> > Graham
> > --
> >
> >
>
--
Born in Roswell... married an alien...
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by Graham Dumpleton <gr...@gmail.com>.
On 1 September 2010 20:15, Graham Leggett <mi...@sharp.fm> wrote:
> On 01 Sep 2010, at 6:07 AM, dave b wrote:
>
>> What is the rational behind not checking the return value of
>> apr_palloc and apr_pcalloc?
>
> The rationale is to not be forced to check for and handle hundreds of
> potential failure cases when you're probably doomed anyway.
>
> The APR pools API gives you the apr_pool_abort_set() function, which
> specifies a function to call if the memory allocation fails. In the case of
> httpd, a function is registered which gracefully shuts down that particular
> server process if the allocation fails, and apr_palloc() is in the process
> guaranteed to never return NULL.
Noting that apr_pool_abort_set() is only setup in Apache 2.3 and not
in Apache 2.2.16 or earlier. Not being in 2.X explains why I couldn't
find it before.
Any reason why setting up apr_pool_abort_set() wasn't back ported to Apache 2.2?
Graham
> Obviously if you're not using APR from httpd, or if you're writing a library
> that depends on APR, and you haven't set an abort function, NULL will
> potentially be returned and you should check for and handle that case.
>
> Regards,
> Graham
> --
>
>
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by Jeff Trawick <tr...@gmail.com>.
On Wed, Sep 1, 2010 at 6:15 AM, Graham Leggett <mi...@sharp.fm> wrote:
> On 01 Sep 2010, at 6:07 AM, dave b wrote:
>
> What is the rational behind not checking the return value of
>> apr_palloc and apr_pcalloc?
>>
>
> The rationale is to not be forced to check for and handle hundreds of
> potential failure cases when you're probably doomed anyway.
>
probably more than hundreds ;)
If there's a real world scenario where allocation failures can occur and
must be dealt with more gracefully than segfaulting, I suspect that you can
find a pragmatic way to deal with it much more reliably than relying on each
individual memory allocation to be checked (that will never be implemented
perfectly, and those paths will almost never be exercised anyway). For
example, a plug-in module might be able to confirm (or fail gracefully) in
an early request hook that enough memory is available to handle the expected
types of requests.
Another way to look at it: If somebody had the time to add all those
checks/error paths, their time would be better spent looking for situations
where httpd would use a lot more memory than normal because of the way
external input was received.
If there's not a repeatable real world scenario to address -- IOW you think
they should be checked "just because" -- there probably won't be any
sympathy here. With a particular scenario in hand there may be ideas
forthcoming to deal with the situation, whether internal to the web server
or external.
HTH!
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by Graham Leggett <mi...@sharp.fm>.
On 01 Sep 2010, at 6:07 AM, dave b wrote:
> What is the rational behind not checking the return value of
> apr_palloc and apr_pcalloc?
The rationale is to not be forced to check for and handle hundreds of
potential failure cases when you're probably doomed anyway.
The APR pools API gives you the apr_pool_abort_set() function, which
specifies a function to call if the memory allocation fails. In the
case of httpd, a function is registered which gracefully shuts down
that particular server process if the allocation fails, and
apr_palloc() is in the process guaranteed to never return NULL.
Obviously if you're not using APR from httpd, or if you're writing a
library that depends on APR, and you haven't set an abort function,
NULL will potentially be returned and you should check for and handle
that case.
Regards,
Graham
--
Re: rational behind not checking the return value of apr_palloc and apr_pcalloc
Posted by Graham Dumpleton <gr...@gmail.com>.
On 1 September 2010 14:07, dave b <db...@gmail.com> wrote:
> What is the rational behind not checking the return value of
> apr_palloc and apr_pcalloc?
Specifically here talking about why HTTPD code doesn't check. Ie.,
core server code and modules supplied with HTTPD.
I am clarifying this because he is hitting up on me as to why mod_wsgi
doesn't do it, yet the HTTPD code itself doesn't do it and I am just
following that precedent. So suggested he ask here why there is no
practice of checking for NULL values in HTTPD code when doing
allocations against pools. :-)
Graham
> code memory/unix/apr_pools.c from apr-1.4.2
> APR_DECLARE(void *) apr_pcalloc(apr_pool_t *pool, apr_size_t size);
> APR_DECLARE(void *) apr_pcalloc(apr_pool_t *pool, apr_size_t size)
> {
> void *mem;
>
> if ((mem = apr_palloc(pool, size)) != NULL) {
> memset(mem, 0, size);
> }
>
> return mem;
> }
>
> and
> apr_palloc can return NULL.
> So I modified the code and the testdir test failed in one place ->
>
> " node = active->next;
> if (size <= node_free_space(node)) {
> list_remove(node);
> }
> else {
> if ((node = allocator_alloc(pool->allocator, size)) == NULL) {
> if (pool->abort_fn)
> pool->abort_fn(APR_ENOMEM); /* HERE */
>
> return NULL;
> }
> }
>
> When you run the testdir (test). If you change the above to be:
>
>
> .....
> if ((node = allocator_alloc(pool->allocator, size)) == NULL) {
> if (! pool->abort_fn) /* note the ! added */
> pool->abort_fn(APR_ENOMEM);
>
> return NULL; /* you end up here */
> }
> }
> and you will fail one of the tests. This to me suggests that this scenario is
> possible if the pool is like that one failed test *but* pool->abort_fn is not
> true :)
> "
>
> So what is the rational behind most users of these method *not*
> checking the return code - because from what I have seen / know it is
> possible return NULL.
>
> Also see: https://issues.apache.org/bugzilla/show_bug.cgi?id=49847
>