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

From: David Aguilar <davvid@gmail.com>
Date: Sep 30, 2021, 17:03 UTC
Message-ID: <CAJDDKr5QkUgHJVrij8bN-o0eNWr=eg7yHhHK0J8MxTALV0wr3Q@mail.gmail.com>
In-Reply-To: <xmqqv92rx8wa.fsf@gitster.g>

On Thu, Sep 23, 2021 at 2:46 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

>
> David Aguilar <davvid@gmail.com> writes:
>
> > 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 don't think the submodules loop ever runs into a scenario where the unlink would be relevant but it certainly wouldn't hurt from a defensive perspective.

Show 8 quoted lines

>
> 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).

From my reading of write_file() usage it seems like we're better off dealing with this just in difftool only. We'd be doing a wasteful unlink() in most situations if we handled the unlinks in write_file().

> 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 ;-).

Indeed. Lifting this pattern out into a common helper would help reduce this risk here.

I have a follow-up patch that addresses this and the edge cases that Ævar pointed out about the exit codes that was just submitted.

They are incremental patches on top of these patches but I resent the entire series for convenience. -- David