dexonsmith added a comment. In D121733#3393640 <https://reviews.llvm.org/D121733#3393640>, @dexonsmith wrote:
> In D121733#3393552 <https://reviews.llvm.org/D121733#3393552>, @bnbarham > wrote: > >> In D121733#3393546 <https://reviews.llvm.org/D121733#3393546>, @rnk wrote: >> >>> I've been somewhat afraid to touch this code because of symlinks. Is that >>> something we need to worry about? Consider this path: >>> root/symlink/../foo.h. remove_dots will turn this into root/foo.h, but if >>> symlink points to some unrelated directory, the .. directory entry points >>> to something other than root. I can't imagine that people rely on this >>> behavior often, but I could be wrong. >> >> `remove_dots` doesn't remove `..` by default, it needs to be passed `true` >> to do that. This is only removing `.`. > > Yeah, I'd be against doing `..`-removal unless we had a cheap-enough way to > detect if that was symlink-incorrect and skipped it in that case. But maybe > it'd be worth a comment here explicitly saying `..`s were A couple more thoughts: - Probably we should change getDirectoryRef at the same time. - Mabye this cleanup could mostly contained to there (rather than having it in both places). - Rely on `getDirectoryRef(parent_path(Filename))` to clean up the directory. - Construct "clean(er)" filename from `Dir->getName()` and `sys::path::filename(Filename))`. - Call `remove_leading_dotslash()` in case the dir is `"."`. - ... then go down and stat the file. - I was a bit worried about the hacks for the VFS with "use-external-names=true" interfering with my suggestion above, but I looked and I think it's okay. - Hacks: see logic for when `Status.getName() != Filename`: `Filename` gets replaced with what `Status` returned. - Seems like `getDirectoryRef()` doesn't have this hack so I don't think it'll interfere here. - BTW, @bnbarham and I talked through a way to remove this hack (to model exposing the external name without replacing the access name); hopefully it won't stick around too much longer! - It's probably not very hard/expensive to do `..`-removal correctly (correctly in all cases, by not removing `..`s unless `stat` proves it's safe). - Call `getDirectoryRef()` as above. - If there are any internal "..", try removing them and call `getDirectoryRef()` again. If they're successful and have the same inode, use the cleaner name. - The extra stat happens only once per directory that has a `..` component. Maybe not too bad? - ... again, probably better to do this internally to `getDirectoryRef()` - ... certainly seems better to land this as a second incremental step, since there's more potential for trouble. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/ https://reviews.llvm.org/D121733 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
