threads / patch / 6627

patch, 2 partsDon't leak file descriptors from unavailable pack files.

Subject: [PATCH 1/2] Don't leak file descriptors from unavailable pack files.

## tl;dr

4 messages between Feb 2, 2007 and Feb 3, 2007. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Shawn O. Pearce· Feb 2, 2007, 08:00 UTC · lore

If open_packed_git failed it may have been because the packfile actually exists and is readable, but some sort of verification did not pass. In this case open_packed_git left pack_fd filled in, as the file descriptor is valid. We don't want to leak the file descriptor, nor do we want to allow someone in the future to use this packed_git.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 sha1_file.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Show changes to sha1_file.c +4 −0
diff --git a/sha1_file.c b/sha1_file.c
index 37669d6..ba1c799 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1414,6 +1414,10 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, cons
 			 * was loaded!
 			 */
 			if (p->pack_fd == -1 && open_packed_git(p)) {
+				if (p->pack_fd != -1) {
+					close(p->pack_fd);
+					p->pack_fd = -1;
+				}
 				error("packfile %s cannot be accessed", p->pack_name);
 				continue;
 			}

-- 
1.5.0.rc3.1.ge4b0e
Junio C Hamano· Feb 2, 2007, 08:37 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 1/2] Don't leak file descriptors from unavailable pack files.

"Shawn O. Pearce" <spearce@spearce.org> writes:
> If open_packed_git failed it may have been because the packfile
> actually exists and is readable, but some sort of verification
> did not pass.  In this case open_packed_git left pack_fd filled
> in, as the file descriptor is valid.
Show 5 quoted lines
>  			if (p->pack_fd == -1 && open_packed_git(p)) {
> +				if (p->pack_fd != -1) {
> +					close(p->pack_fd);
> +					p->pack_fd = -1;
> +				}

I agree leaking fd is not nice, but I wonder if that should be dealt with by the caller.

Originally it did not matter as open_packed_git() died, but shouldn't it be closing the fd and marking p->pack_fd with -1, as you made it return instead of die?

Shawn O. Pearce· Feb 2, 2007, 08:53 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] Don't leak file descriptors from unavailable pack files.

Junio C Hamano <junkio@cox.net> wrote:
Show 6 quoted lines
> I agree leaking fd is not nice, but I wonder if that should be
> dealt with by the caller.
> 
> Originally it did not matter as open_packed_git() died, but
> shouldn't it be closing the fd and marking p->pack_fd with -1, as
> you made it return instead of die?
Yea, I thought of that when I wrote that evil patch...

But open_packed_git has 12 things that can cause it to return an error. That's basically a rewrite of the function. This was 4 lines added to the only caller which didn't die().

Probably the wrong thing to do.  But the patch was shorter!

Do you want to reject this patch and have me rewrite open_packed_git instead?

-- 
Shawn.
Junio C Hamano· Feb 3, 2007, 05:33 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 1/2] Don't leak file descriptors from unavailable pack files.

"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 13 quoted lines
> Junio C Hamano <junkio@cox.net> wrote:
>> I agree leaking fd is not nice, but I wonder if that should be
>> dealt with by the caller.
>> 
>> Originally it did not matter as open_packed_git() died, but
>> shouldn't it be closing the fd and marking p->pack_fd with -1, as
>> you made it return instead of die?
>
> Yea, I thought of that when I wrote that evil patch...
>
> But open_packed_git has 12 things that can cause it to return
> an error.  That's basically a rewrite of the function.  This was
> 4 lines added to the only caller which didn't die().

Well, you can let the compiler to do the job, like this, perhaps, if you really want to be lazy. There won't be any overhead from the extra call level, as there is only one caller of open_packed_git_1() which will be inlined ;-).

Show changes to sha1_file.c +16 −1
diff --git a/sha1_file.c b/sha1_file.c
index 2eff14a..45e410e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -552,7 +552,11 @@ void unuse_pack(struct pack_window **w_cursor)
 	}
 }
 
-static int open_packed_git(struct packed_git *p)
+/*
+ * Do not call this directly as this leaks p->pack_fd on error return;
+ * call open_packed_git() instead.
+ */
+static int open_packed_git_1(struct packed_git *p)
 {
 	struct stat st;
 	struct pack_header hdr;
@@ -608,6 +612,17 @@ static int open_packed_git(struct packed_git *p)
 	return 0;
 }
 
+static int open_packed_git(struct packed_git *p)
+{
+	if (!open_packed_git_1(p))
+		return 0;
+	if (p->pack_fd != -1) {
+		close(p->pack_fd);
+		p->pack_fd = -1;
+	}
+	return -1;
+}
+
 static int in_window(struct pack_window *win, unsigned long offset)
 {
 	/* We must promise at least 20 bytes (one hash) after the

← back to recent threads