| Message ID | pull.1488.v2.git.git.1681165130765.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] clone: error specifically with --local and symlinked objects | expand |
"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes: > ++ if (errno == ENOTDIR) { > ++ int saved_errno = errno; > ++ struct stat st; > ++ if (lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode)) > ++ die(_("'%s' is a symlink, refusing to clone with --local"), > ++ src->buf); > ++ errno = saved_errno; > ++ } > die_errno(_("failed to start iterator over '%s'"), src->buf); I doubt you need saved_errno in the code immediately after this patch gets applied, as what you are saving is guaranteed to be ENOTDIR so you can just restore it before you fall through out of the block. It however would not hurt, though. Especially if the condition that guarding this block is likely to change in the future, we can view the apparent waste as paying insurance premium to protect from future breakages. Will queue. Thanks.
On Mon, Apr 10, 2023 at 10:18:50PM +0000, Glen Choo via GitGitGadget wrote: > I tried teaching dir_iterator_begin() to return an exit code to indicate > that we found a symlink, but it ended up looking quite ugly when I > started to consider plain files as well as generic lstat failures. I > think the extra lstat() is fine as a one-off, but if we need another > instance of this, we'd be better off doing the refactor. ;-). Heh, I was wondering what happened since your [1] made it sound like you would investigate making dir_iterator_begin() return an error code. [1]: https://lore.kernel.org/git/kl6lmt3k3ccc.fsf@chooglen-macbookpro.roam.corp.google.com/ > Documentation/git-clone.txt | 5 +++++ > builtin/clone.c | 11 ++++++++++- > t/t5604-clone-reference.sh | 2 +- > 3 files changed, 16 insertions(+), 2 deletions(-) But if it turned out ugly, I don't mind; this version looks great to me. Thanks, Taylor
On Mon, Apr 10, 2023 at 10:18:50PM +0000, Glen Choo via GitGitGadget wrote: > diff --git a/builtin/clone.c b/builtin/clone.c > index 462c286274c..46f6f689c85 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -327,8 +327,17 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, > > iter = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC); > > - if (!iter) > + if (!iter) { > + if (errno == ENOTDIR) { > + int saved_errno = errno; > + struct stat st; > + if (lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode)) I missed it on my first read, but you may want to consider "!lstat(...)" instead of "lstat(...) == 0". Probably not worth a reroll, though. Thanks, Taylor
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt index d6434d262d6..c37c4a37f74 100644 --- a/Documentation/git-clone.txt +++ b/Documentation/git-clone.txt @@ -58,6 +58,11 @@ never use the local optimizations). Specifying `--no-local` will override the default when `/path/to/repo` is given, using the regular Git transport instead. + +If the repository's `$GIT_DIR/objects` has symbolic links or is a +symbolic link, the clone will fail. This is a security measure to +prevent the unintentional copying of files by dereferencing the symbolic +links. ++ *NOTE*: this operation can race with concurrent modification to the source repository, similar to running `cp -r src dst` while modifying `src`. diff --git a/builtin/clone.c b/builtin/clone.c index 462c286274c..46f6f689c85 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -327,8 +327,17 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, iter = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC); - if (!iter) + if (!iter) { + if (errno == ENOTDIR) { + int saved_errno = errno; + struct stat st; + if (lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode)) + die(_("'%s' is a symlink, refusing to clone with --local"), + src->buf); + errno = saved_errno; + } die_errno(_("failed to start iterator over '%s'"), src->buf); + } strbuf_addch(src, '/'); src_len = src->len; diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh index 83e3c97861d..9845fc04d59 100755 --- a/t/t5604-clone-reference.sh +++ b/t/t5604-clone-reference.sh @@ -358,7 +358,7 @@ test_expect_success SYMLINKS 'clone repo with symlinked objects directory' ' test_must_fail git clone --local malicious clone 2>err && test_path_is_missing clone && - grep "failed to start iterator over" err + grep "is a symlink, refusing to clone with --local" err ' test_done