[PATCH] Fix handle leak in write_tree

[PATCH] Fix handle leak in write_tree

[Alex Riesen]

This is a quick and dirty fix for the broken "git cherry-pick -n" on
some broken OS, which does not remove the directory entry after unlink
succeeded(!) if the file is still open somewher.
The entry is left but "protected": no open, no unlink, no stat.
Very annoying.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

That should be enough to get going, but I have to say that the
interface to lockfiles is really troublesome. Why has the caller close
a handle it didn't open? Especially if there are perfect matches for
the opening function (hold_locked_index) in form of commit and
rollback?

How about something like this (just interface):

struct lock_file
{
	struct lock_file *next;
	pid_t owner;
	int fd;
	char on_list;
	char filename[PATH_MAX];
};

struct lock_file *open_locked(const char *path, int die_on_error);
struct lock_file *open_index_locked(int die_on_error);
void commit_lock_file(struct lock_file *); /* always assuming .lock */
void rollback_lock_file(struct lock_file *);

 builtin-write-tree.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index c88bbd1..d8284b4 100644
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c
@@ -29,6 +29,7 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
 
 	was_valid = cache_tree_fully_valid(active_cache_tree);
 
+	close(newfd);
 	if (!was_valid) {
 		if (cache_tree_update(active_cache_tree,
 				      active_cache, active_nr,
@@ -36,8 +37,10 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
 			die("git-write-tree: error building trees");
 		if (0 <= newfd) {
 			if (!write_cache(newfd, active_cache, active_nr)
-					&& !close(newfd))
+					&& !close(newfd)) {
 				commit_lock_file(lock_file);
+				newfd = -1;
+			}
 		}
 		/* Not being able to write is fine -- we are only interested
 		 * in updating the cache-tree part, and if the next caller
@@ -55,6 +58,8 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
 	else
 		hashcpy(sha1, active_cache_tree->sha1);
 
+	if (0 <= newfd)
+		close(newfd);
 	rollback_lock_file(lock_file);
 
 	return 0;
-- 
1.5.1.1.946.gdb75a

Re: [PATCH] Fix handle leak in write_tree

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> This is a quick and dirty fix for the broken "git cherry-pick -n" on
> some broken OS, which does not remove the directory entry after unlink
> succeeded(!) if the file is still open somewher.
> The entry is left but "protected": no open, no unlink, no stat.
> Very annoying.
>
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
>
> That should be enough to get going, but I have to say that the
> interface to lockfiles is really troublesome. Why has the caller close
> a handle it didn't open? Especially if there are perfect matches for
> the opening function (hold_locked_index) in form of commit and
> rollback?
>
> How about something like this (just interface):
>
> struct lock_file
> {
> 	struct lock_file *next;
> 	pid_t owner;
> 	int fd;
> 	char on_list;
> 	char filename[PATH_MAX];
> };
>
> struct lock_file *open_locked(const char *path, int die_on_error);
> struct lock_file *open_index_locked(int die_on_error);
> void commit_lock_file(struct lock_file *); /* always assuming .lock */
> void rollback_lock_file(struct lock_file *);

I agree that making commit and rollback close the file
descriptor and lock holders to use lock->fd for write() makes
more sense, although it is a bit unclear from the above set of
function signatures what your plan on the lifetime rule for
"struct lock_file" is.  If it will be linked to the list given
to the atexit() handler and the caller of open_locked() never
frees it, I think I am fine with the interface.

Re: [PATCH] Fix handle leak in write_tree

[Alex Riesen]

On 4/24/07, Junio C Hamano <junkio@cox.net> wrote:
> > How about something like this (just interface):
> >
> > struct lock_file
> > {
> >       struct lock_file *next;
> >       pid_t owner;
> >       int fd;
> >       char on_list;
> >       char filename[PATH_MAX];
> > };
> >
> > struct lock_file *open_locked(const char *path, int die_on_error);
> > struct lock_file *open_index_locked(int die_on_error);
> > void commit_lock_file(struct lock_file *); /* always assuming .lock */
> > void rollback_lock_file(struct lock_file *);
>
> I agree that making commit and rollback close the file
> descriptor and lock holders to use lock->fd for write() makes
> more sense, although it is a bit unclear from the above set of
> function signatures what your plan on the lifetime rule for
> "struct lock_file" is.  If it will be linked to the list given
> to the atexit() handler and the caller of open_locked() never
> frees it, I think I am fine with the interface.

I actually expected the caller to define the lifetime. atexit is not exactly
libification-effort friendly. I could imagine open*locked with an additional
argument for atexit registration, I just don't like the idea (dislike
die_on_error
too). Isn't such kind of resource control _generally_ nicer to implement
in the top levels of a program?

Re: [PATCH] Fix handle leak in write_tree

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> ... Isn't such kind of resource control _generally_ nicer to implement
> in the top levels of a program?

In theory perhaps, but my understanding of our use of atexit()
is to clean them up in situations beyond the control of the top
levels of a program, most notably upon exit on signal.

Re: [PATCH] Fix handle leak in write_tree

[Alex Riesen]

Junio C Hamano, Tue, Apr 24, 2007 11:33:48 +0200:
> 
> > ... Isn't such kind of resource control _generally_ nicer to implement
> > in the top levels of a program?
> 
> In theory perhaps, but my understanding of our use of atexit()
> is to clean them up in situations beyond the control of the top
> levels of a program, most notably upon exit on signal.
> 

struct git_context;
/* or whatever is the latest name for the repo bookeeping is.
   It have to be passed to every git-related function anyway. */

struct lock_file *open_index_locked(struct git_context *, int die_on_error);
int commit_index_locked(struct git_context *, struct lock_file *);
... and so on.

Then let the top level call something like

    git_cleanup(struct git_context *);

in its _own_ signal or atexit handlers. If it didn't setup the
handlers, than perhaps it did want it so (leaving tempfiles behind is
sometimes done on purpose). Or it _is_ a bug, but then it is clear:
you have to cleanup, and you do git's part of cleanup with
git_cleanup.

Re: [PATCH] Fix handle leak in write_tree

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Then let the top level call something like
>
>     git_cleanup(struct git_context *);
>
> in its _own_ signal or atexit handlers. If it didn't setup the
> handlers, than perhaps it did want it so (leaving tempfiles behind is
> sometimes done on purpose). Or it _is_ a bug, but then it is clear:
> you have to cleanup, and you do git's part of cleanup with
> git_cleanup.

I would agree that libification would need to go something along
that line.

In the meantime for 1.5.2 stabilization, let's do the minimum
fix of applying your earlier patch.

[PATCH] Fix handle leak in write_tree

[Alex Riesen]

This is a quick and dirty fix for the broken "git cherry-pick -n" on
some broken OS, which does not remove the directory entry after unlink
succeeded(!) if the file is still open somewher.
The entry is left but "protected": no open, no unlink, no stat.
Very annoying.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Alex Riesen, Mon, Apr 23, 2007 21:49:25 +0200:
> diff --git a/builtin-write-tree.c b/builtin-write-tree.c
> index c88bbd1..d8284b4 100644
> --- a/builtin-write-tree.c
> +++ b/builtin-write-tree.c
> @@ -29,6 +29,7 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
>  
>  	was_valid = cache_tree_fully_valid(active_cache_tree);
>  
> +	close(newfd);
>  	if (!was_valid) {
>  		if (cache_tree_update(active_cache_tree,
>  				      active_cache, active_nr,

BTW, can someone explain this hunk? And how does it manage to
pass the tests?! And HOW did it manage to pass me?!!!

 builtin-write-tree.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index c88bbd1..391de53 100644
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c
@@ -36,8 +36,10 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
 			die("git-write-tree: error building trees");
 		if (0 <= newfd) {
 			if (!write_cache(newfd, active_cache, active_nr)
-					&& !close(newfd))
+					&& !close(newfd)) {
 				commit_lock_file(lock_file);
+				newfd = -1;
+			}
 		}
 		/* Not being able to write is fine -- we are only interested
 		 * in updating the cache-tree part, and if the next caller
@@ -55,6 +57,8 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
 	else
 		hashcpy(sha1, active_cache_tree->sha1);
 
+	if (0 <= newfd)
+		close(newfd);
 	rollback_lock_file(lock_file);
 
 	return 0;
-- 
1.5.2.rc0.65.gd296e