Re: execute_control_command may not remove its cleanups

Daniel Jacobowitz
2004-02-19T15:40:21Z

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