Re: execute_control_command may not remove its cleanups

[Daniel Jacobowitz <drow@false.org>]

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