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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html