From: David Gould <david@optimisefitness.com>
Date: Sun, 09 Sep 2012 14:44:54 GMT
Subject: Probable bug in file run-command.c function clear_child_for_cleanup
Message-ID: <504CAB66.1050003@optimisefitness.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