Re: Probable bug in file run-command.c function clear_child_for_cleanup

[Jeff King <peff@peff.net>]

On Sun, Sep 09, 2012 at 03:44:54PM +0100, David Gould wrote:

> static void clear_child_for_cleanup(pid_t pid)
> {
> 	struct child_to_clean **last, *p;
> 
> 	last = &children_to_clean;
> 	for (p = children_to_clean; p; p = p->next) {
> 		if (p->pid == pid) {
> 			*last = p->next;
> 			free(p);
> 			return;
> 		}
> 	}
> }
> 
> It appears that last is intended to point to the next field that's
> being updated, but fails to "follow" the p pointer along the chain.
> The result is that children_to_clean will end up pointing to the
> entry after the deleted one, and all the entries before it will be
> lost. It'll only be fine in the case that the pid is that of the
> first entry in the chain.

Yes, it's a bug. We should update "last" on each iteration.

> You want something like:
> 
> for (... {
> 	if (... {
> 		...
> 	}
> 	last = &p->next;
> }
> 
> or (probably clearer, but I haven't read your coding style guide, if
> there is one, and some people don't like this approach)

Yes, that's the correct fix. Care to submit a patch?

> for (p = children_to_clean; p; last = &p->next, p = p->next) {
> 	...

That is OK, too, but I think I prefer the first one.

-Peff