From: David Aguilar <davvid@gmail.com>
To: "Junio C Hamano" <gitster@pobox.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
"Alan Blotz" <work@blotz.org>,
"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
"Eric Sunshine" <sunshine@sunshineco.com>,
"Jeff King" <peff@peff.net>
Subject: Re: [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode
Date: Thu, 30 Sep 2021 10:03:27 -0700 [thread overview]
Message-ID: <CAJDDKr5QkUgHJVrij8bN-o0eNWr=eg7yHhHK0J8MxTALV0wr3Q@mail.gmail.com> (raw)
In-Reply-To: <xmqqv92rx8wa.fsf@gitster.g>
On Thu, Sep 23, 2021 at 2:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
>
> 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
next prev parent reply other threads:[~2021-09-30 17:04 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-23 4:12 [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches David Aguilar
2021-09-23 4:12 ` [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode David Aguilar
2021-09-23 21:46 ` Junio C Hamano
2021-09-30 17:03 ` David Aguilar [this message]
2021-09-23 4:12 ` [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes David Aguilar
2021-09-24 10:35 ` Ævar Arnfjörð Bjarmason
2021-09-30 17:05 ` David Aguilar
2021-09-23 4:12 ` [PATCH v5 3/3] difftool: add a missing space to the run_dir_diff() comments David Aguilar
2021-09-23 18:24 ` [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJDDKr5QkUgHJVrij8bN-o0eNWr=eg7yHhHK0J8MxTALV0wr3Q@mail.gmail.com' \
--to=davvid@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=congdanhqx@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=sunshine@sunshineco.com \
--cc=work@blotz.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).