threads / patch / 35179

v2Clear fd after closing to avoid double-close error

Subject: [PATCH v2] Clear fd after closing to avoid double-close error

## tl;dr

3 messages between Oct 22, 2013 and Oct 23, 2013. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Jens Lindström· Oct 22, 2013, 13:36 UTC · lore
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(+)
Show changes to 2 files +8 −0

fetch-pack.c, send-pack.c

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
Jeff King· Oct 22, 2013, 15:29 UTC · re: Jens Lindström · lore

Re: [PATCH v2] Clear fd after closing to avoid double-close error

On Tue, Oct 22, 2013 at 03:36:02PM +0200, Jens Lindström wrote:
Show 9 quoted lines
> 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
Duy Nguyen· Oct 23, 2013, 13:00 UTC · re: Jens Lindström · lore

Re: [PATCH v2] Clear fd after closing to avoid double-close error

On Tue, Oct 22, 2013 at 8:36 PM, Jens Lindström <jl@opera.com> wrote:
Show 30 quoted lines
> 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
Show 23 quoted lines
>         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

← back to recent threads