Re: [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 23, 2021, 21:46 UTC
Message-ID: <xmqqv92rx8wa.fsf@gitster.g>
In-Reply-To: <20210923041252.52596-2-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 18 quoted lines

> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index bb9fe7245a..21e055d13a 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -557,11 +557,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>  		if (*entry->left) {
>  			add_path(&ldir, ldir_len, entry->path);
>  			ensure_leading_directories(ldir.buf);
> +			unlink(ldir.buf);
>  			write_file(ldir.buf, "%s", entry->left);
>  		}
>  		if (*entry->right) {
>  			add_path(&rdir, rdir_len, entry->path);
>  			ensure_leading_directories(rdir.buf);
> +			unlink(rdir.buf);
>  			write_file(rdir.buf, "%s", entry->right);
>  		}
>  	}

Curiously, this pattern repeats twice in the vicinity of the code. We cannot see it because it is out of pre-context, but the above is a body of a loop that iterates over "symlinks2" hashmap. There is another identical loop that iterates over "submodules", and we are not protecting ourselves from following a stray/leftover symbolic link in the loop.

I wonder if we should do the same to be defensive? I also wondered if write_file() should be the one that may want to be doing the unlink(), but I ran out of time before I finished reading all the callers to see if that is even a correct thing to do (meaning: some caller may want to truly overwrite an existing file, and follow symlinks if there already is, and I didn't audit all callers to see if there is no such caller).

The two identical looking loops also look like an accident waiting to happen---a patch like this that wants to touch only one of them would risk application to the other, wrong, loop if the patch gets old enough and patch offset grows larger ;-).

Thanks.