[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: r243597 - Avoid failure to canonicalize '..'.
From: Sean Silva <chisophugis () gmail ! com>
Date: 2015-07-30 1:13:26
Message-ID: CAHnXoakvnXksGOkE97KiLd6HN-xJnSjZjRujD7+ETssdw-R6zQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Wed, Jul 29, 2015 at 6:11 PM, Richard Smith <richard@metafoo.co.uk>
wrote:
> On Wed, Jul 29, 2015 at 5:26 PM, Sean Silva <chisophugis@gmail.com> wrote:
>
> > Author: silvas
> > Date: Wed Jul 29 19:26:34 2015
> > New Revision: 243597
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=243597&view=rev
> > Log:
> > Avoid failure to canonicalize '..'.
> >
> > Also fix completely broken and untested code which was hiding the
> > primary bug. The !LLVM_ON_UNIX branch of the ifdef was actually a no-op.
> >
> > I ran into this in the wild. It was causing failures in our SDK build.
> >
> > Ideally we'd have a perfect llvm::sys::fs::canonical, but at least this
> > is a step in the right direction, and fixes an obviously broken case.
> > In some sense the test case I've added here is an integration test. We
> > should have these routines thoroughly unit tested in llvm::sys::fs.
> >
>
> This may be correct for the !LLVM_ON_UNIX case (I'm not sure whether the
> path foo/bar/.. is always the same as foo on Windows), but it's wrong for
> the LLVM_ON_UNIX case. Please revert (except for the bugfix in
> getCanonicalName); the right fix is to fix the places that are introducing
> the unwanted .. path components.
>
I ran into this in the wild where they came from users.
-- Sean Silva
>
> Added:
> > cfe/trunk/test/Modules/Inputs/module-map-path-hash/
> > cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h
> > cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap
> > cfe/trunk/test/Modules/module-map-path-hash.cpp
> > Modified:
> > cfe/trunk/lib/Basic/FileManager.cpp
> >
> > Modified: cfe/trunk/lib/Basic/FileManager.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=243597&r1=243596&r2=243597&view=diff
> >
> >
> > ==============================================================================
> > --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> > +++ cfe/trunk/lib/Basic/FileManager.cpp Wed Jul 29 19:26:34 2015
> > @@ -514,7 +514,7 @@ void FileManager::modifyFileEntry(FileEn
> > File->ModTime = ModificationTime;
> > }
> >
> > -/// Remove '.' path components from the given absolute path.
> > +/// Remove '.' and '..' path components from the given absolute path.
> > /// \return \c true if any changes were made.
> > // FIXME: Move this to llvm::sys::path.
> > bool FileManager::removeDotPaths(SmallVectorImpl<char> &Path) {
> > @@ -525,24 +525,24 @@ bool FileManager::removeDotPaths(SmallVe
> >
> > // Skip the root path, then look for traversal in the components.
> > StringRef Rel = path::relative_path(P);
> > - bool AnyDots = false;
> > for (StringRef C : llvm::make_range(path::begin(Rel), path::end(Rel)))
> > {
> > - if (C == ".") {
> > - AnyDots = true;
> > + if (C == ".")
> > + continue;
> > + if (C == "..") {
> > + if (!ComponentStack.empty())
> > + ComponentStack.pop_back();
> > continue;
> > }
> > ComponentStack.push_back(C);
> > }
> >
> > - if (!AnyDots)
> > - return false;
> > -
> > SmallString<256> Buffer = path::root_path(P);
> > for (StringRef C : ComponentStack)
> > path::append(Buffer, C);
> >
> > + bool Changed = (Path != Buffer);
> > Path.swap(Buffer);
> > - return true;
> > + return Changed;
> > }
> >
> > StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
> > @@ -567,6 +567,9 @@ StringRef FileManager::getCanonicalName(
> > llvm::sys::fs::make_absolute(CanonicalNameBuf);
> > llvm::sys::path::native(CanonicalNameBuf);
> > removeDotPaths(CanonicalNameBuf);
> > + char *Mem =
> > CanonicalNameStorage.Allocate<char>(CanonicalNameBuf.size());
> > + memcpy(Mem, CanonicalNameBuf.data(), CanonicalNameBuf.size());
> > + CanonicalName = StringRef(Mem, CanonicalNameBuf.size());
> > #endif
> >
> > CanonicalDirNames.insert(std::make_pair(Dir, CanonicalName));
> >
> > Added: cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h?rev=243597&view=auto
> >
> > ==============================================================================
> > --- cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h (added)
> > +++ cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h Wed Jul 29
> > 19:26:34 2015
> > @@ -0,0 +1,2 @@
> > +#pragma once
> > +int a = 42;
> >
> > Added: cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap?rev=243597&view=auto
> >
> > ==============================================================================
> > --- cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap
> > (added)
> > +++ cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap
> > Wed Jul 29 19:26:34 2015
> > @@ -0,0 +1,3 @@
> > +module a {
> > + header "a.h"
> > +}
> >
> > Added: cfe/trunk/test/Modules/module-map-path-hash.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/module-map-path-hash.cpp?rev=243597&view=auto
> >
> > ==============================================================================
> > --- cfe/trunk/test/Modules/module-map-path-hash.cpp (added)
> > +++ cfe/trunk/test/Modules/module-map-path-hash.cpp Wed Jul 29 19:26:34
> > 2015
> > @@ -0,0 +1,9 @@
> > +// rm -rf %t
> > +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++
> > -Rmodule-build -I%S/Inputs/module-map-path-hash -fmodules-cache-path=%t
> > -fsyntax-only %s
> > +// xUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++
> > -Rmodule-build -I%S/Inputs//module-map-path-hash -fmodules-cache-path=%t
> > -fsyntax-only %s 2>&1 | FileCheck -allow-empty %s
> > +// xUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++
> > -Rmodule-build -I%S/Inputs/./module-map-path-hash -fmodules-cache-path=%t
> > -fsyntax-only %s 2>&1 | FileCheck -allow-empty %s
> >
>
> Did you mean to commit with disabled RUN lines here?
>
>
> > +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++
> > -Rmodule-build -I%S/Inputs/../Inputs/module-map-path-hash
> > -fmodules-cache-path=%t -fsyntax-only %s 2>&1 | FileCheck -allow-empty %s
> > +
> > +#include "a.h"
> > +
> > +// CHECK-NOT: remark: building module
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits@cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
>
[Attachment #5 (text/html)]
<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul \
29, 2015 at 6:11 PM, Richard Smith <span dir="ltr"><<a \
href="mailto:richard@metafoo.co.uk" \
target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div \
class="gmail_quote"><span class="">On Wed, Jul 29, 2015 at 5:26 PM, Sean Silva <span \
dir="ltr"><<a href="mailto:chisophugis@gmail.com" \
target="_blank">chisophugis@gmail.com</a>></span> wrote:<br></span><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span class="">Author: silvas<br>
Date: Wed Jul 29 19:26:34 2015<br>
New Revision: 243597<br>
<br></span>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm \
-2Dproject-3Frev-3D243597-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW \
_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=0PkuA2uHjglNfs2SmOR5XLymJDS7NOCUKKYoTxEbdyw&s=fPJFea0TIkR9UFZ-UgE6I7rSaa0A1_8lOmFGeKG7CKs&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=243597&view=rev</a><span \
class=""><br> Log:<br>
Avoid failure to canonicalize '..'.<br>
<br>
Also fix completely broken and untested code which was hiding the<br>
primary bug. The !LLVM_ON_UNIX branch of the ifdef was actually a no-op.<br>
<br>
I ran into this in the wild. It was causing failures in our SDK build.<br>
<br>
Ideally we'd have a perfect llvm::sys::fs::canonical, but at least this<br>
is a step in the right direction, and fixes an obviously broken case.<br>
In some sense the test case I've added here is an integration test. We<br>
should have these routines thoroughly unit tested in \
llvm::sys::fs.<br></span></blockquote><div><br></div><div>This may be correct for the \
!LLVM_ON_UNIX case (I'm not sure whether the path foo/bar/.. is always the same \
as foo on Windows), but it's wrong for the LLVM_ON_UNIX case. Please revert \
(except for the bugfix in getCanonicalName); the right fix is to fix the places that \
are introducing the unwanted .. path \
components.</div></div></div></div></blockquote><div><br></div><div>I ran into this \
in the wild where they came from users.</div><div><br></div><div>-- Sean \
Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div \
class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span class=""> Added:<br>
cfe/trunk/test/Modules/Inputs/module-map-path-hash/<br>
cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h<br>
cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap<br>
cfe/trunk/test/Modules/module-map-path-hash.cpp<br>
Modified:<br>
cfe/trunk/lib/Basic/FileManager.cpp<br>
<br>
Modified: cfe/trunk/lib/Basic/FileManager.cpp<br></span>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm \
-2Dproject_cfe_trunk_lib_Basic_FileManager.cpp-3Frev-3D243597-26r1-3D243596-26r2-3D243 \
597-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASR \
OnREeq0cCxk&m=0PkuA2uHjglNfs2SmOR5XLymJDS7NOCUKKYoTxEbdyw&s=54uYlNS2HO43B9GDga8Y-4uRXjKoYdfp4s-d98jdLyw&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Bas \
ic/FileManager.cpp?rev=243597&r1=243596&r2=243597&view=diff</a><div><div \
class="h5"><br> ==============================================================================<br>
--- cfe/trunk/lib/Basic/FileManager.cpp (original)<br>
+++ cfe/trunk/lib/Basic/FileManager.cpp Wed Jul 29 19:26:34 2015<br>
@@ -514,7 +514,7 @@ void FileManager::modifyFileEntry(FileEn<br>
File->ModTime = ModificationTime;<br>
}<br>
<br>
-/// Remove '.' path components from the given absolute path.<br>
+/// Remove '.' and '..' path components from the given absolute \
path.<br> /// \return \c true if any changes were made.<br>
// FIXME: Move this to llvm::sys::path.<br>
bool FileManager::removeDotPaths(SmallVectorImpl<char> &Path) {<br>
@@ -525,24 +525,24 @@ bool FileManager::removeDotPaths(SmallVe<br>
<br>
// Skip the root path, then look for traversal in the components.<br>
StringRef Rel = path::relative_path(P);<br>
- bool AnyDots = false;<br>
for (StringRef C : llvm::make_range(path::begin(Rel), path::end(Rel))) {<br>
- if (C == ".") {<br>
- AnyDots = true;<br>
+ if (C == ".")<br>
+ continue;<br>
+ if (C == "..") {<br>
+ if (!ComponentStack.empty())<br>
+ ComponentStack.pop_back();<br>
continue;<br>
}<br>
ComponentStack.push_back(C);<br>
}<br>
<br>
- if (!AnyDots)<br>
- return false;<br>
-<br>
SmallString<256> Buffer = path::root_path(P);<br>
for (StringRef C : ComponentStack)<br>
path::append(Buffer, C);<br>
<br>
+ bool Changed = (Path != Buffer);<br>
Path.swap(Buffer);<br>
- return true;<br>
+ return Changed;<br>
}<br>
<br>
StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {<br>
@@ -567,6 +567,9 @@ StringRef FileManager::getCanonicalName(<br>
llvm::sys::fs::make_absolute(CanonicalNameBuf);<br>
llvm::sys::path::native(CanonicalNameBuf);<br>
removeDotPaths(CanonicalNameBuf);<br>
+ char *Mem = CanonicalNameStorage.Allocate<char>(CanonicalNameBuf.size());<br>
+ memcpy(Mem, CanonicalNameBuf.data(), CanonicalNameBuf.size());<br>
+ CanonicalName = StringRef(Mem, CanonicalNameBuf.size());<br>
#endif<br>
<br>
CanonicalDirNames.insert(std::make_pair(Dir, CanonicalName));<br>
<br>
Added: cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h<br></div></div>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm \
-2Dproject_cfe_trunk_test_Modules_Inputs_module-2Dmap-2Dpath-2Dhash_a.h-3Frev-3D243597 \
-26view-3Dauto&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnR \
Eeq0cCxk&m=0PkuA2uHjglNfs2SmOR5XLymJDS7NOCUKKYoTxEbdyw&s=V3_4QPJ87XWu6Jc2yQxauDJXHksSyM6XrCr1pJyWSDw&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h?rev=243597&view=auto</a><span \
class=""><br> ==============================================================================<br>
--- cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h (added)<br>
+++ cfe/trunk/test/Modules/Inputs/module-map-path-hash/a.h Wed Jul 29 19:26:34 \
2015<br> @@ -0,0 +1,2 @@<br>
+#pragma once<br>
+int a = 42;<br>
<br>
Added: cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap<br></span>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm \
-2Dproject_cfe_trunk_test_Modules_Inputs_module-2Dmap-2Dpath-2Dhash_module.modulemap-3 \
Frev-3D243597-26view-3Dauto&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70 \
KdZISM_ASROnREeq0cCxk&m=0PkuA2uHjglNfs2SmOR5XLymJDS7NOCUKKYoTxEbdyw&s=sMde_0XOBjFYrVFv9dxvUjJwUnNNHHGkb0nW40L4Cdo&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Mo \
dules/Inputs/module-map-path-hash/module.modulemap?rev=243597&view=auto</a><span \
class=""><br> ==============================================================================<br>
--- cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap (added)<br>
+++ cfe/trunk/test/Modules/Inputs/module-map-path-hash/module.modulemap Wed Jul 29 \
19:26:34 2015<br> @@ -0,0 +1,3 @@<br>
+module a {<br>
+ header "a.h"<br>
+}<br>
<br>
Added: cfe/trunk/test/Modules/module-map-path-hash.cpp<br></span>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm \
-2Dproject_cfe_trunk_test_Modules_module-2Dmap-2Dpath-2Dhash.cpp-3Frev-3D243597-26view \
-3Dauto&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCx \
k&m=0PkuA2uHjglNfs2SmOR5XLymJDS7NOCUKKYoTxEbdyw&s=IyiQdsBkEjSgsPJqMjZmGbqjFf9T1BOWwSiaZtxChPI&e=" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/module-map-path-hash.cpp?rev=243597&view=auto</a><span \
class=""><br> ==============================================================================<br>
--- cfe/trunk/test/Modules/module-map-path-hash.cpp (added)<br>
+++ cfe/trunk/test/Modules/module-map-path-hash.cpp Wed Jul 29 19:26:34 2015<br>
@@ -0,0 +1,9 @@<br>
+// rm -rf %t<br>
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++ -Rmodule-build \
-I%S/Inputs/module-map-path-hash -fmodules-cache-path=%t -fsyntax-only %s<br> +// \
xUN: %clang_cc1 -fmodules -fimplicit-module-maps -x c++ -Rmodule-build \
-I%S/Inputs//module-map-path-hash -fmodules-cache-path=%t -fsyntax-only %s \
2>&1 | FileCheck -allow-empty %s<br> +// xUN: %clang_cc1 -fmodules \
-fimplicit-module-maps -x c++ -Rmodule-build -I%S/Inputs/./module-map-path-hash \
-fmodules-cache-path=%t -fsyntax-only %s 2>&1 | FileCheck -allow-empty \
%s<br></span></blockquote><div><br></div><div>Did you mean to commit with disabled \
RUN lines here?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""> +// RUN: %clang_cc1 \
-fmodules -fimplicit-module-maps -x c++ -Rmodule-build \
-I%S/Inputs/../Inputs/module-map-path-hash -fmodules-cache-path=%t -fsyntax-only %s \
2>&1 | FileCheck -allow-empty %s<br> +<br>
+#include "a.h"<br>
+<br>
+// CHECK-NOT: remark: building module<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
</span><a href="mailto:cfe-commits@cs.uiuc.edu" \
target="_blank">cfe-commits@cs.uiuc.edu</a><br> <a \
href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" \
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br> \
</blockquote></div><br></div></div> </blockquote></div><br></div></div>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic