execute_control_command may not remove its cleanups

execute_control_command may not remove its cleanups

[Dave Allan]

Hello,

I ran into a reproducible segfault in gdb (v.5.3, but the offending code
is still present in the CVS tree I checked out this morning).  I traced
the problemto the execute_control_command function in cli/cli-script.c. 

It appears that execute_control_command doesn't always do or discard the
cleanups it creates before returning, which is not right according to
the gdb internals docs.  According to section 13.1 Cleanups, "Your
function should explicitly do or discard the cleanups it creates. 
Failing to do this leads to non-deterministic behavior..."

The problem is that the call to do_cleanups in execute_control_command
is conditional (cli/cli-script.c at line 430):

if (old_chain)
  do_cleanups (old_chain);

So, if the cleanup_chain was null entering execute_control_command, then
old_chain will be null, and the call to do_cleanups doesn't happen.  

Removing the if statement, thus making the do_cleanups (old_chain)
unconditional eliminates the segfault.  

Unfortunately, the segfault occurred while I was using gdb run under the
"crash" kernel dump analysis tool, and it appears to me that under
normal gdb usage, cleanup_chain is never null going into
execute_control_command.  Thus, do_cleanups is always executed and the
segfault never appears and I don't have a reproducible test case that
works against a vanilla build.  

However, it seems from code inspection and the gdb internals
documentation that the call to do_cleanups ought to be unconditional. 
Does that seem right?  

Dave

Re: execute_control_command may not remove its cleanups

[Daniel Jacobowitz]

On Thu, Feb 19, 2004 at 10:28:38AM -0500, Dave Allan wrote:
> Hello,
> 
> I ran into a reproducible segfault in gdb (v.5.3, but the offending code
> is still present in the CVS tree I checked out this morning).  I traced
> the problemto the execute_control_command function in cli/cli-script.c. 
> 
> It appears that execute_control_command doesn't always do or discard the
> cleanups it creates before returning, which is not right according to
> the gdb internals docs.  According to section 13.1 Cleanups, "Your
> function should explicitly do or discard the cleanups it creates. 
> Failing to do this leads to non-deterministic behavior..."
> 
> The problem is that the call to do_cleanups in execute_control_command
> is conditional (cli/cli-script.c at line 430):
> 
> if (old_chain)
>   do_cleanups (old_chain);
> 
> So, if the cleanup_chain was null entering execute_control_command, then
> old_chain will be null, and the call to do_cleanups doesn't happen.  
> 
> Removing the if statement, thus making the do_cleanups (old_chain)
> unconditional eliminates the segfault.  
> 
> Unfortunately, the segfault occurred while I was using gdb run under the
> "crash" kernel dump analysis tool, and it appears to me that under
> normal gdb usage, cleanup_chain is never null going into
> execute_control_command.  Thus, do_cleanups is always executed and the
> segfault never appears and I don't have a reproducible test case that
> works against a vanilla build.  
> 
> However, it seems from code inspection and the gdb internals
> documentation that the call to do_cleanups ought to be unconditional. 
> Does that seem right?  

No, instead, the cleanup chain should always have an item on it.  If
make_cleanup is not called then old_chain will remain NULL, and
do_cleanups (NULL) means "do all cleanups", not "do nothing".  It looks
to me like command_handler is responsible for there always being a
cleanup on the chain:
  old_chain = make_cleanup (null_cleanup, 0);
but maybe I'm mistaken about that; it's a bit far down the tree.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: execute_control_command may not remove its cleanups

[Dave Allan]

> > However, it seems from code inspection and the gdb internals
> > documentation that the call to do_cleanups ought to be unconditional. 
> > Does that seem right?  
> 
> No, instead, the cleanup chain should always have an item on it.  If
> make_cleanup is not called then old_chain will remain NULL, and
> do_cleanups (NULL) means "do all cleanups", not "do nothing".  It looks
> to me like command_handler is responsible for there always being a
> cleanup on the chain:
>   old_chain = make_cleanup (null_cleanup, 0);
> but maybe I'm mistaken about that; it's a bit far down the tree.

I definitely understand that do_cleanups(NULL) will do all cleanups
which is not what's wanted here.  The call is do_cleanups(old_chain),
though, so if there are cleanups on the chain already, they are
preserved.  The problem isn't the do_cleanups call, it's the fact that
the do_cleanups call is conditional.  The solution is to remove the if
(old_chain) statement and always do the cleanup.  

Given what's stated in the docs, that a function must always remove the
cleanups it creates, it would seem to me that regardless of the state of
cleanup_chain at the beginning of execute_control_command, whether it's
NULL or contains cleanups, we want to get back to that state before we
return.  

Looking at what cleanups execute_control_command puts on cleanup_chain,
that is correct.  Either one or two cleanups are put on the chain where
arg is an automatic variable and function is free_current_contents.  If
these cleanups aren't done before the stack frame is destroyed,
something undefined will later be freed when the cleanups are done.  

For example (and this output is from debugging the gdb I compiled from
the CVS tree this morning), immediately prior to the call to do_cleanups
at line 431 cleanup_chain contains the following:

$11 = {next = 0x82a3d50, function = 0x8080310 <free_current_contents>,
arg = 0xbffff1f4}
$12 = {next = 0x82cf628, function = 0x8080310 <free_current_contents>,
arg = 0xbffff1f8}
$13 = {next = 0x0, function = 0x8080354 <null_cleanup>, arg = 0x0}

So, in this case, as you point out, we return to one entry on
cleanup_chain, however, it seems unsafe to me to count on there always
being an entry on the chain rather than explicitly removing whatever we
put on the chain, which doesn't affect prior entries.  

Dave

Re: execute_control_command may not remove its cleanups

[Daniel Jacobowitz]

On Thu, Feb 19, 2004 at 01:21:52PM -0500, Dave Allan wrote:
> > > However, it seems from code inspection and the gdb internals
> > > documentation that the call to do_cleanups ought to be unconditional. 
> > > Does that seem right?  
> > 
> > No, instead, the cleanup chain should always have an item on it.  If
> > make_cleanup is not called then old_chain will remain NULL, and
> > do_cleanups (NULL) means "do all cleanups", not "do nothing".  It looks
> > to me like command_handler is responsible for there always being a
> > cleanup on the chain:
> >   old_chain = make_cleanup (null_cleanup, 0);
> > but maybe I'm mistaken about that; it's a bit far down the tree.
> 
> I definitely understand that do_cleanups(NULL) will do all cleanups
> which is not what's wanted here.  The call is do_cleanups(old_chain),
> though, so if there are cleanups on the chain already, they are
> preserved.  The problem isn't the do_cleanups call, it's the fact that
> the do_cleanups call is conditional.  The solution is to remove the if
> (old_chain) statement and always do the cleanup.  
> 
> Given what's stated in the docs, that a function must always remove the
> cleanups it creates, it would seem to me that regardless of the state of
> cleanup_chain at the beginning of execute_control_command, whether it's
> NULL or contains cleanups, we want to get back to that state before we
> return.  
> 
> Looking at what cleanups execute_control_command puts on cleanup_chain,
> that is correct.  Either one or two cleanups are put on the chain where
> arg is an automatic variable and function is free_current_contents.  If
> these cleanups aren't done before the stack frame is destroyed,
> something undefined will later be freed when the cleanups are done.  

Think about this again.  Both of those cleanups are conditionally
created.  If neither of them is created, old_chain will still be NULL.
This will lead to running cleanups prematurely.  If the cleanup chain
is non-empty, things work OK.

The alternative is null_cleanup.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: execute_control_command may not remove its cleanups

[Dave Allan]

Ok.  I see your point.  How about setting old_chain to cleanup_chain
unconditionally at the beginning of the function and doing the cleanups
unconditionally at the end?  That way, we're safe against both
scenarios: against doing cleanups prematurely, but also safe against
getting into the function with cleanup_chain null and then freeing
something random at a later point.  

Dave


On Thu, 2004-02-19 at 13:47, Daniel Jacobowitz wrote:
> On Thu, Feb 19, 2004 at 01:21:52PM -0500, Dave Allan wrote:
> > > > However, it seems from code inspection and the gdb internals
> > > > documentation that the call to do_cleanups ought to be unconditional. 
> > > > Does that seem right?  
> > > 
> > > No, instead, the cleanup chain should always have an item on it.  If
> > > make_cleanup is not called then old_chain will remain NULL, and
> > > do_cleanups (NULL) means "do all cleanups", not "do nothing".  It looks
> > > to me like command_handler is responsible for there always being a
> > > cleanup on the chain:
> > >   old_chain = make_cleanup (null_cleanup, 0);
> > > but maybe I'm mistaken about that; it's a bit far down the tree.
> > 
> > I definitely understand that do_cleanups(NULL) will do all cleanups
> > which is not what's wanted here.  The call is do_cleanups(old_chain),
> > though, so if there are cleanups on the chain already, they are
> > preserved.  The problem isn't the do_cleanups call, it's the fact that
> > the do_cleanups call is conditional.  The solution is to remove the if
> > (old_chain) statement and always do the cleanup.  
> > 
> > Given what's stated in the docs, that a function must always remove the
> > cleanups it creates, it would seem to me that regardless of the state of
> > cleanup_chain at the beginning of execute_control_command, whether it's
> > NULL or contains cleanups, we want to get back to that state before we
> > return.  
> > 
> > Looking at what cleanups execute_control_command puts on cleanup_chain,
> > that is correct.  Either one or two cleanups are put on the chain where
> > arg is an automatic variable and function is free_current_contents.  If
> > these cleanups aren't done before the stack frame is destroyed,
> > something undefined will later be freed when the cleanups are done.  
> 
> Think about this again.  Both of those cleanups are conditionally
> created.  If neither of them is created, old_chain will still be NULL.
> This will lead to running cleanups prematurely.  If the cleanup chain
> is non-empty, things work OK.
> 
> The alternative is null_cleanup.

Re: execute_control_command may not remove its cleanups

[Andrew Cagney]

Dave,

> Ok.  I see your point.  How about setting old_chain to cleanup_chain
> unconditionally at the beginning of the function and doing the cleanups
> unconditionally at the end?  That way, we're safe against both
> scenarios: against doing cleanups prematurely, but also safe against
> getting into the function with cleanup_chain null and then freeing
> something random at a later point.  

FYI, that is one of the ways recommended in the doco:

> The first style is try/finally.  Before it exits, your code-block calls
> @code{do_cleanups} with the old cleanup chain and thus ensures that your
> code-block's cleanups are always performed.  For instance, the following
> code-segment avoids a memory leak problem (even when @code{error} is
> called and a forced stack unwind occurs) by ensuring that the
> @code{xfree} will always be called:
> 
> @smallexample
> struct cleanup *old = make_cleanup (null_cleanup, 0);
> data = xmalloc (sizeof blah);
> make_cleanup (xfree, data);
> ... blah blah ...
> do_cleanups (old);
> @end smallexample

The main reason why the the code you're studying doesn't match the doco 
is because it pre-dates that said doco :-(

Andrew