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

From: Jeff King <peff@peff.net>
Date: Sep 10, 2017, 17:38 UTC
Message-ID: <20170910173826.jopzzt5cpc2i3c6m@sigill.intra.peff.net>
In-Reply-To: <7a30c171-22c8-4ae3-3e15-3f4eea893127@web.de>

On Sun, Sep 10, 2017 at 12:37:06PM +0200, René Scharfe wrote:

Show 8 quoted lines

> > 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.
> 
> And we call opendir(3), readdir(3), and closedir(3), which are also not
> listed as async-safe functions by POSIX [1].

Good point. I don't know how dangerous those are in the real-world (i.e., is POSIX leaving an out for some implementations, or are they really unsafe on common platforms like Linux).

Show 7 quoted lines

> > 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.
> 
> We could start with a small buffer and expand it to match the path
> length of written files as we go.

Yes, but I didn't want to touch each code site that creates a file, which is a lot more invasive. I expect expanding to 4096 (or PATH_MAX) would be sufficient in practice.

I'd also note that the current code isn't _remotely_ async safe and nobody's complained. So I'm perfectly happy doing nothing, too. I care a bit more about the tempfile.c interface because it's invoked a lot more frequently.

> Deletion without readdir(3) requires us to keep a list of all written
> files and directories, though.  Perhaps in the form of an append-only
> log; the signal handler could then go through them in reverse order
> and remove them.  Or is there a better way?

No, I think that's how you'd have to do it. The implementation isn't all that bad, but hitting every file-creation would be invasive. I'd almost rather bail to exec-ing "rm -rf $dir", but we probably can't do _that_ in a signal-handler either. :)

-Peff