# Probable bug in file run-command.c function clear_child_for_cleanup 8 messages from 2012-09-09 to 2012-09-11. Participants: David Gould, Jeff King, Erik Faye-Lund, Junio C Hamano. Thread: https://gitlist.dev/t/31485 ## David Gould, 2012-09-09 14:44 Subject: Probable bug in file run-command.c function clear_child_for_cleanup Message-ID: <504CAB66.1050003@optimisefitness.com> URL: https://gitlist.dev/e/504CAB66.1050003%40optimisefitness.com ``` Hi, This is probably the wrong way to do this, and I'm sorry if I end up wasting someone's time. That said, here goes.... While idly browsing through the git source (as you do on a sunny Sunday afternoon), I spotted the following code (that appears to be wrong) in the file https://github.com/git/git/blob/master/run-command.c It's the same in branches maint, next and pu. The branch todo gives me a 404. (line 53 is here) 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; } } } (line 67 is here) 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. 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) for (p = children_to_clean; p; last = &p->next, p = p->next) { ... Cheers, David -- David Gould, Personal Trainer Register of Kettlebell Professionals INWA Nordic Walking Instructor Optimise Fitness Ltd -- fit for life 01264 720709 www.optimisefitness.com ``` ## Jeff King, 2012-09-10 13:44 Subject: Re: Probable bug in file run-command.c function clear_child_for_cleanup Message-ID: <20120910134406.GA7496@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120910134406.GA7496%40sigill.intra.peff.net In-Reply-To: <504CAB66.1050003@optimisefitness.com> ``` 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 ``` ## Erik Faye-Lund, 2012-09-10 13:58 Subject: Re: Probable bug in file run-command.c function clear_child_for_cleanup Message-ID: URL: https://gitlist.dev/e/CABPQNSaufY9vUAFp9VL2i%3D6bNmO5_2%2BanzX6%2Bomfq%2BUgP03kXg%40mail.gmail.com In-Reply-To: <20120910134406.GA7496@sigill.intra.peff.net> ``` On Mon, Sep 10, 2012 at 3:44 PM, Jeff King wrote: > On Sun, Sep 09, 2012 at 03:44:54PM +0100, David Gould wrote: >> 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. > I feel like bikeshedding a bit today! I tend to either prefer either the latter or something like this: while (p) { ... last = p; p = p->next; } because those approaches put all the iteration logic in the same place. The in-body traversal approach is a bit more explicit about the traversal details. And to conclude my bikeshedding for the day: Shouldn't "last" ideally be called something like "prev" instead? It's the previously visited element, not the last element in the list. ``` ## Jeff King, 2012-09-10 14:10 Subject: Re: Probable bug in file run-command.c function clear_child_for_cleanup Message-ID: <20120910141027.GA8284@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120910141027.GA8284%40sigill.intra.peff.net In-Reply-To: ``` On Mon, Sep 10, 2012 at 03:58:40PM +0200, Erik Faye-Lund wrote: > >> for (... { > >> if (... { > >> ... > >> } > >> last = &p->next; > >> } > [...] > I feel like bikeshedding a bit today! > > I tend to either prefer either the latter or something like this: > > while (p) { > ... > > last = p; > p = p->next; > } > > because those approaches put all the iteration logic in the same > place. The in-body traversal approach is a bit more explicit about the > traversal details. Also fine by me. > And to conclude my bikeshedding for the day: Shouldn't "last" ideally > be called something like "prev" instead? It's the previously visited > element, not the last element in the list. It is the "last" element visited (just as "last week" is not the end of the world), but yes, it is ambiguous, and "prev" is not. Either is fine by me. -Peff ``` ## Junio C Hamano, 2012-09-10 20:00 Subject: Re: Probable bug in file run-command.c function clear_child_for_cleanup Message-ID: <7v8vchodgc.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8vchodgc.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120910141027.GA8284@sigill.intra.peff.net> ``` Jeff King writes: > On Mon, Sep 10, 2012 at 03:58:40PM +0200, Erik Faye-Lund wrote: > >> >> for (... { >> >> if (... { >> >> ... >> >> } >> >> last = &p->next; >> >> } >> [...] >> I feel like bikeshedding a bit today! >> >> I tend to either prefer either the latter or something like this: >> >> while (p) { >> ... >> >> last = p; >> p = p->next; >> } >> >> because those approaches put all the iteration logic in the same >> place. The in-body traversal approach is a bit more explicit about the >> traversal details. > > Also fine by me. > >> And to conclude my bikeshedding for the day: Shouldn't "last" ideally >> be called something like "prev" instead? It's the previously visited >> element, not the last element in the list. > > It is the "last" element visited (just as "last week" is not the end of > the world), but yes, it is ambiguous, and "prev" is not. Either is fine > by me. OK, so who's gonna do the honors? ``` ## Jeff King, 2012-09-10 20:01 Subject: Re: Probable bug in file run-command.c function clear_child_for_cleanup Message-ID: <20120910200110.GA32437@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120910200110.GA32437%40sigill.intra.peff.net In-Reply-To: <7v8vchodgc.fsf@alter.siamese.dyndns.org> ``` On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote: > >> And to conclude my bikeshedding for the day: Shouldn't "last" ideally > >> be called something like "prev" instead? It's the previously visited > >> element, not the last element in the list. > > > > It is the "last" element visited (just as "last week" is not the end of > > the world), but yes, it is ambiguous, and "prev" is not. Either is fine > > by me. > > OK, so who's gonna do the honors? I was hoping to give David a chance to submit his first-ever patch to git. -Peff ``` ## Junio C Hamano, 2012-09-10 20:12 Subject: Re: Probable bug in file run-command.c function clear_child_for_cleanup Message-ID: <7v4nn5ocvv.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4nn5ocvv.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120910200110.GA32437@sigill.intra.peff.net> ``` Jeff King writes: > On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote: > >> >> And to conclude my bikeshedding for the day: Shouldn't "last" ideally >> >> be called something like "prev" instead? It's the previously visited >> >> element, not the last element in the list. >> > >> > It is the "last" element visited (just as "last week" is not the end of >> > the world), but yes, it is ambiguous, and "prev" is not. Either is fine >> > by me. >> >> OK, so who's gonna do the honors? > > I was hoping to give David a chance to submit his first-ever patch to > git. OK. David, is it OK for us to expect a patch from you sometime not in distant future (it is an old bug we survived for a long time and nothing ultra-urgent)? ``` ## David Gould, 2012-09-11 08:40 Subject: Re: Probable bug in file run-command.c function clear_child_for_cleanup Message-ID: <504EF913.2050202@optimisefitness.com> URL: https://gitlist.dev/e/504EF913.2050202%40optimisefitness.com In-Reply-To: <7v4nn5ocvv.fsf@alter.siamese.dyndns.org> ``` Hi guys, Sorry for the delayed reply - what passes for my real life intruded somewhat. I'll get on to it today, but please be aware this will be my first-ever patch for ANY project, so am likely to foul up the process. I am reading the How To Submit Patches document even now.... Cheers, David On 10/09/12 21:12, Junio C Hamano wrote: > Jeff King writes: > >> On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote: >> >>>>> And to conclude my bikeshedding for the day: Shouldn't "last" ideally >>>>> be called something like "prev" instead? It's the previously visited >>>>> element, not the last element in the list. >>>> >>>> It is the "last" element visited (just as "last week" is not the end of >>>> the world), but yes, it is ambiguous, and "prev" is not. Either is fine >>>> by me. >>> >>> OK, so who's gonna do the honors? >> >> I was hoping to give David a chance to submit his first-ever patch to >> git. > > OK. David, is it OK for us to expect a patch from you sometime not > in distant future (it is an old bug we survived for a long time and > nothing ultra-urgent)? > -- David Gould, Personal Trainer Register of Kettlebell Professionals INWA Nordic Walking Instructor Optimise Fitness Ltd -- fit for life 01264 720709 www.optimisefitness.com ```