threads / bug / 31485

Probable bug in file run-command.c function clear_child_for_cleanup

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

## tl;dr

8 messages between Sep 9, 2012 and Sep 11, 2012.

replies: 7people: 4as markdown or json

David Gould· Sep 9, 2012, 14:44 UTC · lore
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· Sep 10, 2012, 13:44 UTC · re: David Gould · lore

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
Erik Faye-Lund· Sep 10, 2012, 13:58 UTC · re: Jeff King · lore

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

On Mon, Sep 10, 2012 at 3:44 PM, Jeff King <peff@peff.net> wrote:
Show 20 quoted lines
> 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· Sep 10, 2012, 14:10 UTC · re: Erik Faye-Lund · lore

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

On Mon, Sep 10, 2012 at 03:58:40PM +0200, Erik Faye-Lund wrote:
Show 21 quoted lines
> >> 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· Sep 10, 2012, 20:00 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 33 quoted lines
> 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· Sep 10, 2012, 20:01 UTC · re: Junio C Hamano · lore

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

On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> >> 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· Sep 10, 2012, 20:12 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> 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· Sep 11, 2012, 08:40 UTC · re: Junio C Hamano · lore

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

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:
Show 21 quoted lines
> Jeff King <peff@peff.net> 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

← back to recent threads