From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762936AbbA3SSk (ORCPT ); Fri, 30 Jan 2015 13:18:40 -0500 Received: from cloud.peff.net ([50.56.180.127]:43376 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1762838AbbA3SSi (ORCPT ); Fri, 30 Jan 2015 13:18:38 -0500 X-Greylist: delayed 401 seconds by postgrey-1.27 at vger.kernel.org; Fri, 30 Jan 2015 13:18:37 EST Date: Fri, 30 Jan 2015 13:11:53 -0500 From: Jeff King To: Junio C Hamano Cc: Git Mailing List , Josh Boyer , "Linux-Kernel@Vger. Kernel. Org" , twaugh@redhat.com, Linus Torvalds Subject: Re: [PATCH] apply: refuse touching a file beyond symlink Message-ID: <20150130181153.GA25513@peff.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 29, 2015 at 12:45:22PM -0800, Junio C Hamano wrote: > +static int path_is_beyond_symlink(const char *name_) > +{ > + struct strbuf name = STRBUF_INIT; > + > + strbuf_addstr(&name, name_); > + do { > + struct patch *previous; > + > + while (--name.len && name.buf[name.len] != '/') > + ; /* scan backwards */ > + if (!name.len) > + break; I imagine it is impossible here for "name_" to be initially empty, but it would make the backwards-scan loop go quite badly. Worth a comment or an assert()? > + name.buf[name.len] = '\0'; > + previous = in_fn_table(name.buf); > + if (previous) { > + if (!was_deleted(previous) && > + !to_be_deleted(previous) && > + previous->new_mode && > + S_ISLNK(previous->new_mode)) > + goto symlink_found; > + } else if (check_index) { > + int pos = cache_name_pos(name.buf, name.len); > + if (0 <= pos && > + S_ISLNK(active_cache[pos]->ce_mode)) > + goto symlink_found; > + } else { > + struct stat st; > + if (!lstat(name.buf, &st) && S_ISLNK(st.st_mode)) > + goto symlink_found; > + } > + } while (1); > + > + strbuf_release(&name); > + return 0; > +symlink_found: > + strbuf_release(&name); > + return 1; Style nit, but might this be easier to follow the logic without the gotos, by putting the setup and cleanup in a wrapper function and returning directly from the main logic? static int path_is_beyond_symlink(const char *name) { struct strbuf buf = STRBUF_INIT; int ret; strbuf_addstr(&buf, name); ret = path_is_beyond_symlink_1(name); strbuf_release(&buf); return ret; } I can live with it either way, though. > + if (!patch->is_delete && path_is_beyond_symlink(patch->new_name)) > + return error(_("affected file '%s' is beyond a symbolic link"), > + patch->new_name); Why does this not kick in when deleting a file? If it is not OK to add across a symlink, why is it OK to delete? IOW, why should this test fail: diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh index 0a8de4a..f03b604 100755 --- a/t/t4122-apply-symlink-inside.sh +++ b/t/t4122-apply-symlink-inside.sh @@ -64,6 +64,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (setup)' ' >arch/x86_64/dir/file && git add arch/x86_64/dir/file && git diff HEAD >add_file.patch && + git diff -R HEAD >del_file.patch && git reset --hard && rm -fr arch/x86_64/dir && @@ -111,7 +112,11 @@ test_expect_success SYMLINKS 'do not follow symbolic link (existing)' ' test_must_fail git apply --cached add_file.patch 2>error-ct-file && test_i18ngrep "beyond a symbolic link" error-ct-file && - test_must_fail git ls-files --error-unmatch arch/i386/dir + test_must_fail git ls-files --error-unmatch arch/i386/dir && + + >arch/i386/dir/file && + test_must_fail git apply del_file.patch && + test_path_is_file arch/i386/dir/file ' test_done > + test ! -e arch/x86_64/dir && > + test ! -e arch/i386/dir/file && Minor nit: use test_path_is_missing here (and elsewhere in the added tests). -Peff