{"thread":{"id":"38452","subject":"patch-2.7.3 no longer applies relative symbolic link patches","startedAt":"2015-01-26T16:29:07Z","lastAt":"2015-01-31T21:27:37Z","messageCount":35,"participants":["Josh Boyer","Linus Torvalds","David Kastrup","Junio C Hamano","Andreas Gruenbacher","Stefan Beller","Christian Couder","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"255292","messageId":"CA+5PVA7rVy6Li_1haj1QmGG0D6avLB5Xej=2YGt6K-11kKHR5A@mail.gmail.com","threadId":"38452","inReplyTo":null,"subject":"patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Josh Boyer","fromEmail":"jwboyer@fedoraproject.org","sentAt":"2015-01-26T16:29:07Z","receivedAt":"2015-01-26T16:29:07Z","isPatch":false,"sender":{"key":"jwboyer@fedoraproject.org","avatar":null},"body":"Hi,\n\nI went to do the Fedora 3.19-rc6 build this morning and it failed in\nour buildsystem with:\n\n+ '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'\n+ case \"$patch\" in\n+ unxz\n+ patch -p1 -F1 -s\nsymbolic link target '../../../../../include/dt-bindings' is invalid\nerror: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)\n\nThat is coming from the hunk in patch-3.19-rc6.xz that creates the\nsymbolic link from arch/arm64/boot/dts/include/dt-bindings to\ninclude/dt-bindings. Oddly enough, patch-3.19-rc5.xz contains the\nsame hunk and it built fine last week.\n\nDigging in, it seems that upstream patch has decided that relative\nsymlinks are forbidden now as part of a fix for CVE-2015-1196. You\ncan find the relevant bugs here:\n\nhttps://bugzilla.redhat.com/show_bug.cgi?id=1185928\nhttps://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775901#13\n\nAside from locally modifying patch-3.19-rc6.xz, I'm not sure what else\nto do. I thought I would send a heads up since anyone that is using\npatch-2.7.3 is probably going to run into this issue.\n\njosh\n"},{"id":"255294","messageId":"CA+5PVA4bs6CYU8MHn1JqBjnb-5wYJT2Tjqa65=v2uSPL8c7dYw@mail.gmail.com","threadId":"38452","inReplyTo":"CA+5PVA7rVy6Li_1haj1QmGG0D6avLB5Xej=2YGt6K-11kKHR5A@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Josh Boyer","fromEmail":"jwboyer@fedoraproject.org","sentAt":"2015-01-26T16:32:17Z","receivedAt":"2015-01-26T16:32:17Z","isPatch":false,"sender":{"key":"jwboyer@fedoraproject.org","avatar":null},"body":"[Adding Junio's correct email address. Sigh.]\n\nOn Mon, Jan 26, 2015 at 11:29 AM, Josh Boyer wrote:\n> Hi,\n>\n> I went to do the Fedora 3.19-rc6 build this morning and it failed in\n> our buildsystem with:\n>\n> + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'\n> + case \"$patch\" in\n> + unxz\n> + patch -p1 -F1 -s\n> symbolic link target '../../../../../include/dt-bindings' is invalid\n> error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)\n>\n> That is coming from the hunk in patch-3.19-rc6.xz that creates the\n> symbolic link from arch/arm64/boot/dts/include/dt-bindings to\n> include/dt-bindings. Oddly enough, patch-3.19-rc5.xz contains the\n> same hunk and it built fine last week.\n>\n> Digging in, it seems that upstream patch has decided that relative\n> symlinks are forbidden now as part of a fix for CVE-2015-1196. You\n> can find the relevant bugs here:\n>\n> https://bugzilla.redhat.com/show_bug.cgi?id=1185928\n> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775901#13\n>\n> Aside from locally modifying patch-3.19-rc6.xz, I'm not sure what else\n> to do. I thought I would send a heads up since anyone that is using\n> patch-2.7.3 is probably going to run into this issue.\n>\n> josh\n"},{"id":"255302","messageId":"CA+55aFxbY21vBbPs5qCFPT1HSBbaeS+Z2Fr9So1r3rXrMWe_ZQ@mail.gmail.com","threadId":"38452","inReplyTo":"CA+5PVA4bs6CYU8MHn1JqBjnb-5wYJT2Tjqa65=v2uSPL8c7dYw@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-01-26T20:44:33Z","receivedAt":"2015-01-26T20:44:33Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Jan 26, 2015 at 8:32 AM, Josh Boyer wrote:\n>\n> I went to do the Fedora 3.19-rc6 build this morning and it failed in\n> our buildsystem with:\n>\n> + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'\n> + case \"$patch\" in\n> + unxz\n> + patch -p1 -F1 -s\n> symbolic link target '../../../../../include/dt-bindings' is invalid\n> error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)\n\nUgh. I don't see anything we can do about this on the git side, and I\ndo kind of understand why 'patch' would be worried about '..' files.\nIn a perfect world, patch would parse the filename and see that it\nstays within the directory structure of the project, but that is a\nrather harder thing to do than just say \"no dot-dot files\".\n\nThe short-term fix is likely to just use \"git apply\" instead of \"patch\".\n\nThe long-term fix? I dunno. I don't see us not using symlinks, and a\nquick check says that every *single* symlink we have in the kernel\nsource tree is one that points to a different directory using \"..\"\nformat. And while I could imagine that \"patch\" ends up counting the\ndot-dot entries and checking that it's all inside the same tree it is\npatching, I could also easily see patch *not* doing that. So using\n\"git apply\" _might_ end up being the long-term fix too.\n\nI suspect that if \"patch\" cannot apply even old-style kernel patches\ndue to the symlinks we have in the tree, and people end up having to\nuse \"git apply\" for them, I might end up starting to just use\nrename-patches (ie using \"git diff -M\") for the kernel.\n\nI've considered that for a while already, because \"patch\" _does_ kind\nof understand them these days, although I think it gets the\ncross-rename case wrong because it fundamentally works on a\nfile-by-file basis. But if \"patch\" just ends up not working at all,\nthe argument for trying to maintain backwards compatibility gets\nreally weak.\n\n Linus\n"},{"id":"255303","messageId":"87twzdl0iw.fsf@fencepost.gnu.org","threadId":"38452","inReplyTo":"CA+55aFxbY21vBbPs5qCFPT1HSBbaeS+Z2Fr9So1r3rXrMWe_ZQ@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2015-01-26T21:01:11Z","receivedAt":"2015-01-26T21:01:11Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Linus Torvalds writes:\n\n> On Mon, Jan 26, 2015 at 8:32 AM, Josh Boyer wrote:\n>>\n>> I went to do the Fedora 3.19-rc6 build this morning and it failed in\n>> our buildsystem with:\n>>\n>> + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'\n>> + case \"$patch\" in\n>> + unxz\n>> + patch -p1 -F1 -s\n>> symbolic link target '../../../../../include/dt-bindings' is invalid\n>> error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)\n>\n> Ugh. I don't see anything we can do about this on the git side, and I\n> do kind of understand why 'patch' would be worried about '..' files.\n> In a perfect world, patch would parse the filename and see that it\n> stays within the directory structure of the project, but that is a\n> rather harder thing to do than just say \"no dot-dot files\".\n>\n> The short-term fix is likely to just use \"git apply\" instead of \"patch\".\n>\n> The long-term fix? I dunno. I don't see us not using symlinks, and a\n> quick check says that every *single* symlink we have in the kernel\n> source tree is one that points to a different directory using \"..\"\n> format. And while I could imagine that \"patch\" ends up counting the\n> dot-dot entries and checking that it's all inside the same tree it is\n> patching, I could also easily see patch *not* doing that.\n\nI consider it rather hard and error-prone and/or an attack vector to\nchoose a course of action for ../ in connection with the -p option.\n\n-- \nDavid Kastrup\n"},{"id":"255304","messageId":"CA+5PVA5RdtLyRiYerG=u--bRZQ87qU0EGf7kGPMiQs9_KB3hRw@mail.gmail.com","threadId":"38452","inReplyTo":"CA+55aFxbY21vBbPs5qCFPT1HSBbaeS+Z2Fr9So1r3rXrMWe_ZQ@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Josh Boyer","fromEmail":"jwboyer@fedoraproject.org","sentAt":"2015-01-26T21:07:22Z","receivedAt":"2015-01-26T21:07:22Z","isPatch":false,"sender":{"key":"jwboyer@fedoraproject.org","avatar":null},"body":"On Mon, Jan 26, 2015 at 3:44 PM, Linus Torvalds\n wrote:\n> On Mon, Jan 26, 2015 at 8:32 AM, Josh Boyer wrote:\n>>\n>> I went to do the Fedora 3.19-rc6 build this morning and it failed in\n>> our buildsystem with:\n>>\n>> + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'\n>> + case \"$patch\" in\n>> + unxz\n>> + patch -p1 -F1 -s\n>> symbolic link target '../../../../../include/dt-bindings' is invalid\n>> error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)\n>\n> Ugh. I don't see anything we can do about this on the git side, and I\n> do kind of understand why 'patch' would be worried about '..' files.\n> In a perfect world, patch would parse the filename and see that it\n> stays within the directory structure of the project, but that is a\n> rather harder thing to do than just say \"no dot-dot files\".\n>\n> The short-term fix is likely to just use \"git apply\" instead of \"patch\".\n\nWell, that's one fix anyway. I just removed the hunk from the local\ncopy of patch-3.19-rc6.xz and added the symlink manually. See why\nbelow.\n\n> The long-term fix? I dunno. I don't see us not using symlinks, and a\n> quick check says that every *single* symlink we have in the kernel\n> source tree is one that points to a different directory using \"..\"\n> format. And while I could imagine that \"patch\" ends up counting the\n> dot-dot entries and checking that it's all inside the same tree it is\n> patching, I could also easily see patch *not* doing that. So using\n> \"git apply\" _might_ end up being the long-term fix too.\n\nIt could, but from a distro perspective that requires either doing\n'untar linux-3.N.tar.xz; cd linux-3.N; git add .; git apply\npatch-3.N+1-rcX' , or just using a git tree to begin with, which then\nmakes all of this unnecessary anyway. Creating a git repo from a\ntarball for each build is kind of silly. Some might say not just\nusing a git tree to build from to begin with in 2015 is also kind of\nsilly.\n\nOr did I miss a way that git-apply can take a git patch and apply it\nto a tree that isn't a git repo?\n\n> I suspect that if \"patch\" cannot apply even old-style kernel patches\n> due to the symlinks we have in the tree, and people end up having to\n> use \"git apply\" for them, I might end up starting to just use\n> rename-patches (ie using \"git diff -M\") for the kernel.\n\nI'm kind of wondering why we'd generate patches at all if you have to\napply them to a git repo, but maybe people like doing things the\nold-fashioned way just for the hell of it.\n\n> I've considered that for a while already, because \"patch\" _does_ kind\n> of understand them these days, although I think it gets the\n> cross-rename case wrong because it fundamentally works on a\n> file-by-file basis. But if \"patch\" just ends up not working at all,\n> the argument for trying to maintain backwards compatibility gets\n> really weak.\n\nYeah. I mostly wanted to give people a heads up on the issue. I'm\nsure it's going to impact more than just the kernel. I think for us\nit's mostly limited to the -rcX patches, because once the tarball for\nthe final release is out the symlink should be created by tar just\nfine.\n\njosh\n"},{"id":"255306","messageId":"CA+55aFwa1-pudNus+r=5EghpGkm33h--GZNND5UHt=ZKvP15Xw@mail.gmail.com","threadId":"38452","inReplyTo":"CA+5PVA5RdtLyRiYerG=u--bRZQ87qU0EGf7kGPMiQs9_KB3hRw@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-01-26T21:30:47Z","receivedAt":"2015-01-26T21:30:47Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Jan 26, 2015 at 1:07 PM, Josh Boyer wrote:\n>\n> Or did I miss a way that git-apply can take a git patch and apply it\n> to a tree that isn't a git repo?\n\nExactly. \"git apply\" works as a straight \"patch\" replacement outside\nof a git repository. It doesn't actually need a git tree to work.\n\n(Of course, \"git apply\" is _not_ a \"patch\" replacement in the general\nsense. It only applies context diffs - preferentially git style ones -\n so no old-style patches etc need apply. And it's not\nreplacement-compatible in a syntax sense either, in that while many of\nthe options are the same, not all are etc etc).\n\n Linus\n"},{"id":"255307","messageId":"CAPc5daVu=hjjYwDoCwco=cdg16kib80ZBbArh3z8R+j2vq6C6g@mail.gmail.com","threadId":"38452","inReplyTo":"CA+55aFwa1-pudNus+r=5EghpGkm33h--GZNND5UHt=ZKvP15Xw@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-26T21:35:36Z","receivedAt":"2015-01-26T21:35:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Jan 26, 2015 at 1:30 PM, Linus Torvalds\n wrote:\n> On Mon, Jan 26, 2015 at 1:07 PM, Josh Boyer wrote:\n>>\n>> Or did I miss a way that git-apply can take a git patch and apply it\n>> to a tree that isn't a git repo?\n>\n> Exactly. \"git apply\" works as a straight \"patch\" replacement outside\n> of a git repository. It doesn't actually need a git tree to work.\n\nWhat is your take on CVE-2015-1196, which brought this /regression/ to\nGNU patch?\nIf \"git apply\" get /fixed/ for that same CVE, would that /break/ your fix?\n"},{"id":"255308","messageId":"CA+55aFxdssyi_CrhB_yf8yXrG2PnuEHxf-=X6NnoVFxJnG0Jww@mail.gmail.com","threadId":"38452","inReplyTo":"CAPc5daVu=hjjYwDoCwco=cdg16kib80ZBbArh3z8R+j2vq6C6g@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-01-26T21:50:10Z","receivedAt":"2015-01-26T21:50:10Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Jan 26, 2015 at 1:35 PM, Junio C Hamano wrote:\n>\n> What is your take on CVE-2015-1196, which brought this /regression/ to\n> GNU patch?\n> If \"git apply\" get /fixed/ for that same CVE, would that /break/ your fix?\n\nI _think_ we allow arbitrary symlinks to be created, but then we\nshould be careful about actually _following_ them.\n\nAt least I _thought_ we were already quite careful not to do that,\neven if it's been a long time since I looked at the code. So even if\nwe create a symlink to outside the repository, it normally shouldn't\nmatter. We have that whole \"lstat_cache()\" thing that exists exactly\nto make it efficient to do pathname lookups while at the same time\nbeing aware of symlinks in the middle.\n\nOf course, our lstat cache is racy if somebody else modifies the tree\nconcurrently and changes things, but that's a non-issue, because if\nsomebody can just directly create random symlinks in the middle of the\ntree, I don't think we care about any symlinks _git_ might be creating\nconcurrently ;)\n\nBut it is entirely possible that \"git apply\" - especially when used\noutside of a real git directory - ends up doing that. And it's not\nlike we necessarily always use the whole \"lstat-cache\" mechanism to\nbegin with, so the fact that we have the infrastructure to be careful\nin no way means that we necessarily always _are_ careful...\n\n Linus\n"},{"id":"255310","messageId":"CA+5PVA7Hb1ppHFYA4wHC+uEyULk4m_7eX2FRNuisi1uOTagBEw@mail.gmail.com","threadId":"38452","inReplyTo":"CA+55aFwa1-pudNus+r=5EghpGkm33h--GZNND5UHt=ZKvP15Xw@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Josh Boyer","fromEmail":"jwboyer@fedoraproject.org","sentAt":"2015-01-26T22:15:50Z","receivedAt":"2015-01-26T22:15:50Z","isPatch":false,"sender":{"key":"jwboyer@fedoraproject.org","avatar":null},"body":"On Mon, Jan 26, 2015 at 4:30 PM, Linus Torvalds\n wrote:\n> On Mon, Jan 26, 2015 at 1:07 PM, Josh Boyer wrote:\n>>\n>> Or did I miss a way that git-apply can take a git patch and apply it\n>> to a tree that isn't a git repo?\n>\n> Exactly. \"git apply\" works as a straight \"patch\" replacement outside\n> of a git repository. It doesn't actually need a git tree to work.\n\nAh. I had somehow missed that entirely. Good to know for future reference.\n\n> (Of course, \"git apply\" is _not_ a \"patch\" replacement in the general\n> sense. It only applies context diffs - preferentially git style ones -\n> so no old-style patches etc need apply. And it's not\n> replacement-compatible in a syntax sense either, in that while many of\n> the options are the same, not all are etc etc).\n\nSure. Though for the Fedora kernel builds, we tend to use git\nformatted patches only anyway. I might play around with this and see\nhow it works as the normal way to apply things.\n\njosh\n"},{"id":"255317","messageId":"xmqqzj94lx7z.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"CA+55aFxbY21vBbPs5qCFPT1HSBbaeS+Z2Fr9So1r3rXrMWe_ZQ@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-27T03:27:12Z","receivedAt":"2015-01-27T03:27:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Ugh. I don't see anything we can do about this on the git side, and I\n> do kind of understand why 'patch' would be worried about '..' files.\n> In a perfect world, patch would parse the filename and see that it\n> stays within the directory structure of the project, but that is a\n> rather harder thing to do than just say \"no dot-dot files\".\n\nIt is unclear to me why \"limit to the current directory and below\"\nis such a big deal in the first place.\n\nIf the user wants to apply a patch that touches ../etc/shadow, is\nthe tool in the place to complain?\"\n"},{"id":"255327","messageId":"ma8am9$t01$1@ger.gmane.org","threadId":"38452","inReplyTo":"CA+55aFxbY21vBbPs5qCFPT1HSBbaeS+Z2Fr9So1r3rXrMWe_ZQ@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Andreas Gruenbacher","fromEmail":"agruen@gnu.org","sentAt":"2015-01-27T15:26:01Z","receivedAt":"2015-01-27T15:26:01Z","isPatch":false,"sender":{"key":"agruen@gnu.org","avatar":null},"body":"On Mon, 26 Jan 2015 12:44:33 -0800, Linus Torvalds wrote:\n> I've considered that for a while already, because \"patch\" _does_ kind of\n> understand them these days, although I think it gets the cross-rename\n> case wrong because it fundamentally works on a file-by-file basis.\n\nPatch handles cross-renames correctly nowadays; if not, it's a bug.\n\nThat's not enough to solve the symlink problem unfortunately.\n\nAndreas\n"},{"id":"255330","messageId":"ma8btn$t01$2@ger.gmane.org","threadId":"38452","inReplyTo":"CA+55aFxdssyi_CrhB_yf8yXrG2PnuEHxf-=X6NnoVFxJnG0Jww@mail.gmail.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Andreas Gruenbacher","fromEmail":"agruen@gnu.org","sentAt":"2015-01-27T15:47:04Z","receivedAt":"2015-01-27T15:47:04Z","isPatch":false,"sender":{"key":"agruen@gnu.org","avatar":null},"body":"On Mon, 26 Jan 2015 13:50:10 -0800, Linus Torvalds wrote:\n\n> On Mon, Jan 26, 2015 at 1:35 PM, Junio C Hamano \n> wrote:\n>>\n>> What is your take on CVE-2015-1196, which brought this /regression/ to\n>> GNU patch?\n>> If \"git apply\" get /fixed/ for that same CVE, would that /break/ your\n>> fix?\n> \n> I _think_ we allow arbitrary symlinks to be created, but then we should\n> be careful about actually _following_ them.\n\nI would prefer to allow arbitrary symlinks even in GNU patch, but patch \nstill must not be allowed to leave the working directory. The only way to \nachieve that I can think of is to implement path traversal in user space, \nwhich is not so easy to do correctly and efficiently.\n\nI think file system modifications from \"outside\" are not much of a \nconcern.\n\nAndreas\n"},{"id":"255347","messageId":"xmqqa914klg0.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"xmqqzj94lx7z.fsf@gitster.dls.corp.google.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-27T20:39:11Z","receivedAt":"2015-01-27T20:39:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Linus Torvalds writes:\n>\n>> Ugh. I don't see anything we can do about this on the git side, and I\n>> do kind of understand why 'patch' would be worried about '..' files.\n>> In a perfect world, patch would parse the filename and see that it\n>> stays within the directory structure of the project, but that is a\n>> rather harder thing to do than just say \"no dot-dot files\".\n>\n> It is unclear to me why \"limit to the current directory and below\"\n> is such a big deal in the first place.\n>\n> If the user wants to apply a patch that touches ../etc/shadow, is\n> the tool in the place to complain?\"\n\nLet me take this part back.\n\nI think \"git apply\" should behave closely to \"git apply --index\"\n(which is used by \"git am\" unless there is a very good reason not to\n(and \"'git apply --index' behaves differently from GNU patch, and we\nshould match what the latter does\" is not a very good reason). When\nthe index guards the working tree, we do not follow any symlink,\nwhether the destination is inside the current directory or not.\n\nI however do not think the current \"git apply\" notices that it will\noverwrite a path beyond a symlink---we may need to fix that if that\nis the case. I'll see what I can find (but I'll be doing 2.3-rc2\ntoday so it may be later this week).\n\nThanks.\n"},{"id":"255395","messageId":"xmqqfvauf7ej.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"xmqqa914klg0.fsf@gitster.dls.corp.google.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-29T06:05:56Z","receivedAt":"2015-01-29T06:05:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> If the user wants to apply a patch that touches ../etc/shadow, is\n>> the tool in the place to complain?\"\n>\n> Let me take this part back.\n>\n> I think \"git apply\" should behave closely to \"git apply --index\"\n> (which is used by \"git am\" unless there is a very good reason not to\n> (and \"'git apply --index' behaves differently from GNU patch, and we\n> should match what the latter does\" is not a very good reason). When\n> the index guards the working tree, we do not follow any symlink,\n> whether the destination is inside the current directory or not.\n>\n> I however do not think the current \"git apply\" notices that it will\n> overwrite a path beyond a symlink---we may need to fix that if that\n> is the case. I'll see what I can find (but I'll be doing 2.3-rc2\n> today so it may be later this week).\n\nYikes. It turns out that the index is what protects us from going\noutside the working tree. \"apply --index\" (hence \"am\") is immune\nagainst the CVE-2015-1196, but that is not because we do not follow\nsymbolic links.\n\nAlso the solution is not just a simple has_symlink_leading_path().\nHere is tonight's snapshot of what I've found out (not tested beyond\npassing the test suite including the new test added by the patch).\n\n-- >8 --\nSubject: [PATCH] apply: refuse touching a file beyond symlink\n\nBecause Git tracks symbolic links as symbolic links, a path that has\na symbolic link in its leading part (e.g. path/to/dir being a\nsymbolic link to somewhere else, be it inside or outside the working\ntree) can never appear in a patch that validly apply, unless the\nsame patch first removes the symbolic link.\n\nDetect and reject such a patch. Things to note:\n\n - Unfortunately, we cannot reuse the has_symlink_leading_path()\n from dir.c, as that is only about the working tree, but \"git\n apply\" can be told to apply the patch only to the index.\n\n - We cannot directly use has_symlink_leading_path() even when we\n are applying to the working tree, as an early patch of a valid\n input may remove a symbolic link path/to/dir and then a later\n patch of the input may create a path path/to/dir/file. The\n leading symbolic link check must be done on the interim result we\n compute in core (i.e. after the first patch, there is no\n path/to/dir symbolic link and it is perfectly valid to create\n path/to/dir/file). Similarly, when an input creates a symbolic\n link path/to/dir and then creates a file path/to/dir/file, we\n need to flag it as an error without actually creating path/to/dir\n symbolic link in the filesystem.\n\n - Instead, for any patch in the input that leaves a path (i.e. a\n non deletion) in the result, we check all leading paths against\n interim result and then either the index or the working tree.\n The interim result of applying patch is already kept track of\n by fn_table logic for us.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 44 +++++++++++++++++++++++++++++++++++++++++\n t/t4122-apply-symlink-inside.sh | 37 ++++++++++++++++++++++++++++++++++\n 2 files changed, 81 insertions(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex ef32e4f..da088c5 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3483,6 +3483,46 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n \treturn 0;\n }\n \n+static int path_is_beyond_symlink(const char *name_)\n+{\n+\tstruct strbuf name = STRBUF_INIT;\n+\n+\tstrbuf_addstr(&name, name_);\n+\tdo {\n+\t\tstruct patch *previous;\n+\n+\t\twhile (--name.len && name.buf[name.len] != '/')\n+\t\t\t; /* scan backwards */\n+\t\tif (!name.len)\n+\t\t\tbreak;\n+\t\tname.buf[name.len] = '\\0';\n+\t\tprevious = in_fn_table(name.buf);\n+\t\tif (previous) {\n+\t\t\tif (!was_deleted(previous) &&\n+\t\t\t !to_be_deleted(previous) &&\n+\t\t\t previous->new_mode &&\n+\t\t\t S_ISLNK(previous->new_mode))\n+\t\t\t\tgoto symlink_found;\n+\t\t} else if (check_index) {\n+\t\t\tint pos = cache_name_pos(name.buf, name.len);\n+\t\t\tif (0 <= pos &&\n+\t\t\t S_ISLNK(active_cache[pos]->ce_mode))\n+\t\t\t\tgoto symlink_found;\n+\t\t} else {\n+\t\t\tstruct stat st;\n+\t\t\tif (!lstat(name.buf, &st) && S_ISLNK(st.st_mode))\n+\t\t\t\tgoto symlink_found;\n+\t\t}\n+\t} while (1);\n+\n+\tstrbuf_release(&name);\n+\treturn 0;\n+symlink_found:\n+\tstrbuf_release(&name);\n+\treturn -1;\n+\n+}\n+\n /*\n * Check and apply the patch in-core; leave the result in patch->result\n * for the caller to write it out to the final destination.\n@@ -3570,6 +3610,10 @@ static int check_patch(struct patch *patch)\n \t\t}\n \t}\n \n+\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n+\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n+\t\t\t patch->new_name);\n+\n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n \tpatch->rejected = 0;\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 70b3a06..8b11bc6 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -52,4 +52,41 @@ test_expect_success 'check result' '\n \n '\n \n+test_expect_success 'do not follow symbolic link' '\n+\n+\tgit reset --hard &&\n+\ttest_ln_s_add ../i386/dir arch/x86_64/dir &&\n+\tgit diff HEAD >add_symlink.patch &&\n+\tgit reset --hard &&\n+\n+\tmkdir arch/x86_64/dir &&\n+\t>arch/x86_64/dir/file &&\n+\tgit add arch/x86_64/dir/file &&\n+\tgit diff HEAD >add_file.patch &&\n+\tgit reset --hard &&\n+\trm -fr arch/x86_64/dir &&\n+\n+\tcat add_symlink.patch add_file.patch >patch &&\n+\n+\tmkdir arch/i386/dir &&\n+\n+\ttest_must_fail git apply patch 2>error-wt &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt &&\n+\ttest ! -e arch/x86_64/dir &&\n+\ttest ! -e arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index patch 2>error-ix &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ix &&\n+\ttest ! -e arch/x86_64/dir &&\n+\ttest ! -e arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached patch 2>error-ct &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ct &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir\n+\n+'\n+\n test_done\n-- \n2.3.0-rc2-149-gdd42ee9\n"},{"id":"255400","messageId":"xmqqtwzadrj8.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"xmqqfvauf7ej.fsf@gitster.dls.corp.google.com","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-29T06:34:03Z","receivedAt":"2015-01-29T06:34:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Subject: [PATCH] apply: refuse touching a file beyond symlink\n>\n> Because Git tracks symbolic links as symbolic links, a path that has\n> a symbolic link in its leading part (e.g. path/to/dir being a\n> symbolic link to somewhere else, be it inside or outside the working\n> tree) can never appear in a patch that validly apply, unless the\n> same patch first removes the symbolic link.\n\nI should rephrase the above to make it more readable.\n\n ... its leading part (e.g. path/to/dir/file, where path/to/dir is a\n symbolic link to somewhere else, ...\n\nis what I meant to say.\n"},{"id":"255416","messageId":"xmqqa911e2ot.fsf_-_@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"xmqqtwzadrj8.fsf@gitster.dls.corp.google.com","subject":"[PATCH] apply: refuse touching a file beyond symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-29T20:45:22Z","receivedAt":"2015-01-29T20:45:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Because Git tracks symbolic links as symbolic links, a path that has\na symbolic link in its leading part (e.g. path/to/dir/file, where\npath/to/dir is a symbolic link to somewhere else, be it inside or\noutside the working tree) can never appear in a patch that validly\napplies, unless the same patch first removes the symbolic link to\nallow a directory to be there.\n\nDetect and reject such a patch. Things to note:\n\n - Unfortunately, we cannot reuse the has_symlink_leading_path()\n from dir.c, as that is only about the working tree, but \"git\n apply\" can be told to apply the patch only to the index or to\n both the index and to the working tree.\n\n - We cannot directly use has_symlink_leading_path() even when we\n are applying only to the working tree, as an early patch of a\n valid input may remove a symbolic link path/to/dir and then a\n later patch of the input may create a path path/to/dir/file, but\n \"git apply\" first checks the input without touching either the\n index or the working tree. The leading symbolic link check must\n be done on the interim result we compute in-core (i.e. after the\n first patch, there is no path/to/dir symbolic link and it is\n perfectly valid to create path/to/dir/file).\n\n Similarly, when an input creates a symbolic link path/to/dir and\n then creates a file path/to/dir/file, we need to flag it as an\n error without actually creating path/to/dir symbolic link in the\n filesystem.\n\nInstead, for any patch in the input that leaves a path (i.e. a non\ndeletion) in the result, we check all leading paths against interim\nresult and then either the index or the working tree. The interim\nresults of applying patches are kept track of by fn_table logic for\nus already, so use it to fiture out if existing a symbolic link will\ncause problems, if a new symbolic link that will cause problems will\nappear, etc.\n\nSigned-off-by: Junio C Hamano \n---\n\n * At least I convinced myself enough to say that I do not seem to\n be breaking things with this patch, after taking patches out of\n dozens of random pairs of commits from the Linux kernel history\n and applying them using this version ;-) No code change since\n last night's snapshot, but the test script is a bit more thorough\n in this version.\n\n builtin/apply.c | 44 +++++++++++++++++++++++++++++\n t/t4122-apply-symlink-inside.sh | 62 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 106 insertions(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex ef32e4f..dcb44fb 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3483,6 +3483,46 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n \treturn 0;\n }\n \n+static int path_is_beyond_symlink(const char *name_)\n+{\n+\tstruct strbuf name = STRBUF_INIT;\n+\n+\tstrbuf_addstr(&name, name_);\n+\tdo {\n+\t\tstruct patch *previous;\n+\n+\t\twhile (--name.len && name.buf[name.len] != '/')\n+\t\t\t; /* scan backwards */\n+\t\tif (!name.len)\n+\t\t\tbreak;\n+\t\tname.buf[name.len] = '\\0';\n+\t\tprevious = in_fn_table(name.buf);\n+\t\tif (previous) {\n+\t\t\tif (!was_deleted(previous) &&\n+\t\t\t !to_be_deleted(previous) &&\n+\t\t\t previous->new_mode &&\n+\t\t\t S_ISLNK(previous->new_mode))\n+\t\t\t\tgoto symlink_found;\n+\t\t} else if (check_index) {\n+\t\t\tint pos = cache_name_pos(name.buf, name.len);\n+\t\t\tif (0 <= pos &&\n+\t\t\t S_ISLNK(active_cache[pos]->ce_mode))\n+\t\t\t\tgoto symlink_found;\n+\t\t} else {\n+\t\t\tstruct stat st;\n+\t\t\tif (!lstat(name.buf, &st) && S_ISLNK(st.st_mode))\n+\t\t\t\tgoto symlink_found;\n+\t\t}\n+\t} while (1);\n+\n+\tstrbuf_release(&name);\n+\treturn 0;\n+symlink_found:\n+\tstrbuf_release(&name);\n+\treturn 1;\n+\n+}\n+\n /*\n * Check and apply the patch in-core; leave the result in patch->result\n * for the caller to write it out to the final destination.\n@@ -3570,6 +3610,10 @@ static int check_patch(struct patch *patch)\n \t\t}\n \t}\n \n+\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n+\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n+\t\t\t patch->new_name);\n+\n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n \tpatch->rejected = 0;\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 70b3a06..0a8de4a 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -52,4 +52,66 @@ test_expect_success 'check result' '\n \n '\n \n+test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '\n+\n+\tgit reset --hard &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\tgit add arch/x86_64/dir &&\n+\tgit diff HEAD >add_symlink.patch &&\n+\tgit reset --hard &&\n+\n+\tmkdir arch/x86_64/dir &&\n+\t>arch/x86_64/dir/file &&\n+\tgit add arch/x86_64/dir/file &&\n+\tgit diff HEAD >add_file.patch &&\n+\tgit reset --hard &&\n+\trm -fr arch/x86_64/dir &&\n+\n+\tcat add_symlink.patch add_file.patch >patch &&\n+\n+\tmkdir arch/i386/dir\n+'\n+\n+test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '\n+\n+\t# same input creates a confusihng symbolic link\n+\ttest_must_fail git apply patch 2>error-wt &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt &&\n+\ttest ! -e arch/x86_64/dir &&\n+\ttest ! -e arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index patch 2>error-ix &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ix &&\n+\ttest ! -e arch/x86_64/dir &&\n+\ttest ! -e arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached patch 2>error-ct &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ct &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir\n+'\n+\n+test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '\n+\n+\t# existing symbolic link\n+\tgit reset --hard &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\tgit add arch/x86_64/dir &&\n+\n+\ttest_must_fail git apply add_file.patch 2>error-wt-file &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt-file &&\n+\ttest ! -e arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index add_file.patch 2>error-ix-file &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ix-file &&\n+\ttest ! -e arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached add_file.patch 2>error-ct-file &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ct-file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir\n+'\n+\n test_done\n-- \n2.3.0-rc2-153-g9e53805\n"},{"id":"255419","messageId":"CAGZ79kYqGoLGMkXChH+-63JtGbAAb8gnAbUHYuJYv=NUUNt4XQ@mail.gmail.com","threadId":"38452","inReplyTo":"xmqqa911e2ot.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-29T22:15:13Z","receivedAt":"2015-01-29T22:15:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 29, 2015 at 12:45 PM, Junio C Hamano wrote:\n\n> +\n> +test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '\n> +\n> + # same input creates a confusihng symbolic link\n\ns/confusihng/confusing/\n"},{"id":"255421","messageId":"xmqqzj91cfnl.fsf_-_@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"xmqqa911e2ot.fsf_-_@gitster.dls.corp.google.com","subject":"[PATCH 2/1] apply: reject input that touches outside $cwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-29T23:48:14Z","receivedAt":"2015-01-29T23:48:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"By default, a patch that affects outside the working area is\nrejected as a mistake; Git itself never creates such a patch\nunless the user bends backwards and specifies nonstandard\nprefix to \"git diff\" and friends.\n\nWhen `git apply` is used without either `--index` or `--cached`\noption as a \"better GNU patch\", the user can pass `--allow-uplevel`\noption to override this safety check. This cannot be used to escape\noutside the working tree when using `--index` or `--cached` to apply\nthe patch to the index.\n\nThe new test was stolen from Jeff King with slight enhancements.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Meant to apply on top of the previous one, but these two are\n about separate and orthogonal issues.\n\n Documentation/git-apply.txt | 14 ++++-\n builtin/apply.c | 26 +++++++++\n t/t4139-apply-escape.sh | 137 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 176 insertions(+), 1 deletion(-)\n create mode 100755 t/t4139-apply-escape.sh\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex f605327..20c3a6f 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -16,7 +16,7 @@ SYNOPSIS\n \t [--ignore-space-change | --ignore-whitespace ]\n \t [--whitespace=(nowarn|warn|fix|error|error-all)]\n \t [--exclude=] [--include=] [--directory=]\n-\t [--verbose] [...]\n+\t [--verbose] [--allow-uplevel] [...]\n \n DESCRIPTION\n -----------\n@@ -229,6 +229,18 @@ For example, a patch that talks about updating `a/git-gui.sh` to `b/git-gui.sh`\n can be applied to the file in the working tree `modules/git-gui/git-gui.sh` by\n running `git apply --directory=modules/git-gui`.\n \n+--allow-uplevel::\n+\tBy default, a patch that affects outside the working area is\n+\trejected as a mistake; Git itself never creates such a patch\n+\tunless the user bends backwards and specifies nonstandard\n+\tprefix to \"git diff\" and friends.\n++\n+When `git apply` is used without either `--index` or `--cached`\n+option as a \"better GNU patch\", the user can pass `--allow-uplevel`\n+option to override this safety check. This cannot be used to escape\n+outside the working tree when using `--index` or `--cached` to apply\n+the patch to the index.\n+\n Configuration\n -------------\n \ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex dcb44fb..ce5a594 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -50,6 +50,7 @@ static int apply_verbosely;\n static int allow_overlap;\n static int no_add;\n static int threeway;\n+static int allow_uplevel;\n static const char *fake_ancestor;\n static int line_termination = '\\n';\n static unsigned int p_context = UINT_MAX;\n@@ -3523,6 +3524,23 @@ symlink_found:\n \n }\n \n+static void die_on_uplevel_path(struct patch *patch)\n+{\n+\tconst char *old_name = NULL;\n+\tconst char *new_name = NULL;\n+\tif (patch->is_delete)\n+\t\told_name = patch->old_name;\n+\telse if (!patch->is_new && !patch->is_copy)\n+\t\told_name = patch->old_name;\n+\tif (!patch->is_delete)\n+\t\tnew_name = patch->new_name;\n+\n+\tif (old_name && !verify_path(old_name))\n+\t\tdie(_(\"invalid path '%s'\"), old_name);\n+\tif (new_name && !verify_path(new_name))\n+\t\tdie(_(\"invalid path '%s'\"), new_name);\n+}\n+\n /*\n * Check and apply the patch in-core; leave the result in patch->result\n * for the caller to write it out to the final destination.\n@@ -3614,6 +3632,9 @@ static int check_patch(struct patch *patch)\n \t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n \t\t\t patch->new_name);\n \n+\tif (!allow_uplevel)\n+\t\tdie_on_uplevel_path(patch);\n+\n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n \tpatch->rejected = 0;\n@@ -4423,6 +4444,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \t\t\tN_(\"make sure the patch is applicable to the current index\")),\n \t\tOPT_BOOL(0, \"cached\", &cached,\n \t\t\tN_(\"apply a patch without touching the working tree\")),\n+\t\tOPT_BOOL(0, \"allow-uplevel\", &allow_uplevel,\n+\t\t\tN_(\"accept a patch to touch outside the current directory\")),\n \t\tOPT_BOOL(0, \"apply\", &force_apply,\n \t\t\tN_(\"also apply the patch (use with --stat/--summary/--check)\")),\n \t\tOPT_BOOL('3', \"3way\", &threeway,\n@@ -4495,6 +4518,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \t\t\tdie(_(\"--cached outside a repository\"));\n \t\tcheck_index = 1;\n \t}\n+\tif (check_index)\n+\t\tallow_uplevel = 0;\n+\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tint fd;\ndiff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh\nnew file mode 100755\nindex 0000000..39de838\n--- /dev/null\n+++ b/t/t4139-apply-escape.sh\n@@ -0,0 +1,137 @@\n+#!/bin/sh\n+\n+test_description='paths written by git-apply cannot escape the working tree'\n+. ./test-lib.sh\n+\n+# tests will try to write to ../foo, and we do not\n+# want them to escape the trash directory when they\n+# fail\n+test_expect_success 'bump git repo one level down' '\n+\tmkdir inside &&\n+\tmv .git inside/ &&\n+\tcd inside\n+'\n+\n+# $1 = name of file\n+# $2 = current path to file (if different)\n+mkpatch_add() {\n+\trm -f \"${2:-$1}\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tnew file mode 100644\n+\tindex 0000000..53c74cd\n+\t--- /dev/null\n+\t+++ b/$1\n+\t@@ -0,0 +1 @@\n+\t+evil\n+\tEOF\n+}\n+\n+mkpatch_del() {\n+\techo evil >\"${2:-$1}\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tdeleted file mode 100644\n+\tindex 53c74cd..0000000\n+\t--- a/$1\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-evil\n+\tEOF\n+}\n+\n+# $1 = name of file\n+# $2 = content of symlink\n+mkpatch_symlink() {\n+\trm -f \"$1\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tnew file mode 120000\n+\tindex 0000000..$(printf \"%s\" \"$2\" | git hash-object --stdin)\n+\t--- /dev/null\n+\t+++ b/$1\n+\t@@ -0,0 +1 @@\n+\t+$2\n+\t\\ No newline at end of file\n+\tEOF\n+}\n+\n+test_expect_success 'cannot add file containing ..' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'can add file containing .. with --allow-uplevel' '\n+\tmkpatch_add ../foo >patch &&\n+\tgit apply --allow-uplevel patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_success 'cannot add file containing .. (index)' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot add file containing .. with --allow-uplevel (index)' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply --index --allow-uplevel patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot del file containing ..' '\n+\tmkpatch_del ../foo >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_success 'can del file containing .. with --allow-uplevel' '\n+\tmkpatch_del ../foo >patch &&\n+\tgit apply --allow-uplevel patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot del file containing .. (index)' '\n+\tmkpatch_del ../foo >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_success 'symlink escape via ..' '\n+\t{\n+\t\tmkpatch_symlink tmp .. &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'symlink escape via .. (index)' '\n+\t{\n+\t\tmkpatch_symlink tmp .. &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'symlink escape via absolute path' '\n+\t{\n+\t\tmkpatch_symlink tmp \"$(pwd)\" &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'symlink escape via absolute path (index)' '\n+\t{\n+\t\tmkpatch_symlink tmp \"$(pwd)\" &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_done\n-- \n2.3.0-rc2-158-g17413e7\n"},{"id":"255427","messageId":"CAP8UFD0zourNU6oqxcORP=3x2oXmTa3xz+jicdWRLXBgN7QQtA@mail.gmail.com","threadId":"38452","inReplyTo":"xmqqa911e2ot.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-01-30T09:04:57Z","receivedAt":"2015-01-30T09:04:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jan 29, 2015 at 9:45 PM, Junio C Hamano wrote:\n>\n> Instead, for any patch in the input that leaves a path (i.e. a non\n> deletion) in the result, we check all leading paths against interim\n> result and then either the index or the working tree. The interim\n> results of applying patches are kept track of by fn_table logic for\n> us already, so use it to fiture out if existing a symbolic link will\n\ns/fiture/figure/\ns/existing a symbolic link/an existing symbolic link/\n\n> cause problems, if a new symbolic link that will cause problems will\n> appear, etc.\n"},{"id":"255440","messageId":"20150130181153.GA25513@peff.net","threadId":"38452","inReplyTo":"xmqqa911e2ot.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-30T18:11:53Z","receivedAt":"2015-01-30T18:11:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 29, 2015 at 12:45:22PM -0800, Junio C Hamano wrote:\n\n> +static int path_is_beyond_symlink(const char *name_)\n> +{\n> +\tstruct strbuf name = STRBUF_INIT;\n> +\n> +\tstrbuf_addstr(&name, name_);\n> +\tdo {\n> +\t\tstruct patch *previous;\n> +\n> +\t\twhile (--name.len && name.buf[name.len] != '/')\n> +\t\t\t; /* scan backwards */\n> +\t\tif (!name.len)\n> +\t\t\tbreak;\n\nI imagine it is impossible here for \"name_\" to be initially empty, but\nit would make the backwards-scan loop go quite badly. Worth a comment or\nan assert()?\n\n> +\t\tname.buf[name.len] = '\\0';\n> +\t\tprevious = in_fn_table(name.buf);\n> +\t\tif (previous) {\n> +\t\t\tif (!was_deleted(previous) &&\n> +\t\t\t !to_be_deleted(previous) &&\n> +\t\t\t previous->new_mode &&\n> +\t\t\t S_ISLNK(previous->new_mode))\n> +\t\t\t\tgoto symlink_found;\n> +\t\t} else if (check_index) {\n> +\t\t\tint pos = cache_name_pos(name.buf, name.len);\n> +\t\t\tif (0 <= pos &&\n> +\t\t\t S_ISLNK(active_cache[pos]->ce_mode))\n> +\t\t\t\tgoto symlink_found;\n> +\t\t} else {\n> +\t\t\tstruct stat st;\n> +\t\t\tif (!lstat(name.buf, &st) && S_ISLNK(st.st_mode))\n> +\t\t\t\tgoto symlink_found;\n> +\t\t}\n> +\t} while (1);\n> +\n> +\tstrbuf_release(&name);\n> +\treturn 0;\n> +symlink_found:\n> +\tstrbuf_release(&name);\n> +\treturn 1;\n\nStyle nit, but might this be easier to follow the logic without the\ngotos, by putting the setup and cleanup in a wrapper function and\nreturning directly from the main logic?\n\n static int path_is_beyond_symlink(const char *name)\n {\n\tstruct strbuf buf = STRBUF_INIT;\n\tint ret;\n\n\tstrbuf_addstr(&buf, name);\n\tret = path_is_beyond_symlink_1(name);\n\tstrbuf_release(&buf);\n\n\treturn ret;\n }\n\nI can live with it either way, though.\n\n> +\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n> +\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n> +\t\t\t patch->new_name);\n\nWhy does this not kick in when deleting a file? If it is not OK to\nadd across a symlink, why is it OK to delete? IOW, why should this test\nfail:\n\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 0a8de4a..f03b604 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -64,6 +64,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '\n \t>arch/x86_64/dir/file &&\n \tgit add arch/x86_64/dir/file &&\n \tgit diff HEAD >add_file.patch &&\n+\tgit diff -R HEAD >del_file.patch &&\n \tgit reset --hard &&\n \trm -fr arch/x86_64/dir &&\n \n@@ -111,7 +112,11 @@ test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '\n \n \ttest_must_fail git apply --cached add_file.patch 2>error-ct-file &&\n \ttest_i18ngrep \"beyond a symbolic link\" error-ct-file &&\n-\ttest_must_fail git ls-files --error-unmatch arch/i386/dir\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\t>arch/i386/dir/file &&\n+\ttest_must_fail git apply del_file.patch &&\n+\ttest_path_is_file arch/i386/dir/file\n '\n \n test_done\n\n> +\ttest ! -e arch/x86_64/dir &&\n> +\ttest ! -e arch/i386/dir/file &&\n\nMinor nit: use test_path_is_missing here (and elsewhere in the added\ntests).\n\n-Peff\n"},{"id":"255441","messageId":"20150130182456.GA29477@peff.net","threadId":"38452","inReplyTo":"xmqqzj91cfnl.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/1] apply: reject input that touches outside $cwd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-30T18:24:56Z","receivedAt":"2015-01-30T18:24:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 29, 2015 at 03:48:14PM -0800, Junio C Hamano wrote:\n\n> By default, a patch that affects outside the working area is\n> rejected as a mistake; Git itself never creates such a patch\n> unless the user bends backwards and specifies nonstandard\n> prefix to \"git diff\" and friends.\n> \n> When `git apply` is used without either `--index` or `--cached`\n> option as a \"better GNU patch\", the user can pass `--allow-uplevel`\n> option to override this safety check. This cannot be used to escape\n> outside the working tree when using `--index` or `--cached` to apply\n> the patch to the index.\n\nIt looks like your new --allow-uplevel goes to verify_path(). So this\nisn't just about \"..\", but it will also protect against applying a patch\ninside \".git\". Which seems like a good thing to me, but I wonder if the\noption name is a little misleading. It is really about applying the same\nchecks we do for index paths to the non-index mode of \"git apply\".\n\n> * Meant to apply on top of the previous one, but these two are\n> about separate and orthogonal issues.\n\nI agree they are orthogonal in concept, though I doubt the symlink tests\nhere would pass without the previous one (since verify_path does not\nknow or care about crossing symlink boundaries).\n\n-Peff\n"},{"id":"255442","messageId":"xmqqegqcccjt.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"20150130182456.GA29477@peff.net","subject":"Re: [PATCH 2/1] apply: reject input that touches outside $cwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-30T19:07:34Z","receivedAt":"2015-01-30T19:07:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It looks like your new --allow-uplevel goes to verify_path(). So this\n> isn't just about \"..\", but it will also protect against applying a patch\n> inside \".git\". Which seems like a good thing to me, but I wonder if the\n> option name is a little misleading.\n\nTrue; not just misleading but is incorrect, I would say.\nSuggestions?\n\n> I agree they are orthogonal in concept, though I doubt the symlink tests\n> here would pass without the previous one...\n\nIt won't; \"do not apply across symlinks\" is unconditional, and the\nnew codepath introduced by this patch, which is conditional to the\nuser option, shouldn't have to worry about them.\n"},{"id":"255443","messageId":"20150130191621.GA30156@peff.net","threadId":"38452","inReplyTo":"xmqqegqcccjt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/1] apply: reject input that touches outside $cwd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-30T19:16:21Z","receivedAt":"2015-01-30T19:16:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 30, 2015 at 11:07:34AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It looks like your new --allow-uplevel goes to verify_path(). So this\n> > isn't just about \"..\", but it will also protect against applying a patch\n> > inside \".git\". Which seems like a good thing to me, but I wonder if the\n> > option name is a little misleading.\n> \n> True; not just misleading but is incorrect, I would say.\n> Suggestions?\n\nI think just \"--verify-paths\" (and \"--no-verify-paths\", since the former\nwould be the default) might be fine. That leaves the definition of\n\"verify\" vague, but I think that's OK. It used to mean \"no '..' and no\n'.git'\", and now it has been widened to include \"no weird\nfilesystem-specific variants of .git\".\n\nIf you wanted to avoid the negative being the commonly used option,\nmaybe \"--unsafe-paths\" (or \"--allow-unsafe-paths\" if you like verbs).\n\n-Peff\n"},{"id":"255444","messageId":"xmqqa910cax2.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"20150130181153.GA25513@peff.net","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-30T19:42:49Z","receivedAt":"2015-01-30T19:42:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jan 29, 2015 at 12:45:22PM -0800, Junio C Hamano wrote:\n>\n>> +\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n>> +\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n>> +\t\t\t patch->new_name);\n>\n> Why does this not kick in when deleting a file?\n\nHalf-written logic, forgotten to be revisited (i.e. \"ok, anything\nthat is not delete we can check new_name, so do that first, later\nwe'd deal with deletion patch and I think the way to do so is by\nchecking old_name, but let's make sure this case works first\").\n\nThanks for catching.\n"},{"id":"255445","messageId":"20150130194615.GA30738@peff.net","threadId":"38452","inReplyTo":"xmqqa910cax2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-30T19:46:15Z","receivedAt":"2015-01-30T19:46:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 30, 2015 at 11:42:49AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Thu, Jan 29, 2015 at 12:45:22PM -0800, Junio C Hamano wrote:\n> >\n> >> +\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n> >> +\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n> >> +\t\t\t patch->new_name);\n> >\n> > Why does this not kick in when deleting a file?\n> \n> Half-written logic, forgotten to be revisited (i.e. \"ok, anything\n> that is not delete we can check new_name, so do that first, later\n> we'd deal with deletion patch and I think the way to do so is by\n> checking old_name, but let's make sure this case works first\").\n\nOK, I was worried I was missing something clever. :)\n\nI agree that checking patch->old_name should work in that case.\n\n-Peff\n"},{"id":"255446","messageId":"xmqq61bocao1.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"20150130181153.GA25513@peff.net","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-30T19:48:14Z","receivedAt":"2015-01-30T19:48:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n>> +\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n>> +\t\t\t patch->new_name);\n>\n> Why does this not kick in when deleting a file? If it is not OK to\n> add across a symlink, why is it OK to delete?\n\nHmph, adding\n\n\tif (patch->is_delete &&\tpath_is_beyond_symlink(patch->old_name))\n\t\treturn error(_(\"deleted file '%s' is beyond a symlink\"),\n\t\t\t\tpatch->old_name);\n\nseems to break t4114.11, which wants to apply this patch to a tree\nthat does not have a symbolic link but a directory at 'foo/'.\n\ndiff --git a/foo b/foo\nnew file mode 120000\nindex 0000000..ba0e162\n--- /dev/null\n+++ b/foo\n@@ -0,0 +1 @@\n+bar\n\\ No newline at end of file\ndiff --git a/foo/baz b/foo/baz\ndeleted file mode 100644\nindex 682c76b..0000000\n--- a/foo/baz\n+++ /dev/null\n@@ -1 +0,0 @@\n-if only I knew\n"},{"id":"255447","messageId":"20150130200731.GC30738@peff.net","threadId":"38452","inReplyTo":"xmqq61bocao1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-30T20:07:32Z","receivedAt":"2015-01-30T20:07:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 30, 2015 at 11:48:14AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> +\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n> >> +\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n> >> +\t\t\t patch->new_name);\n> >\n> > Why does this not kick in when deleting a file? If it is not OK to\n> > add across a symlink, why is it OK to delete?\n> \n> Hmph, adding\n> \n> \tif (patch->is_delete &&\tpath_is_beyond_symlink(patch->old_name))\n> \t\treturn error(_(\"deleted file '%s' is beyond a symlink\"),\n> \t\t\t\tpatch->old_name);\n> \n> seems to break t4114.11, which wants to apply this patch to a tree\n> that does not have a symbolic link but a directory at 'foo/'.\n> \n> diff --git a/foo b/foo\n> new file mode 120000\n> index 0000000..ba0e162\n> --- /dev/null\n> +++ b/foo\n> @@ -0,0 +1 @@\n> +bar\n> \\ No newline at end of file\n> diff --git a/foo/baz b/foo/baz\n> deleted file mode 100644\n> index 682c76b..0000000\n> --- a/foo/baz\n> +++ /dev/null\n> @@ -1 +0,0 @@\n> -if only I knew\n\nHrm. That only works in the current code because we apply the deletion\nin the directory (and then clean up the now-empty directory) first. So I\nthink you would need to check the paths progressively as you apply them,\nsince those other parts of the diff \"haven't happened yet\".\n\nIf we take deletion as one phase and addition as another, I think you\ncould get away with:\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex f5491cd..12c9d8e 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3549,7 +3549,7 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n \treturn 0;\n }\n \n-static int path_is_beyond_symlink(const char *name_)\n+static int path_is_beyond_symlink(const char *name_, int check_table)\n {\n \tstruct strbuf name = STRBUF_INIT;\n \n@@ -3562,7 +3562,8 @@ static int path_is_beyond_symlink(const char *name_)\n \t\tif (!name.len)\n \t\t\tbreak;\n \t\tname.buf[name.len] = '\\0';\n-\t\tprevious = in_fn_table(name.buf);\n+\n+\t\tprevious = check_table ? in_fn_table(name.buf) : NULL;\n \t\tif (previous) {\n \t\t\tif (!was_deleted(previous) &&\n \t\t\t !to_be_deleted(previous) &&\n@@ -3676,9 +3677,12 @@ static int check_patch(struct patch *patch)\n \t\t}\n \t}\n \n-\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n+\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name, 1))\n \t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n \t\t\t patch->new_name);\n+\tif (patch->is_delete && path_is_beyond_symlink(patch->old_name, 0))\n+\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n+\t\t\t patch->old_name);\n \n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n\n\nbut I suspect we could construct a case that depends more closely on the\norder of application. E.g., a patch that does:\n\n 1. add foo as a symlink\n 2. add file foo/bar\n\nis definitely wrong. But is:\n\n 1. add file foo/bar\n 2. add foo as a symlink\n\ndoes not technically fall afoul of the symlink rules. It is a _bogus_\npatch, of course, because the second part will get a D/F conflict. I am\nnot sure if there are any legitimate patches that could run into this\nordering problem, but even without it, it smells a bit funny to complain\nfor the wrong reason.\n\nLooking at the code, though, it seems like we should be doing these\nchecks progressively as we add entries to the fn_table. So that is doing\nthe right thing. It is only the deletion re-ordering that trips us up.\nCan we reorder all deletions before all additions before calling\ncheck_patch on each?\n\n-Peff\n"},{"id":"255448","messageId":"xmqq1tmcc9l9.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"xmqq61bocao1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-30T20:11:30Z","receivedAt":"2015-01-30T20:11:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>>> +\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n>>> +\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n>>> +\t\t\t patch->new_name);\n>>\n>> Why does this not kick in when deleting a file? If it is not OK to\n>> add across a symlink, why is it OK to delete?\n>\n> Hmph, adding\n>\n> \tif (patch->is_delete &&\tpath_is_beyond_symlink(patch->old_name))\n> \t\treturn error(_(\"deleted file '%s' is beyond a symlink\"),\n> \t\t\t\tpatch->old_name);\n>\n> seems to break t4114.11, which wants to apply this patch to a tree\n> that does not have a symbolic link but a directory at 'foo/'.\n>\n> diff --git a/foo b/foo\n> new file mode 120000\n> index 0000000..ba0e162\n> --- /dev/null\n> +++ b/foo\n> @@ -0,0 +1 @@\n> +bar\n> \\ No newline at end of file\n> diff --git a/foo/baz b/foo/baz\n> deleted file mode 100644\n> index 682c76b..0000000\n> --- a/foo/baz\n> +++ /dev/null\n> @@ -1 +0,0 @@\n> -if only I knew\n\n\nI am not sure how to fix this, without completely ripping out the\nmisguided \"We should be able to concatenate outputs from multiple\ninvocations of 'git diff' into a single file and apply the result\nwith a single invocation of 'git apply'\" change I grudgingly\naccepted long time ago (7a07841c (git-apply: handle a patch that\ntouches the same path more than once better, 2008-06-27).\n\n\"git diff\" output is designed each patch to apply independently to\nthe preimage to produce the postimage, and that allows patches to\ntwo files can be swapped via -Oorderfile mechanism, and also \"X was\ncreated by copying from Y and Y is modified in place\" will result in\nX with the contents of Y in the preimage (i.e. before the in-place\nmodification of Y in the same patch) regardless of the order of X\nand Y in the \"git diff\" output. The above input used by t4114.11\nexpects to remove 'foo/baz' (leaving an empty directory foo as an\nresult but we do not track directories so it can be nuked to make\nroom if other patch in the same input wants to put something else,\neither a regular file or a symbolic link, there) and create a blob\nat 'foo', and such an input should apply regardless of the order of\npatches in it.\n\nThe in_fn_table[] stuff broke that design completely.\n"},{"id":"255449","messageId":"20150130201620.GA4133@peff.net","threadId":"38452","inReplyTo":"xmqq1tmcc9l9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-30T20:16:20Z","receivedAt":"2015-01-30T20:16:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 30, 2015 at 12:11:30PM -0800, Junio C Hamano wrote:\n\n> I am not sure how to fix this, without completely ripping out the\n> misguided \"We should be able to concatenate outputs from multiple\n> invocations of 'git diff' into a single file and apply the result\n> with a single invocation of 'git apply'\" change I grudgingly\n> accepted long time ago (7a07841c (git-apply: handle a patch that\n> touches the same path more than once better, 2008-06-27).\n> \n> \"git diff\" output is designed each patch to apply independently to\n> the preimage to produce the postimage, and that allows patches to\n> two files can be swapped via -Oorderfile mechanism, and also \"X was\n> created by copying from Y and Y is modified in place\" will result in\n> X with the contents of Y in the preimage (i.e. before the in-place\n> modification of Y in the same patch) regardless of the order of X\n> and Y in the \"git diff\" output. The above input used by t4114.11\n> expects to remove 'foo/baz' (leaving an empty directory foo as an\n> result but we do not track directories so it can be nuked to make\n> room if other patch in the same input wants to put something else,\n> either a regular file or a symbolic link, there) and create a blob\n> at 'foo', and such an input should apply regardless of the order of\n> patches in it.\n> \n> The in_fn_table[] stuff broke that design completely.\n\nI had the impression that we did not apply in any arbitrary order that\ncould work, but rather that we did deletions first followed by\nadditions. But I am fairly ignorant of the apply code.\n\nIf that assumption is correct, then I think we could just follow the\nsame phases that the actual application does. Here's a hacky version\nbelow. Probably the check of phase versus is_delete needs to be better\n(and ideally the logic would be factored out of write_one_result so they\nalways match).\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex f5491cd..85364b8 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3593,7 +3593,7 @@ symlink_found:\n * Check and apply the patch in-core; leave the result in patch->result\n * for the caller to write it out to the final destination.\n */\n-static int check_patch(struct patch *patch)\n+static int check_patch(struct patch *patch, int phase)\n {\n \tstruct stat st;\n \tconst char *old_name = patch->old_name;\n@@ -3604,6 +3604,9 @@ static int check_patch(struct patch *patch)\n \tint ok_if_exists;\n \tint status;\n \n+\tif (!phase != patch->is_delete)\n+\t\treturn 0;\n+\n \tpatch->rejected = 1; /* we will drop this after we succeed */\n \n \tstatus = check_preimage(patch, &ce, &st);\n@@ -3679,6 +3682,9 @@ static int check_patch(struct patch *patch)\n \tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n \t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n \t\t\t patch->new_name);\n+\tif (patch->is_delete && path_is_beyond_symlink(patch->old_name))\n+\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n+\t\t\t patch->old_name);\n \n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n@@ -3686,7 +3692,7 @@ static int check_patch(struct patch *patch)\n \treturn 0;\n }\n \n-static int check_patch_list(struct patch *patch)\n+static int check_patch_list_1(struct patch *patch, int phase)\n {\n \tint err = 0;\n \n@@ -3695,12 +3701,22 @@ static int check_patch_list(struct patch *patch)\n \t\tif (apply_verbosely)\n \t\t\tsay_patch_name(stderr,\n \t\t\t\t _(\"Checking patch %s...\"), patch);\n-\t\terr |= check_patch(patch);\n+\t\terr |= check_patch(patch, phase);\n \t\tpatch = patch->next;\n \t}\n \treturn err;\n }\n \n+static int check_patch_list(struct patch *patch)\n+{\n+\tint err = 0;\n+\tint phase;\n+\n+\tfor (phase = 0; phase < 2; phase++)\n+\t\terr |= check_patch_list_1(patch, phase);\n+\treturn err;\n+}\n+\n /* This function tries to read the sha1 from the current index */\n static int get_current_sha1(const char *path, unsigned char *sha1)\n {\n"},{"id":"255450","messageId":"xmqqwq44auml.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"20150130201620.GA4133@peff.net","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-30T20:20:02Z","receivedAt":"2015-01-30T20:20:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I had the impression that we did not apply in any arbitrary order that\n> could work, but rather that we did deletions first followed by\n> additions. But I am fairly ignorant of the apply code.\n\nNo, you are thinking about the write-out of the finished result,\nwhich may have to turn existing directory to a file or vice versa on\nthe filesystem, but that happens _after_ we decide what to turn into\nwhat else, completely in-core.\n\nAnd the decision to determine what the input _means_ should not\ndepend on the order of patches in the input.\n"},{"id":"255451","messageId":"xmqqsiesau2g.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"20150130200731.GC30738@peff.net","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-30T20:32:07Z","receivedAt":"2015-01-30T20:32:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hrm. That only works in the current code because we apply the deletion\n> in the directory (and then clean up the now-empty directory) first. So I\n> think you would need to check the paths progressively as you apply them,\n> since those other parts of the diff \"haven't happened yet\".\n\nJust to make sure that I am not hallucinating, I added this one:\n\ndiff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh\nindex ebadbc3..83ddf62 100755\n--- a/t/t4114-apply-typechange.sh\n+++ b/t/t4114-apply-typechange.sh\n@@ -119,4 +119,12 @@ test_expect_success 'directory becomes symlink' '\n test_debug 'cat patch'\n \n \n+test_expect_success 'directory becomes symlink' '\n+\tgit checkout -f foo-becomes-a-directory &&\n+\tprintf \"%s\\n\" foo/baz foo >order &&\n+\tgit diff-tree -Oorder -p HEAD foo-symlinked-to-bar >patch &&\n+\tgit apply --index Jeff King writes:\n> \n> > I had the impression that we did not apply in any arbitrary order that\n> > could work, but rather that we did deletions first followed by\n> > additions. But I am fairly ignorant of the apply code.\n> \n> No, you are thinking about the write-out of the finished result,\n> which may have to turn existing directory to a file or vice versa on\n> the filesystem, but that happens _after_ we decide what to turn into\n> what else, completely in-core.\n> \n> And the decision to determine what the input _means_ should not\n> depend on the order of patches in the input.\n\nAh, OK. Yeah, doing it progressively can only be accurate if our\nname-checks follow the same order as applying, because we are checking\nagainst a particular state.\n\nBut could we instead pull this check to just before the write-out time?\nThat is, to let any horrible thing happen in-core, as long as what we\nwrite out to the index and the filesystem is sane?\n\n-Peff\n"},{"id":"255453","messageId":"xmqqmw50asan.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"20150130204805.GA10616@peff.net","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-30T21:10:24Z","receivedAt":"2015-01-30T21:10:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Ah, OK. Yeah, doing it progressively can only be accurate if our\n> name-checks follow the same order as applying, because we are checking\n> against a particular state.\n>\n> But could we instead pull this check to just before the write-out time?\n> That is, to let any horrible thing happen in-core, as long as what we\n> write out to the index and the filesystem is sane?\n\nThat would make me feel dirty.\n\nI noticed one thing. The PATH_TO_BE_DELETED/PATH_WAS_DELETED crud\nkicks in -only- during the actual application phase, and all patches\nthat remove paths from the end result should have been appropriately\nmarked in fn_table[] by the call to prepare_fn_table() at the\nbeginning of check_patch_list() as PATH_TO_BE_DELETED.\n\nBut it was wrong to call previous_patch() in my fix. The function\nis the cause of evil I see in the \"let's support concatenated patch,\nmaking the later patch depend on the result of earlier ones\" and\ndeliberately ignores PATH_TO_BE_DELETED patches. We would need to\ndo the early part of previous_patch() without the filtering.\n\nThis is a preparatory step to clean-up the mess I have in mind. It\ndoes not mean to change the semantics (applied to the codebase with\nor without the changes we have been discussing); it only makes it\nalways return the \"previous\" patch to the callers and makes them\nresponsible to see if the previous was to-be-deleted or was-deleted.\n\nWith that change, I think my symlink fix plus the \"check the deleted\none with old_name, too\" change has a better chance to do the moral\nequivalent of your two-phase thing. Essentially, \"First see what\nwill be deleted in the input as a whole\" has already been done by\nthe prepare_fn_table() thing.\n\n builtin/apply.c | 34 ++++++++++++----------------------\n 1 file changed, 12 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 41b7236..a064017 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3097,25 +3097,12 @@ static int checkout_target(struct cache_entry *ce, struct stat *st)\n \treturn 0;\n }\n \n-static struct patch *previous_patch(struct patch *patch, int *gone)\n+static struct patch *previous_patch(struct patch *patch)\n {\n-\tstruct patch *previous;\n-\n-\t*gone = 0;\n \tif (patch->is_copy || patch->is_rename)\n \t\treturn NULL; /* \"git\" patches do not depend on the order */\n \n-\tprevious = in_fn_table(patch->old_name);\n-\tif (!previous)\n-\t\treturn NULL;\n-\n-\tif (to_be_deleted(previous))\n-\t\treturn NULL; /* the deletion hasn't happened yet */\n-\n-\tif (was_deleted(previous))\n-\t\t*gone = 1;\n-\n-\treturn previous;\n+\treturn in_fn_table(patch->old_name);\n }\n \n static int verify_index_match(const struct cache_entry *ce, struct stat *st)\n@@ -3170,11 +3157,11 @@ static int load_preimage(struct image *image,\n \tstruct patch *previous;\n \tint status;\n \n-\tprevious = previous_patch(patch, &status);\n-\tif (status)\n+\tprevious = previous_patch(patch);\n+\tif (was_deleted(previous))\n \t\treturn error(_(\"path %s has been renamed/deleted\"),\n \t\t\t patch->old_name);\n-\tif (previous) {\n+\tif (previous && !to_be_deleted(previous)) {\n \t\t/* We have a patched copy in memory; use that. */\n \t\tstrbuf_add(&buf, previous->result, previous->resultsize);\n \t} else {\n@@ -3384,18 +3371,18 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n {\n \tconst char *old_name = patch->old_name;\n \tstruct patch *previous = NULL;\n-\tint stat_ret = 0, status;\n+\tint stat_ret = 0;\n \tunsigned st_mode = 0;\n \n \tif (!old_name)\n \t\treturn 0;\n \n \tassert(patch->is_new <= 0);\n-\tprevious = previous_patch(patch, &status);\n+\tprevious = previous_patch(patch);\n \n-\tif (status)\n+\tif (was_deleted(previous))\n \t\treturn error(_(\"path %s has been renamed/deleted\"), old_name);\n-\tif (previous) {\n+\tif (previous && !to_be_deleted(previous)) {\n \t\tst_mode = previous->new_mode;\n \t} else if (!cached) {\n \t\tstat_ret = lstat(old_name, st);\n@@ -3403,6 +3390,9 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \t\t\treturn error(_(\"%s: %s\"), old_name, strerror(errno));\n \t}\n \n+\tif (to_be_deleted(previous))\n+\t\tprevious = NULL;\n+\n \tif (check_index && !previous) {\n \t\tint pos = cache_name_pos(old_name, strlen(old_name));\n \t\tif (pos < 0) {\n"},{"id":"255455","messageId":"xmqqd25waqf9.fsf@gitster.dls.corp.google.com","threadId":"38452","inReplyTo":"20150130204805.GA10616@peff.net","subject":"Re: [PATCH] apply: refuse touching a file beyond symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-30T21:50:50Z","receivedAt":"2015-01-30T21:50:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But could we instead pull this check to just before the write-out time?\n> That is, to let any horrible thing happen in-core, as long as what we\n> write out to the index and the filesystem is sane?\n\nThe check in-core is somewhat tricky, because we would need to (1)\ncatch a patch that creates a symlink and also a file as if that new\nsymlink is a directory and (2) allow a patch that removes a symlink\nand also a file in a new directory at the location removed symlink\nused to occupy.\n\nFor (1) we need to see if there is a patch in the entire input that\ncreates a symbolic link and reject the input. For (2) we need to\nsee if there is a patch that removes the symbolic link. (1) cannot\nbe caught with the approach based on fn_table[], which is inherently\nmeant to help incremental application, that is oblivious to a path\nthat will materialize after applying a later patch in the input.\n\nLet me think about it a bit more. The fix probably needs to abandon\ndepending on fn_table[] stuff, if we want to do in the \"sanity check\nthe input and compute the final state all in-core\" route.\n"},{"id":"255483","messageId":"majhc9$ant$1@ger.gmane.org","threadId":"38452","inReplyTo":"ma8btn$t01$2@ger.gmane.org","subject":"Re: patch-2.7.3 no longer applies relative symbolic link patches","fromName":"Andreas Gruenbacher","fromEmail":"agruen@gnu.org","sentAt":"2015-01-31T21:27:37Z","receivedAt":"2015-01-31T21:27:37Z","isPatch":false,"sender":{"key":"agruen@gnu.org","avatar":null},"body":"On Tue, 27 Jan 2015 15:47:04 +0000, Andreas Gruenbacher wrote:\n> On Mon, 26 Jan 2015 13:50:10 -0800, Linus Torvalds wrote:\n>> I _think_ we allow arbitrary symlinks to be created, but then we should\n>> be careful about actually _following_ them.\n> \n> I would prefer to allow arbitrary symlinks even in GNU patch, but patch\n> still must not be allowed to leave the working directory. The only way\n> to achieve that I can think of is to implement path traversal in user\n> space, which is not so easy to do correctly and efficiently.\n\nThis should be working in patch-2.7.4 now.\n\nAndreas\n"}]}