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

Reply via email to