Re: [PATCH 06/34] clone: release strbuf after use in remove_junk()

[Jeff King <peff@peff.net>]

On Sun, Sep 10, 2017 at 08:27:40AM +0200, René Scharfe wrote:

> >>   	if (junk_work_tree) {
> >>   		strbuf_addstr(&sb, junk_work_tree);
> >>   		remove_dir_recursively(&sb, 0);
> >> -		strbuf_reset(&sb);
> >>   	}
> >> +	strbuf_release(&sb);
> >>   }
> > 
> > The code definitely needs a _release() at the end, but I feel
> > lukewarm about the "if we are about to _release(), do not bother to
> > _reset()" micro-optimization.  Keeping the existing two users that
> > use sb as a (shared and reused) temporary similar would help those
> > who add the third one or reuse the pattern in their code elsewhere.
> 
> That's not intended as an optimization, but as a promotion -- the reset
> is moved to the outer block and upgraded to a release.  The result is
> consistent with builtin/worktree.c::remove_junk().

Hmm. This is a cleanup function called only from signal and atexit
handlers. I don't think we actually do need to clean up, and this might
be a good candidate for UNLEAK().

And in fact, being called from a signal handler means we should
generally avoid touching malloc or free (which could be holding locks).
That would mean preferring a leak to strbuf_release(). Of course that is
the tip of the iceberg. We call strbuf_addstr() here, and
remove_dir_recursively() will grow our buffer.

So I actually wonder if junk_git_dir and junk_work_tree should be
pre-sized strbufs themselves. And that makes the leak "go away" in the
eyes of leak-checkers because we hold onto the static strbufs until
program exit.

I.e., something like this:

diff --git a/builtin/clone.c b/builtin/clone.c
index 8d11b570a1..a350f7801e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -471,8 +471,19 @@ static void clone_local(const char *src_repo, const char *dest_repo)
 		fprintf(stderr, _("done.\n"));
 }
 
-static const char *junk_work_tree;
-static const char *junk_git_dir;
+static void register_junk(struct strbuf *junk, const char *str)
+{
+	/*
+	 * we don't want to have to allocate for recursive removal during a
+	 * signal handler, so pre-size our strbufs to something that is
+	 * unlikely to overflow.
+	 */
+	strbuf_grow(junk, 4096);
+	strbuf_addstr(junk, str);
+}
+
+static struct strbuf junk_work_tree = STRBUF_INIT;
+static struct strbuf junk_git_dir = STRBUF_INIT;
 static enum {
 	JUNK_LEAVE_NONE,
 	JUNK_LEAVE_REPO,
@@ -486,8 +497,6 @@ N_("Clone succeeded, but checkout failed.\n"
 
 static void remove_junk(void)
 {
-	struct strbuf sb = STRBUF_INIT;
-
 	switch (junk_mode) {
 	case JUNK_LEAVE_REPO:
 		warning("%s", _(junk_leave_repo_msg));
@@ -499,16 +508,10 @@ static void remove_junk(void)
 		break;
 	}
 
-	if (junk_git_dir) {
-		strbuf_addstr(&sb, junk_git_dir);
-		remove_dir_recursively(&sb, 0);
-		strbuf_reset(&sb);
-	}
-	if (junk_work_tree) {
-		strbuf_addstr(&sb, junk_work_tree);
-		remove_dir_recursively(&sb, 0);
-		strbuf_reset(&sb);
-	}
+	if (junk_git_dir.len)
+		remove_dir_recursively(&junk_git_dir, 0);
+	if (junk_work_tree.len)
+		remove_dir_recursively(&junk_work_tree, 0);
 }
 
 static void remove_junk_on_signal(int signo)
@@ -970,11 +973,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		if (!dest_exists && mkdir(work_tree, 0777))
 			die_errno(_("could not create work tree dir '%s'"),
 				  work_tree);
-		junk_work_tree = work_tree;
+		register_junk(&junk_work_tree, work_tree);
 		set_git_work_tree(work_tree);
 	}
 
-	junk_git_dir = real_git_dir ? real_git_dir : git_dir;
+	register_junk(&junk_git_dir, real_git_dir ? real_git_dir : git_dir);
 	if (safe_create_leading_directories_const(git_dir) < 0)
 		die(_("could not create leading directories of '%s'"), git_dir);
 

Technically this would probably also benefit from all of the variables
being marked volatile, but we'd have to cast the volatility away to use
any strbuf functions. :(

If we really wanted to make this robust for signals (and I'm not sure
that it is worth the effort), I suspect the best route would be to teach
the tempfile.c code (which tries very hard to be careful about signals
and volatility) to handle directories.

-Peff