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
