On Wed, Jun 26, 2024 at 11:49:22AM -0700, Junio C Hamano wrote:
> Abhijeet Sonar <abhijeet.nkt@xxxxxxxxx> writes:
>
> > On 26/06/24 23:05, Junio C Hamano wrote:
> >> Abhijeet Sonar <abhijeet.nkt@xxxxxxxxx> 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
