Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used

[Jeff King <peff@peff.net>]

On Wed, Jun 26, 2024 at 11:49:22AM -0700, Junio C Hamano wrote:

> Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:
> 
> > On 26/06/24 23:05, Junio C Hamano wrote:
> >> Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:
> >> 
> >>> To me, this looks much better.  child_process_clear's name already
> >>> suggests that is sort of like a destructor, so it makes sense to
> >>> re-initialize everything here.  I even wonder why it was not that way to
> >>> begin with.  I suppose no callers are assuming that it only clears args
> >>> and env though?
> >> 
> >> I guess that validating that supposition is a prerequisite to
> >> declare the change as "much better" and "makes sense".
> >
> > OK.  I found one: at the end of submodule.c:push_submodule()
> >
> > 	if (...) {
> > 		...some setup...
> > 		if (run_command(&cp))
> > 			return 0;
> > 		close(cp.out);
> > 	}
> 
> This is curious.
> 
>  * What is this thing trying to do?  When run_command() fails, it
>    wants to leave cp.out open, so that the caller this returns to
>    can write into it???  That cannot be the case, as cp itself is
>    internal.  So does this "close(cp.out)" really matter?

I think it's totally broken. Using cp.out, cp.in, etc, with
run_command() is a deadlock waiting to happen, since it implies opening
a pipe, not doing anything with our end, and then doing a waitpid() on
the child.

You'd always want to use start_command(), and then do something useful
with the pipe, and then finish_command(). Arguably run_command() should
bug if cp.out, etc are non-zero.

In this case the code leaves cp.out as 0, so we are not asking for a
pipe. That is good, and we are not subject to any race/deadlock. But
then...what is the cleanup trying to do? This will always just close(0),
the parent's stdin. That is certainly surprising, but I guess nobody
ever noticed because git-push does not usually read from its stdin.

So I think the close() is superfluous and should be deleted. It goes all
the way back to eb21c732d6 (push: teach --recurse-submodules the
on-demand option, 2012-03-29). I guess at one point the author thought
we'd read output from a pipe (rather than letting it just go to the
parent's stdout) and this was leftover?

>  * Even though we are running child_process_clear() to release the
>    resources in run_command() we are not closing the file descriptor
>    cp.out in the child_process_clear() and force the caller to close
>    it instead.  An open file descriptor is a resource, and a file
>    descriptor opened but forgotten is considered a leak.  I wonder
>    if child_process_clear() should be closing the file descriptor,
>    at least the ones it opened or dup2()ed.

Usually the caller will have handled this already (since it cares about
exactly _when_ to close), so we'd end up double-closing in most cases.
This is sometimes harmless (you'll get EBADF), but is a problem if the
descriptor was assigned to something else in the meantime.

In most cases callers shouldn't be using child_process_clear() at all
after start_command(), etc. Either:

  - start_command() failed, in which case it cleans up everything
    already (both in the struct and any pipes it opened)

  - we succeeded in starting the command, in which case
    child_process_clear() is insufficient. You need to actually
    finish_command() to avoid leaking the pid.

  - after finish_command(), the struct has been cleaned already. You do
    need to close pipes in the caller, but you'd have to do so before
    calling finish_command() anyway, to avoid the deadlock.

You really only need to call child_process_clear() yourself when you set
up a struct but didn't actually start the process. And from a quick skim
over the grep results, it looks like that's how it's used.

-Peff