Subject: Re: [PATCH 3/5] mv: move src_dir cleanup to end of cmd_mv()

From: Patrick Steinhardt <[email-masked]>
Date: Thu, 30 May 2024 09:04:28 +0200

On Thu, May 30, 2024 at 02:44:22AM -0400, Jeff King wrote:
> Commit b6f51e3db9 (mv: cleanup empty WORKING_DIRECTORY, 2022-08-09)
> added an auxiliary array where we store directory arguments that we see
> while processing the incoming arguments. After actually moving things,
> we then use that array to remove now-empty directories, and then
> immediately free the array.
> 
> But if the actual move queues any errors in only_match_skip_worktree,
> that can cause us to jump straight to the "out" label to clean up,
> skipping the free() and leaking the array.
> 
> Let's push the free() down past the "out" label so that we always clean
> up (the array is initialized to NULL, so this is always safe). We'll
> hold on to the memory a little longer than necessary, but clarity is
> more important than micro-optimizing here.
> 
> Note that the adjacent "a_src_dir" strbuf does not suffer the same
> problem; it is only allocated during the removal step.
> 
> Signed-off-by: Jeff King <[email-masked]>
> ---
> Reported as "new" by Coverity, but I think that is only because of the
> "goto out". Before your recent patches it was a straight "return", which
> was even worse. ;)

Ouf of curiosity, did you check whether this makes any additional tests
pass with SANITIZE=leak?

The fix itself looks good to me, thanks.

Patrick