Bug Report -- sorry if duplicate my first attempt at sending my email was not working.

[Joshua Moore-Oliva <jo...@chatgris.com>]

Currently in apr_pool_destroy there is this chunk of code

    while (pool->child)
        apr_pool_destroy(pool->child);

  run_cleanups(&pool->cleanups);


The problem here is if I created a subpool for temporary scratch space, then 
registered that subpool as a cleanup the pool will be cleaned up in the first 
chunk of code.

  while (pool->child)
        apr_pool_destroy(pool->child);

  THEN will be run again with

  run_cleanups(&pool->cleanups);

  causing a segmentation fault.


This code here Fixed the problem by running the cleanups first then destroying 
the subpools.


  run_cleanups(&pool->cleanups);


    while (pool->child)
        apr_pool_destroy(pool->child);



Sample pseudo//simplified code that causes the problem is below for further 
clarification.

apr_status_t file_field_clean( t ) {
  apr_pool_destroy( t->read_pool );
  apr_pool_destroy( t->write_pool );
  free(t);

  return APR_SUCCESS;
}

struct ffm_t file_field_init( apr_pool_t * pool ) {
  struct ffm_t * t;

  t = malloc( sizeof( struct ffm_t ) );

  if ( apr_pool_create( &(t->read_pool), pool ) != APR_SUCCESS ) {
    return NULL;
  }

  if ( apr_pool_create( &(t->write_pool), pool ) != APR_SUCCESS ) {
    return NULL;
  }

  apr_pool_cleanup_register( pool, t, file_field_clean, file_field_clean );

  return t;
}

Re: Bug Report -- sorry if duplicate my first attempt at sending my email was not working.

[Joshua Moore-Oliva <jo...@chatgris.com>]

Yep, thanks and I should have clarified in my response that I wasn't sure 
whether it was for earlier reasons or just a we've already set a spec.

Josh.

On July 10, 2003 06:26 pm, Cliff Woolley wrote:
> On Thu, 10 Jul 2003, Joshua Moore-Oliva wrote:
> > Regardless of it's usefulness, it is something that people can do.  I do
> > not see any performance penalty by running the cleanups before clearing
> > the subpools, and it eliminates a possible segmentation fault.
>
> Actually it would cause a lot more segfaults if you changed it.  The
> reason I asked you to just take my word for it before is that these
> cleanup problems are horrendously complicated and intertwined and hard to
> explain, not because I was trying to say "we've already decided it and you
> have no input."
>
> Here's one example: Let's say you have an object a in pool p and a
> childpool q that has an object b (which refers to object a) in it.  If you
> destroy a before you cleanup pool q, then the cleanup for b will run after
> things it depends on from object a are already destroyed.
>
> So the pool cleanup order is always LIFO... we always guarantee that when
> q's cleanups run, everything in p still exists.  If that were not the
> case, then it would be very difficult to cleanup things in q.
>
> Does that help clarify?
>
> --Cliff

Re: Bug Report -- sorry if duplicate my first attempt at sending my email was not working.

[Cliff Woolley <jw...@virginia.edu>]

On Thu, 10 Jul 2003, Joshua Moore-Oliva wrote:

> Regardless of it's usefulness, it is something that people can do.  I do not
> see any performance penalty by running the cleanups before clearing the
> subpools, and it eliminates a possible segmentation fault.

Actually it would cause a lot more segfaults if you changed it.  The
reason I asked you to just take my word for it before is that these
cleanup problems are horrendously complicated and intertwined and hard to
explain, not because I was trying to say "we've already decided it and you
have no input."

Here's one example: Let's say you have an object a in pool p and a
childpool q that has an object b (which refers to object a) in it.  If you
destroy a before you cleanup pool q, then the cleanup for b will run after
things it depends on from object a are already destroyed.

So the pool cleanup order is always LIFO... we always guarantee that when
q's cleanups run, everything in p still exists.  If that were not the
case, then it would be very difficult to cleanup things in q.

Does that help clarify?

--Cliff

Re: Bug Report -- sorry if duplicate my first attempt at sending my email was not working.

[Joshua Moore-Oliva <jo...@chatgris.com>]

<snip>
> All you have to do is get rid of those two apr_pool_destroy() lines and it
> will work fine.  You can go ahead and free(t) in your cleanup function.
> Then when you cleanup "pool", read_pool and write_pool will be cleaned up
> and then t will be freed.
>
> The point is that you should never register a cleanup in a parent pool
> that calls apr_pool_destroy() on a child pool.  It's already done
> implicitly.

I do understand that it is done implicitly.  However, I do believe that this 
small change should be done. So I shall state my case and be done with it. 
(this is all IMHO, you guys have a great project )

Regardless of it's usefulness, it is something that people can do.  I do not 
see any performance penalty by running the cleanups before clearing the 
subpools, and it eliminates a possible segmentation fault.  Why leave 
something in the code that can be a trip-up when there is no reason to 
eliminate the possible bug in another persons program? (saying that the bug 
is technically in the persons program, but why allow there to be a bug at 
all? )

Anyways, I have changed my code to work with the current design.

Josh.

Re: Bug Report -- sorry if duplicate my first attempt at sending my email was not working.

[Cliff Woolley <jw...@virginia.edu>]

On Thu, 10 Jul 2003, Joshua Moore-Oliva wrote:

> This code here Fixed the problem by running the cleanups first then destroying
> the subpools.
>
>   run_cleanups(&pool->cleanups);
>
>     while (pool->child)
>         apr_pool_destroy(pool->child);

-1 .. the order is correct as-is by design, I promise.  :)

> Sample pseudo//simplified code that causes the problem is below for further
> clarification.
>
> apr_status_t file_field_clean( t ) {
>   apr_pool_destroy( t->read_pool );
>   apr_pool_destroy( t->write_pool );
>   free(t);
>
>   return APR_SUCCESS;
> }

All you have to do is get rid of those two apr_pool_destroy() lines and it
will work fine.  You can go ahead and free(t) in your cleanup function.
Then when you cleanup "pool", read_pool and write_pool will be cleaned up
and then t will be freed.

The point is that you should never register a cleanup in a parent pool
that calls apr_pool_destroy() on a child pool.  It's already done
implicitly.

--Cliff