Re: Probable bug in file run-command.c function clear_child_for_cleanup
On Sun, Sep 09, 2012 at 03:44:54PM +0100, David Gould wrote:
Show 20 quoted lines
> 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.
Show 11 quoted lines
> 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
Jeff King <peff@peff.net>