Probable bug in file run-command.c function clear_child_for_cleanup

Probable bug in file run-command.c function clear_child_for_cleanup

[David Gould]

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

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

[Jeff King]

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

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

[Erik Faye-Lund]

On Mon, Sep 10, 2012 at 3:44 PM, Jeff King <peff@peff.net> 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.

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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?

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

[Jeff King]

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

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

[Junio C Hamano]

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)?

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

[David Gould]

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 <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