* [PATCH v2] Clear fd after closing to avoid double-close error
@ 2013-10-22 13:36 Jens Lindström
2013-10-22 15:29 ` Jeff King
2013-10-23 13:00 ` Duy Nguyen
0 siblings, 2 replies; 3+ messages in thread
From: Jens Lindström @ 2013-10-22 13:36 UTC (permalink / raw)
To: git; +Cc: Duy Nguyen
From: Jens Lindstrom <jl@opera.com>
In send_pack(), clear the fd passed to pack_objects() by setting
it to -1, since pack_objects() closes the fd (via a call to
run_command()). Likewise, in get_pack(), clear the fd passed to
run_command().
Not doing so risks having git_transport_push(), caller of
send_pack(), closing the fd again, possibly incorrectly closing
some other open file; or similarly with fetch_refs_from_pack(),
indirect caller of get_pack().
Signed-off-by: Jens Lindström <jl@opera.com>
---
fetch-pack.c | 4 ++++
send-pack.c | 4 ++++
2 files changed, 8 insertions(+)
diff --git a/fetch-pack.c b/fetch-pack.c
index f5d99c1..29b711a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -776,6 +776,10 @@ static int get_pack(struct fetch_pack_args *args,
close(cmd.out);
}
+ if (!use_sideband)
+ /* Closed by start_command() */
+ xd[0] = -1;
+
ret = finish_command(&cmd);
if (!ret || (args->check_self_contained_and_connected && ret == 1))
args->self_contained_and_connected =
diff --git a/send-pack.c b/send-pack.c
index 7d172ef..edbfd07 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -300,8 +300,12 @@ int send_pack(struct send_pack_args *args,
shutdown(fd[0], SHUT_WR);
if (use_sideband)
finish_async(&demux);
+ fd[1] = -1;
return -1;
}
+ if (!args->stateless_rpc)
+ /* Closed by pack_objects() via start_command() */
+ fd[1] = -1;
}
if (args->stateless_rpc && cmds_sent)
packet_flush(out);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] Clear fd after closing to avoid double-close error
2013-10-22 13:36 [PATCH v2] Clear fd after closing to avoid double-close error Jens Lindström
@ 2013-10-22 15:29 ` Jeff King
2013-10-23 13:00 ` Duy Nguyen
1 sibling, 0 replies; 3+ messages in thread
From: Jeff King @ 2013-10-22 15:29 UTC (permalink / raw)
To: Jens Lindström; +Cc: git, Duy Nguyen
On Tue, Oct 22, 2013 at 03:36:02PM +0200, Jens Lindström wrote:
> In send_pack(), clear the fd passed to pack_objects() by setting
> it to -1, since pack_objects() closes the fd (via a call to
> run_command()). Likewise, in get_pack(), clear the fd passed to
> run_command().
>
> Not doing so risks having git_transport_push(), caller of
> send_pack(), closing the fd again, possibly incorrectly closing
> some other open file; or similarly with fetch_refs_from_pack(),
> indirect caller of get_pack().
Thanks, this looks like the right thing to do.
You had mentioned earlier in the thread the possibility of just dropping
the close() in git_transport_push. I agree that it is not worth going
that route, as it serves as a safety check to avoid leaking in early
returns (and indeed, in both the fetch and push cases, we can return
early without spawning a subprocess when the receiver already has all of
the necessary objects).
> Signed-off-by: Jens Lindström <jl@opera.com>
Acked-by: Jeff King <peff@peff.net>
-Peff
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] Clear fd after closing to avoid double-close error
2013-10-22 13:36 [PATCH v2] Clear fd after closing to avoid double-close error Jens Lindström
2013-10-22 15:29 ` Jeff King
@ 2013-10-23 13:00 ` Duy Nguyen
1 sibling, 0 replies; 3+ messages in thread
From: Duy Nguyen @ 2013-10-23 13:00 UTC (permalink / raw)
To: Jens Lindström; +Cc: Git Mailing List
On Tue, Oct 22, 2013 at 8:36 PM, Jens Lindström <jl@opera.com> wrote:
> From: Jens Lindstrom <jl@opera.com>
>
> In send_pack(), clear the fd passed to pack_objects() by setting
> it to -1, since pack_objects() closes the fd (via a call to
> run_command()). Likewise, in get_pack(), clear the fd passed to
> run_command().
>
> Not doing so risks having git_transport_push(), caller of
> send_pack(), closing the fd again, possibly incorrectly closing
> some other open file; or similarly with fetch_refs_from_pack(),
> indirect caller of get_pack().
>
> Signed-off-by: Jens Lindström <jl@opera.com>
> ---
> fetch-pack.c | 4 ++++
> send-pack.c | 4 ++++
> 2 files changed, 8 insertions(+)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index f5d99c1..29b711a 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -776,6 +776,10 @@ static int get_pack(struct fetch_pack_args *args,
> close(cmd.out);
> }
>
> + if (!use_sideband)
> + /* Closed by start_command() */
> + xd[0] = -1;
> +
Further up demux.out still holds this handle. But we don't actually
start_async when use_sideband is false so demux.out can't be used
anywhere else. So it's good.
Acked-by: me
> ret = finish_command(&cmd);
> if (!ret || (args->check_self_contained_and_connected && ret == 1))
> args->self_contained_and_connected =
> diff --git a/send-pack.c b/send-pack.c
> index 7d172ef..edbfd07 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -300,8 +300,12 @@ int send_pack(struct send_pack_args *args,
> shutdown(fd[0], SHUT_WR);
> if (use_sideband)
> finish_async(&demux);
> + fd[1] = -1;
> return -1;
> }
> + if (!args->stateless_rpc)
> + /* Closed by pack_objects() via start_command() */
> + fd[1] = -1;
> }
> if (args->stateless_rpc && cmds_sent)
> packet_flush(out);
> --
> 1.8.1.2
>
--
Duy
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-10-23 13:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-22 13:36 [PATCH v2] Clear fd after closing to avoid double-close error Jens Lindström
2013-10-22 15:29 ` Jeff King
2013-10-23 13:00 ` Duy Nguyen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).