From 7378ec85f0f4c7e1ee2ea92fb7e55fdcb3435f01 Mon Sep 17 00:00:00 2001 From: "A.J. Beamon" Date: Tue, 26 Jun 2018 16:30:47 -0700 Subject: [PATCH 01/12] Fixes parentDirectory to work when our path is a child of the root directory. Also works when the path has trailing slashes. --- fdbmonitor/fdbmonitor.cpp | 14 +++++++++----- flow/Platform.cpp | 9 +++++++-- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/fdbmonitor/fdbmonitor.cpp b/fdbmonitor/fdbmonitor.cpp index c1b57dfa6d..b5a74fec10 100644 --- a/fdbmonitor/fdbmonitor.cpp +++ b/fdbmonitor/fdbmonitor.cpp @@ -240,13 +240,17 @@ std::string abspath(std::string const& filename) { return std::string(r); } -std::string parentDirectory(std::string const& filename) { +std::string parentDirectory(std::string filename) { size_t sep = filename.find_last_of( CANONICAL_PATH_SEPARATOR ); if (sep == std::string::npos) { return ""; } - return filename.substr(0, sep); + while(filename.size() && (filename.back() == '/' || filename.back() == CANONICAL_PATH_SEPARATOR)) { + filename = filename.substr(0, filename.size()-1); + } + + return filename.substr(0, sep+1); } int mkdir(std::string const& directory) { @@ -944,7 +948,7 @@ std::unordered_map> set_watches(std::string if(exists) { log_msg(SevInfo, "Watching parent directory of symlink %s (%d)\n", subpath.c_str(), wd); - additional_watch_wds[wd].insert(subpath.substr(parent.size()+1)); + additional_watch_wds[wd].insert(subpath.substr(parent.size())); } else { /* If the subpath has appeared since we set the watch, we should cancel it and resume traversing the path */ @@ -955,7 +959,7 @@ std::unordered_map> set_watches(std::string } log_msg(SevInfo, "Watching parent directory of missing directory %s (%d)\n", subpath.c_str(), wd); - additional_watch_wds[wd].insert(subpath.substr(parent.size()+1)); + additional_watch_wds[wd].insert(subpath.substr(parent.size())); break; } @@ -1043,7 +1047,7 @@ int main(int argc, char** argv) { // Will always succeed given an absolute path std::string confdir = parentDirectory(confpath); - std::string conffile = confpath.substr(confdir.size()+1); + std::string conffile = confpath.substr(confdir.size()); #ifdef __linux__ // Setup inotify diff --git a/flow/Platform.cpp b/flow/Platform.cpp index 88ac4ef587..f07cc9338d 100644 --- a/flow/Platform.cpp +++ b/flow/Platform.cpp @@ -1633,7 +1633,7 @@ void atomicReplace( std::string const& path, std::string const& content ) { try { INJECT_FAULT( io_error, "atomicReplace" ); - std::string tempfilename = parentDirectory(path) + CANONICAL_PATH_SEPARATOR + g_random->randomUniqueID().toString() + ".tmp"; + std::string tempfilename = joinPath(parentDirectory(path), g_random->randomUniqueID().toString() + ".tmp"); f = fopen( tempfilename.c_str(), "wt" ); if(!f) throw io_error(); @@ -1812,7 +1812,12 @@ std::string parentDirectory( std::string const& filename ) { .GetLastError(); throw platform_error(); } - return abs.substr(0, sep); + + while (abs.size() && (abs.back() == '/' || abs.back() == CANONICAL_PATH_SEPARATOR)) { + abs = abs.substr(0, abs.size()-1); + } + + return abs.substr(0, sep+1); } std::string getUserHomeDirectory() { From e17c04f666329e76f2e8aebb5ede288010b390b8 Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Mon, 2 Jul 2018 14:25:24 -0700 Subject: [PATCH 02/12] Changed assignments to substr() to resize() for clarity/simplicity. --- fdbmonitor/fdbmonitor.cpp | 2 +- flow/Platform.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fdbmonitor/fdbmonitor.cpp b/fdbmonitor/fdbmonitor.cpp index b5a74fec10..f37c8b6639 100644 --- a/fdbmonitor/fdbmonitor.cpp +++ b/fdbmonitor/fdbmonitor.cpp @@ -247,7 +247,7 @@ std::string parentDirectory(std::string filename) { } while(filename.size() && (filename.back() == '/' || filename.back() == CANONICAL_PATH_SEPARATOR)) { - filename = filename.substr(0, filename.size()-1); + filename.resize(filename.size() - 1); } return filename.substr(0, sep+1); diff --git a/flow/Platform.cpp b/flow/Platform.cpp index f07cc9338d..b778c270a6 100644 --- a/flow/Platform.cpp +++ b/flow/Platform.cpp @@ -1814,7 +1814,7 @@ std::string parentDirectory( std::string const& filename ) { } while (abs.size() && (abs.back() == '/' || abs.back() == CANONICAL_PATH_SEPARATOR)) { - abs = abs.substr(0, abs.size()-1); + abs.resize(abs.size() - 1); } return abs.substr(0, sep+1); From 2efb6f4c0d12ab2a7fd1598df6a7cfa04b3f63f1 Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Fri, 15 Mar 2019 23:54:33 -0700 Subject: [PATCH 03/12] Added cleanPath() which puts a path in a canonical form without .., ., or duplicate separators without using the filesystem or resolving symbolic links. absPath() redefined to use cleanPath() so it will return the same result for a path without symbolic links regardless of whether or not the path actually exists. Redefined parentDirectory() to use absPath() and error on certain inputs. Added comments describing behavior of these functions, and added a unit test which verbosely tests many inputs to them. --- fdbmonitor/fdbmonitor.cpp | 2 + fdbserver/worker.actor.cpp | 4 +- flow/Platform.cpp | 159 +++++++++++++++++++++++++++++++++---- flow/Platform.h | 8 +- 4 files changed, 154 insertions(+), 19 deletions(-) diff --git a/fdbmonitor/fdbmonitor.cpp b/fdbmonitor/fdbmonitor.cpp index d025891284..c4852b7daa 100644 --- a/fdbmonitor/fdbmonitor.cpp +++ b/fdbmonitor/fdbmonitor.cpp @@ -244,6 +244,8 @@ std::string abspath(std::string const& filename) { return std::string(r); } +// Get the parent directory of a filename *without* resolving symbolic links so that fdbmonitor can support +// symbolic links updates as a way of atomitically changing the configuration file std::string parentDirectory(std::string filename) { size_t sep = filename.find_last_of( CANONICAL_PATH_SEPARATOR ); if (sep == std::string::npos) { diff --git a/fdbserver/worker.actor.cpp b/fdbserver/worker.actor.cpp index 187f7cadcf..af101826b1 100644 --- a/fdbserver/worker.actor.cpp +++ b/fdbserver/worker.actor.cpp @@ -635,6 +635,8 @@ ACTOR Future workerServer( Reference connFile, Refe state WorkerInterface interf( locality ); + folder = abspath(folder); + if(metricsPrefix.size() > 0) { if( metricsConnFile.size() > 0) { try { @@ -725,7 +727,7 @@ ACTOR Future workerServer( Reference connFile, Refe StringRef optionsString = StringRef(filename).removePrefix(fileVersionedLogDataPrefix).eat("-"); logQueueBasename = fileLogQueuePrefix.toString() + optionsString.toString() + "-"; } - ASSERT_WE_THINK( StringRef( parentDirectory(s.filename) ).endsWith( StringRef(folder) ) ); + ASSERT_WE_THINK( abspath(parentDirectory(s.filename)) == folder ); IKeyValueStore* kv = openKVStore( s.storeType, s.filename, s.storeID, memoryLimit, validateDataFiles ); IDiskQueue* queue = openDiskQueue( joinPath( folder, logQueueBasename + s.storeID.toString() + "-"), tlogQueueExtension.toString(), s.storeID, 10*SERVER_KNOBS->TARGET_BYTES_PER_TLOG); diff --git a/flow/Platform.cpp b/flow/Platform.cpp index 9dd1f586e7..a2bef2d981 100644 --- a/flow/Platform.cpp +++ b/flow/Platform.cpp @@ -1629,7 +1629,7 @@ std::string joinPath( std::string const& directory, std::string const& filename while (f.size() && (f[0] == '/' || f[0] == CANONICAL_PATH_SEPARATOR)) f = f.substr(1); while (d.size() && (d.back() == '/' || d.back() == CANONICAL_PATH_SEPARATOR)) - d = d.substr(0, d.size()-1); + d.resize(d.size() - 1); return d + CANONICAL_PATH_SEPARATOR + f; } @@ -1796,7 +1796,55 @@ bool createDirectory( std::string const& directory ) { }; // namespace platform +const uint8_t separatorChar = CANONICAL_PATH_SEPARATOR; +StringRef separator(&separatorChar, 1); +StringRef dotdot = LiteralStringRef(".."); + +std::string cleanPath(std::string const &path) { + std::vector parts; + std::vector finalParts; + bool absolute = !path.empty() && path[0] == CANONICAL_PATH_SEPARATOR; + + StringRef p(path); + + while(p.size() != 0) { + StringRef part = p.eat(separator); + if(part.size() == 0 || (part.size() == 1 && part[0] == '.')) + continue; + if(part == dotdot) { + if(!finalParts.empty() && finalParts.back() != dotdot) { + finalParts.pop_back(); + continue; + } + if(absolute) { + TraceEvent(SevWarnAlways, "CleanPathError").detail("Path", path); + throw platform_error(); + } + } + finalParts.push_back(part); + } + + std::string result; + result.reserve(PATH_MAX); + if(absolute) { + result.append(1, CANONICAL_PATH_SEPARATOR); + } + + for(int i = 0; i < finalParts.size(); ++i) { + if(i != 0) { + result.append(1, CANONICAL_PATH_SEPARATOR); + } + result.append((const char *)finalParts[i].begin(), finalParts[i].size()); + } + + return result; +} + std::string abspath( std::string const& filename ) { + if(filename.empty()) { + throw platform_error(); + } + // Returns an absolute path canonicalized to use only CANONICAL_PATH_SEPARATOR INJECT_FAULT( platform_error, "abspath" ); @@ -1817,13 +1865,11 @@ std::string abspath( std::string const& filename ) { auto r = realpath( filename.c_str(), result ); if (!r) { if (errno == ENOENT) { - int sep = filename.find_last_of( CANONICAL_PATH_SEPARATOR ); - if (sep != std::string::npos) { - return joinPath( abspath( filename.substr(0, sep) ), filename.substr(sep) ); - } - else if (filename.find("~") == std::string::npos) { - return joinPath( abspath( "." ), filename ); + bool absolute = !filename.empty() && filename.front() == CANONICAL_PATH_SEPARATOR; + if(absolute) { + return cleanPath(filename); } + return cleanPath(joinPath(platform::getWorkingDirectory(), filename)); } Error e = systemErrorCodeToError(); TraceEvent(SevError, "AbsolutePathError").detail("Filename", filename).GetLastError().error(e); @@ -1843,20 +1889,22 @@ std::string basename( std::string const& filename ) { } std::string parentDirectory( std::string const& filename ) { - auto abs = abspath(filename); - size_t sep = abs.find_last_of( CANONICAL_PATH_SEPARATOR ); - if (sep == std::string::npos) { - TraceEvent(SevError, "GetParentDirectoryOfFile") + std::string abs = abspath(filename); + + // Can't take the parent of empty, root, or anything ending in an unresolved .. + if(abs.empty() + || (abs.size() == 1 && abs.front() == CANONICAL_PATH_SEPARATOR) + || abs == ".." + || (abs.size() > 2 && StringRef(abs).endsWith(dotdot) && abs[abs.size() - 3] == CANONICAL_PATH_SEPARATOR) + ) { + TraceEvent(SevWarnAlways, "GetParentDirectoryOfFile") .detail("File", filename) .GetLastError(); throw platform_error(); } - while (abs.size() && (abs.back() == '/' || abs.back() == CANONICAL_PATH_SEPARATOR)) { - abs.resize(abs.size() - 1); - } - - return abs.substr(0, sep+1); + abs.resize(abs.find_last_of( CANONICAL_PATH_SEPARATOR ) + 1); + return abs; } std::string getUserHomeDirectory() { @@ -2842,3 +2890,82 @@ TEST_CASE("/flow/Platform/getMemoryInfo") { return Void(); } #endif + +int testAbsPath(std::string a, ErrorOr b) { + ErrorOr result; + try { result = abspath(a); } catch(Error &e) { result = e; } + + bool r = result.isError() == b.isError() && (b.isError() || b.get() == result.get()) && (!b.isError() || b.getError().code() == result.getError().code()); + + printf("abspath('%s') -> %s", a.c_str(), result.isError() ? result.getError().what() : format("'%s'", result.get().c_str()).c_str()); + if(!r) { + printf(" ERROR expected %s", b.isError() ? b.getError().what() : format("'%s'", b.get().c_str()).c_str()); + } + printf("\n"); + return r ? 0 : 1; +} + +int testPathFunction(const char *name, std::function fun, std::string a, ErrorOr b) { + ErrorOr result; + try { result = fun(a); } catch(Error &e) { result = e; } + bool r = result.isError() == b.isError() && (b.isError() || b.get() == result.get()) && (!b.isError() || b.getError().code() == result.getError().code()); + + printf("%s('%s') -> %s", name, a.c_str(), result.isError() ? result.getError().what() : format("'%s'", result.get().c_str()).c_str()); + if(!r) { + printf(" ERROR expected %s", b.isError() ? b.getError().what() : format("'%s'", b.get().c_str()).c_str()); + } + printf("\n"); + return r ? 0 : 1; +} + +#ifdef __unixish__ +TEST_CASE("/flow/Platform/directoryOps") { + int errors = 0; + + errors += testPathFunction("cleanPath", cleanPath, "", ""); + + errors += testPathFunction("cleanPath", cleanPath, "", ""); + errors += testPathFunction("cleanPath", cleanPath, "/", "/"); + errors += testPathFunction("cleanPath", cleanPath, "///.///", "/"); + errors += testPathFunction("cleanPath", cleanPath, "/a/b/.././../c/./././////./d/..//", "/c"); + errors += testPathFunction("cleanPath", cleanPath, "a/b/.././../c/./././////./d/..//", "c"); + errors += testPathFunction("cleanPath", cleanPath, "..", ".."); + errors += testPathFunction("cleanPath", cleanPath, "../.././", "../.."); + errors += testPathFunction("cleanPath", cleanPath, "../a/b/..//", "../a"); + errors += testPathFunction("cleanPath", cleanPath, "a/b/.././../c/./././////./d/..//..", ""); + errors += testPathFunction("cleanPath", cleanPath, "/..", platform_error()); + errors += testPathFunction("cleanPath", cleanPath, "/a/b/../.././../", platform_error()); + + errors += testPathFunction("abspath", abspath, "", platform_error()); + errors += testPathFunction("abspath", abspath, "/", "/"); + errors += testPathFunction("abspath", abspath, "/dev", "/dev"); + errors += testPathFunction("abspath", abspath, "/dev/", "/dev"); + errors += testPathFunction("abspath", abspath, "/dev/..", "/"); + errors += testPathFunction("abspath", abspath, "/dev/../", "/"); + errors += testPathFunction("abspath", abspath, "/dev/../", "/"); + errors += testPathFunction("abspath", abspath, "/dev/..///", "/"); + + errors += testPathFunction("abspath", abspath, "blahfoo", joinPath(platform::getWorkingDirectory(), "blahfoo")); + errors += testPathFunction("abspath", abspath, "blahfoo/bar", joinPath(platform::getWorkingDirectory(), "blahfoo/bar")); + errors += testPathFunction("abspath", abspath, "blahfoo/bar/", joinPath(platform::getWorkingDirectory(), "blahfoo/bar")); + errors += testPathFunction("abspath", abspath, "blahfoo/bar/..//", joinPath(platform::getWorkingDirectory(), "blahfoo")); + errors += testPathFunction("abspath", abspath, "blahfoo/bar/..//../", platform::getWorkingDirectory()); + + errors += testPathFunction("abspath", abspath, "/blahfoo", "/blahfoo"); + errors += testPathFunction("abspath", abspath, "/blahfoo/bar", "/blahfoo/bar"); + errors += testPathFunction("abspath", abspath, "/blahfoo/bar/", "/blahfoo/bar"); + + errors += testPathFunction("parentDirectory", parentDirectory, "", platform_error()); + errors += testPathFunction("parentDirectory", parentDirectory, "/", platform_error()); + errors += testPathFunction("parentDirectory", parentDirectory, "//", platform_error()); + errors += testPathFunction("parentDirectory", parentDirectory, "/a", "/"); + errors += testPathFunction("parentDirectory", parentDirectory, "/a/", "/"); + errors += testPathFunction("parentDirectory", parentDirectory, "/a/b", "/a/"); + errors += testPathFunction("parentDirectory", parentDirectory, "/a/b/c/.././", "/a/"); + errors += testPathFunction("parentDirectory", parentDirectory, "a/b/c/.././", joinPath(platform::getWorkingDirectory(), "a/")); + + printf("%d errors.\n", errors); + ASSERT(errors == 0); + return Void(); +} +#endif diff --git a/flow/Platform.h b/flow/Platform.h index a76ca334d7..8f088057d6 100644 --- a/flow/Platform.h +++ b/flow/Platform.h @@ -320,13 +320,17 @@ void writeFile(std::string const& filename, std::string const& content); std::string joinPath( std::string const& directory, std::string const& filename ); -// Returns an absolute path canonicalized to use only CANONICAL_PATH_SEPARATOR +// Removes ./, ../, and duplicate separators from path, converts all separators to CANONICAL_PATH_SEPARATOR, +// but does NOT resolve symlinks and does not check the filesystem for the existence of any components +std::string cleanPath( std::string const& filename ); + +// Returns an cleaned path (see cleanPath()) that also has symbolic links resolved. std::string abspath( std::string const& filename ); // Returns the portion of the path following the last path separator (e.g. the filename or directory name) std::string basename( std::string const& filename ); -// Returns the parent directory of the given file or directory +// Returns the parent directory of the given file or directory with a single trailing separator std::string parentDirectory( std::string const& filename ); // Returns the home directory of the current user From 98a295a8038df11a7e57bf3bf327845a1015298f Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Sat, 16 Mar 2019 00:07:38 -0700 Subject: [PATCH 04/12] Changed parentDirectory() trace events back to SevError outside of simulation, did the same for cleanPath(). --- flow/Platform.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/flow/Platform.cpp b/flow/Platform.cpp index a2bef2d981..ac5487eb8e 100644 --- a/flow/Platform.cpp +++ b/flow/Platform.cpp @@ -1817,7 +1817,7 @@ std::string cleanPath(std::string const &path) { continue; } if(absolute) { - TraceEvent(SevWarnAlways, "CleanPathError").detail("Path", path); + TraceEvent(g_network->isSimulated() ? SevWarnAlways : SevError, "CleanPathError").detail("Path", path); throw platform_error(); } } @@ -1897,7 +1897,7 @@ std::string parentDirectory( std::string const& filename ) { || abs == ".." || (abs.size() > 2 && StringRef(abs).endsWith(dotdot) && abs[abs.size() - 3] == CANONICAL_PATH_SEPARATOR) ) { - TraceEvent(SevWarnAlways, "GetParentDirectoryOfFile") + TraceEvent(g_network->isSimulated() ? SevWarnAlways : SevError, "GetParentDirectoryOfFile") .detail("File", filename) .GetLastError(); throw platform_error(); @@ -2910,9 +2910,9 @@ int testPathFunction(const char *name, std::function f try { result = fun(a); } catch(Error &e) { result = e; } bool r = result.isError() == b.isError() && (b.isError() || b.get() == result.get()) && (!b.isError() || b.getError().code() == result.getError().code()); - printf("%s('%s') -> %s", name, a.c_str(), result.isError() ? result.getError().what() : format("'%s'", result.get().c_str()).c_str()); + printf("%s: %s('%s') -> %s", r ? "PASS" : "FAIL", name, a.c_str(), result.isError() ? result.getError().what() : format("'%s'", result.get().c_str()).c_str()); if(!r) { - printf(" ERROR expected %s", b.isError() ? b.getError().what() : format("'%s'", b.get().c_str()).c_str()); + printf(" *ERROR* expected %s", b.isError() ? b.getError().what() : format("'%s'", b.get().c_str()).c_str()); } printf("\n"); return r ? 0 : 1; @@ -2922,8 +2922,6 @@ int testPathFunction(const char *name, std::function f TEST_CASE("/flow/Platform/directoryOps") { int errors = 0; - errors += testPathFunction("cleanPath", cleanPath, "", ""); - errors += testPathFunction("cleanPath", cleanPath, "", ""); errors += testPathFunction("cleanPath", cleanPath, "/", "/"); errors += testPathFunction("cleanPath", cleanPath, "///.///", "/"); From 9a4fc25de56cda47dc2616a6c9e2a464dfb1ad67 Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Sun, 17 Mar 2019 15:58:04 -0700 Subject: [PATCH 05/12] ParentDirectory() in Platform,h no longer uses absPath() so it does not resolve symbolic links, making it suitable for use in fdbmonitor. CleanPath() will return "." instead of empty string to mean a relative path that refers to the current directory. Updated tests, and added new tests for ".". --- flow/Platform.cpp | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/flow/Platform.cpp b/flow/Platform.cpp index cc8c0405bf..8559f2782c 100644 --- a/flow/Platform.cpp +++ b/flow/Platform.cpp @@ -1801,7 +1801,6 @@ StringRef separator(&separatorChar, 1); StringRef dotdot = LiteralStringRef(".."); std::string cleanPath(std::string const &path) { - std::vector parts; std::vector finalParts; bool absolute = !path.empty() && path[0] == CANONICAL_PATH_SEPARATOR; @@ -1837,7 +1836,7 @@ std::string cleanPath(std::string const &path) { result.append((const char *)finalParts[i].begin(), finalParts[i].size()); } - return result; + return result.empty() ? "." : result; } std::string abspath( std::string const& filename ) { @@ -1888,23 +1887,22 @@ std::string basename( std::string const& filename ) { return abs.substr(sep+1); } -std::string parentDirectory( std::string const& filename ) { - std::string abs = abspath(filename); +std::string parentDirectory( std::string const& path ) { + std::string c = cleanPath(joinPath(path, "..")); // Can't take the parent of empty, root, or anything ending in an unresolved .. - if(abs.empty() - || (abs.size() == 1 && abs.front() == CANONICAL_PATH_SEPARATOR) - || abs == ".." - || (abs.size() > 2 && StringRef(abs).endsWith(dotdot) && abs[abs.size() - 3] == CANONICAL_PATH_SEPARATOR) - ) { + if(StringRef(c).endsWith(dotdot)) { TraceEvent(g_network->isSimulated() ? SevWarnAlways : SevError, "GetParentDirectoryOfFile") - .detail("File", filename) + .detail("Path", path) .GetLastError(); throw platform_error(); } - abs.resize(abs.find_last_of( CANONICAL_PATH_SEPARATOR ) + 1); - return abs; + if(c.back() != CANONICAL_PATH_SEPARATOR) { + c.append(1, CANONICAL_PATH_SEPARATOR); + } + + return c; } std::string getUserHomeDirectory() { @@ -2922,7 +2920,7 @@ int testPathFunction(const char *name, std::function f TEST_CASE("/flow/Platform/directoryOps") { int errors = 0; - errors += testPathFunction("cleanPath", cleanPath, "", ""); + errors += testPathFunction("cleanPath", cleanPath, "", "."); errors += testPathFunction("cleanPath", cleanPath, "/", "/"); errors += testPathFunction("cleanPath", cleanPath, "///.///", "/"); errors += testPathFunction("cleanPath", cleanPath, "/a/b/.././../c/./././////./d/..//", "/c"); @@ -2930,10 +2928,12 @@ TEST_CASE("/flow/Platform/directoryOps") { errors += testPathFunction("cleanPath", cleanPath, "..", ".."); errors += testPathFunction("cleanPath", cleanPath, "../.././", "../.."); errors += testPathFunction("cleanPath", cleanPath, "../a/b/..//", "../a"); - errors += testPathFunction("cleanPath", cleanPath, "a/b/.././../c/./././////./d/..//..", ""); + errors += testPathFunction("cleanPath", cleanPath, "a/b/.././../c/./././////./d/..//..", "."); errors += testPathFunction("cleanPath", cleanPath, "/..", platform_error()); errors += testPathFunction("cleanPath", cleanPath, "/a/b/../.././../", platform_error()); + errors += testPathFunction("cleanPath", cleanPath, ".", "."); + errors += testPathFunction("abspath", abspath, ".", platform::getWorkingDirectory()); errors += testPathFunction("abspath", abspath, "", platform_error()); errors += testPathFunction("abspath", abspath, "/", "/"); errors += testPathFunction("abspath", abspath, "/dev", "/dev"); @@ -2960,7 +2960,7 @@ TEST_CASE("/flow/Platform/directoryOps") { errors += testPathFunction("parentDirectory", parentDirectory, "/a/", "/"); errors += testPathFunction("parentDirectory", parentDirectory, "/a/b", "/a/"); errors += testPathFunction("parentDirectory", parentDirectory, "/a/b/c/.././", "/a/"); - errors += testPathFunction("parentDirectory", parentDirectory, "a/b/c/.././", joinPath(platform::getWorkingDirectory(), "a/")); + errors += testPathFunction("parentDirectory", parentDirectory, "a/b/c/.././", "a/"); printf("%d errors.\n", errors); ASSERT(errors == 0); From bec6f0a37a2305596e20eefc499d32b7e9377d48 Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Wed, 20 Mar 2019 22:52:47 -0700 Subject: [PATCH 06/12] Added flags to absPath() and parentDirectory() for whether or not symbolic links should be resolved when possible and whether the final resolved path must actually exist on the filesystem. Updated tests, added more cases. --- flow/Platform.cpp | 269 ++++++++++++++++++++++++++++++++-------------- flow/Platform.h | 26 +++-- 2 files changed, 208 insertions(+), 87 deletions(-) diff --git a/flow/Platform.cpp b/flow/Platform.cpp index 8559f2782c..b6e4a27aa6 100644 --- a/flow/Platform.cpp +++ b/flow/Platform.cpp @@ -1816,8 +1816,7 @@ std::string cleanPath(std::string const &path) { continue; } if(absolute) { - TraceEvent(g_network->isSimulated() ? SevWarnAlways : SevError, "CleanPathError").detail("Path", path); - throw platform_error(); + continue; } } finalParts.push_back(part); @@ -1839,19 +1838,78 @@ std::string cleanPath(std::string const &path) { return result.empty() ? "." : result; } -std::string abspath( std::string const& filename ) { - if(filename.empty()) { - throw platform_error(); +// Removes the last component from a path string (if possible) and returns the result with one trailing separator. +// If there is only one path component, the result will be "" for relative paths and "/" for absolute paths. +// Note that this is NOT the same as getting the parant of path, as the final component could be ".." +// or "." and it would still be simply removed. +// ALL of the following inputs will yield the result "/a/" +// /a/b +// /a/b/ +// /a/.. +// /a/../ +// /a/. +// /a/./ +// /a//..// +std::string popPath(const std::string &path) { + int i = path.size() - 1; + // Skip over any trailing separators + while(i >= 0 && path[i] == CANONICAL_PATH_SEPARATOR) { + --i; + } + // Skip over non separators + while(i >= 0 && path[i] != CANONICAL_PATH_SEPARATOR) { + --i; + } + // Skip over trailing separators again + bool foundSeparator = false; + while(i >= 0 && path[i] == CANONICAL_PATH_SEPARATOR) { + --i; + foundSeparator = true; + } + + if(foundSeparator) { + ++i; + } + else { + // If absolute then we popped off the only path component so return "/" + if(!path.empty() && path.front() == CANONICAL_PATH_SEPARATOR) { + return "/"; + } + } + return path.substr(0, i + 1); +} + +std::string abspath( std::string const& path, bool resolveLinks, bool mustExist ) { + if(path.empty()) { + Error e = platform_error(); + Severity sev = e.code() == error_code_io_error ? SevError : SevWarnAlways; + TraceEvent(sev, "AbsolutePathError").detail("Path", path).error(e); + throw e; } // Returns an absolute path canonicalized to use only CANONICAL_PATH_SEPARATOR INJECT_FAULT( platform_error, "abspath" ); + if(!resolveLinks) { + // TODO: Not resolving symbolic links does not yet behave well on Windows because of drive letters + // and network names, so it's not currently allowed here (but it is allowed in fdbmonitor which is unix-only) + ASSERT(g_network->isSimulated()); + std::string clean = cleanPath(joinPath(platform::getWorkingDirectory(), path)); + if(mustExist && !fileExists(clean)) { + Error e = systemErrorCodeToError(); + Severity sev = e.code() == error_code_io_error ? SevError : SevWarnAlways; + TraceEvent(sev, "AbsolutePathError").detail("Path", path).GetLastError().error(e); + throw e; + } + return clean; + } + #ifdef _WIN32 char nameBuffer[MAX_PATH]; - if (!GetFullPathName(filename.c_str(), MAX_PATH, nameBuffer, NULL)) { + if (!GetFullPathName(path.c_str(), MAX_PATH, nameBuffer, NULL) || (mustExist && !fileExists(nameBuffer)) { Error e = systemErrorCodeToError(); - TraceEvent(SevError, "AbsolutePathError").detail("Filename", filename).GetLastError().error(e); + Severity sev = e.code() == error_code_io_error ? SevError : SevWarnAlways; + TraceEvent(sev, "AbsolutePathError").detail("Path", path).GetLastError().error(e); throw e; } // Not totally obvious from the help whether GetFullPathName canonicalizes slashes, so let's do it... @@ -1860,18 +1918,24 @@ std::string abspath( std::string const& filename ) { *x = CANONICAL_PATH_SEPARATOR; return nameBuffer; #elif (defined(__linux__) || defined(__APPLE__)) + char result[PATH_MAX]; - auto r = realpath( filename.c_str(), result ); - if (!r) { - if (errno == ENOENT) { - bool absolute = !filename.empty() && filename.front() == CANONICAL_PATH_SEPARATOR; - if(absolute) { - return cleanPath(filename); + // Must resolve links, so first try realpath on the whole thing + const char *r = realpath( path.c_str(), result ); + if(r == nullptr) { + // If the error was ENOENT and the path doesn't have to exist, + // try to resolve symlinks in progressively shorter prefixes of the path + if(errno == ENOENT && !mustExist) { + std::string prefix = popPath(path); + std::string suffix = path.substr(prefix.size()); + if(prefix.empty()) { + prefix = "."; } - return cleanPath(joinPath(platform::getWorkingDirectory(), filename)); + return cleanPath(joinPath(abspath(prefix, true, false), suffix)); } Error e = systemErrorCodeToError(); - TraceEvent(SevError, "AbsolutePathError").detail("Filename", filename).GetLastError().error(e); + Severity sev = e.code() == error_code_io_error ? SevError : SevWarnAlways; + TraceEvent(sev, "AbsolutePathError").detail("Path", path).GetLastError().error(e); throw e; } return std::string(r); @@ -1880,6 +1944,19 @@ std::string abspath( std::string const& filename ) { #endif } +std::string parentDirectory( std::string const& path, bool resolveLinks, bool mustExist ) { + auto abs = abspath(path, resolveLinks, mustExist); + size_t sep = abs.find_last_of( CANONICAL_PATH_SEPARATOR ); + if (sep == std::string::npos) { + Error e = platform_error(); + TraceEvent(SevWarnAlways, "GetParentDirectory") + .detail("File", path) + .error(e); + throw e; + } + return abs.substr(0, sep + 1); +} + std::string basename( std::string const& filename ) { auto abs = abspath(filename); size_t sep = abs.find_last_of( CANONICAL_PATH_SEPARATOR ); @@ -1887,24 +1964,6 @@ std::string basename( std::string const& filename ) { return abs.substr(sep+1); } -std::string parentDirectory( std::string const& path ) { - std::string c = cleanPath(joinPath(path, "..")); - - // Can't take the parent of empty, root, or anything ending in an unresolved .. - if(StringRef(c).endsWith(dotdot)) { - TraceEvent(g_network->isSimulated() ? SevWarnAlways : SevError, "GetParentDirectoryOfFile") - .detail("Path", path) - .GetLastError(); - throw platform_error(); - } - - if(c.back() != CANONICAL_PATH_SEPARATOR) { - c.append(1, CANONICAL_PATH_SEPARATOR); - } - - return c; -} - std::string getUserHomeDirectory() { #if defined(__unixish__) const char* ret = getenv( "HOME" ); @@ -2889,20 +2948,6 @@ TEST_CASE("/flow/Platform/getMemoryInfo") { } #endif -int testAbsPath(std::string a, ErrorOr b) { - ErrorOr result; - try { result = abspath(a); } catch(Error &e) { result = e; } - - bool r = result.isError() == b.isError() && (b.isError() || b.get() == result.get()) && (!b.isError() || b.getError().code() == result.getError().code()); - - printf("abspath('%s') -> %s", a.c_str(), result.isError() ? result.getError().what() : format("'%s'", result.get().c_str()).c_str()); - if(!r) { - printf(" ERROR expected %s", b.isError() ? b.getError().what() : format("'%s'", b.get().c_str()).c_str()); - } - printf("\n"); - return r ? 0 : 1; -} - int testPathFunction(const char *name, std::function fun, std::string a, ErrorOr b) { ErrorOr result; try { result = fun(a); } catch(Error &e) { result = e; } @@ -2916,11 +2961,35 @@ int testPathFunction(const char *name, std::function f return r ? 0 : 1; } -#ifdef __unixish__ +int testPathFunction2(const char *name, std::function fun, std::string a, bool x, bool y, ErrorOr b) { + ErrorOr result; + try { result = fun(a, x, y); } catch(Error &e) { result = e; } + bool r = result.isError() == b.isError() && (b.isError() || b.get() == result.get()) && (!b.isError() || b.getError().code() == result.getError().code()); + + printf("%s: %s('%s', %d, %d) -> %s", r ? "PASS" : "FAIL", name, a.c_str(), x, y, result.isError() ? result.getError().what() : format("'%s'", result.get().c_str()).c_str()); + if(!r) { + printf(" *ERROR* expected %s", b.isError() ? b.getError().what() : format("'%s'", b.get().c_str()).c_str()); + } + printf("\n"); + return r ? 0 : 1; +} + TEST_CASE("/flow/Platform/directoryOps") { int errors = 0; - errors += testPathFunction("cleanPath", cleanPath, "", "."); + errors += testPathFunction("popPath", popPath, "a", ""); + errors += testPathFunction("popPath", popPath, "a/", ""); + errors += testPathFunction("popPath", popPath, "a///", ""); + errors += testPathFunction("popPath", popPath, "a///..", "a/"); + errors += testPathFunction("popPath", popPath, "a///../", "a/"); + errors += testPathFunction("popPath", popPath, "a///..//", "a/"); + errors += testPathFunction("popPath", popPath, "/", "/"); + errors += testPathFunction("popPath", popPath, "/a", "/"); + errors += testPathFunction("popPath", popPath, "/a/b", "/a/"); + errors += testPathFunction("popPath", popPath, "/a/b/", "/a/"); + errors += testPathFunction("popPath", popPath, "/a/b/..", "/a/b/"); + errors += testPathFunction("popPath", popPath, "/a/b///..//", "/a/b/"); + errors += testPathFunction("cleanPath", cleanPath, "/", "/"); errors += testPathFunction("cleanPath", cleanPath, "///.///", "/"); errors += testPathFunction("cleanPath", cleanPath, "/a/b/.././../c/./././////./d/..//", "/c"); @@ -2929,41 +2998,83 @@ TEST_CASE("/flow/Platform/directoryOps") { errors += testPathFunction("cleanPath", cleanPath, "../.././", "../.."); errors += testPathFunction("cleanPath", cleanPath, "../a/b/..//", "../a"); errors += testPathFunction("cleanPath", cleanPath, "a/b/.././../c/./././////./d/..//..", "."); - errors += testPathFunction("cleanPath", cleanPath, "/..", platform_error()); - errors += testPathFunction("cleanPath", cleanPath, "/a/b/../.././../", platform_error()); + errors += testPathFunction("cleanPath", cleanPath, "/..", "/"); + errors += testPathFunction("cleanPath", cleanPath, "/../foo/bar///", "/foo/bar"); + errors += testPathFunction("cleanPath", cleanPath, "/a/b/../.././../", "/"); errors += testPathFunction("cleanPath", cleanPath, ".", "."); - errors += testPathFunction("abspath", abspath, ".", platform::getWorkingDirectory()); - errors += testPathFunction("abspath", abspath, "", platform_error()); - errors += testPathFunction("abspath", abspath, "/", "/"); - errors += testPathFunction("abspath", abspath, "/dev", "/dev"); - errors += testPathFunction("abspath", abspath, "/dev/", "/dev"); - errors += testPathFunction("abspath", abspath, "/dev/..", "/"); - errors += testPathFunction("abspath", abspath, "/dev/../", "/"); - errors += testPathFunction("abspath", abspath, "/dev/../", "/"); - errors += testPathFunction("abspath", abspath, "/dev/..///", "/"); - - errors += testPathFunction("abspath", abspath, "blahfoo", joinPath(platform::getWorkingDirectory(), "blahfoo")); - errors += testPathFunction("abspath", abspath, "blahfoo/bar", joinPath(platform::getWorkingDirectory(), "blahfoo/bar")); - errors += testPathFunction("abspath", abspath, "blahfoo/bar/", joinPath(platform::getWorkingDirectory(), "blahfoo/bar")); - errors += testPathFunction("abspath", abspath, "blahfoo/bar/..//", joinPath(platform::getWorkingDirectory(), "blahfoo")); - errors += testPathFunction("abspath", abspath, "blahfoo/bar/..//../", platform::getWorkingDirectory()); + // Creating this directory in backups avoids some sanity checks + platform::createDirectory("simfdb/backups/one/two/three"); + std::string cwd = platform::getWorkingDirectory(); - errors += testPathFunction("abspath", abspath, "/blahfoo", "/blahfoo"); - errors += testPathFunction("abspath", abspath, "/blahfoo/bar", "/blahfoo/bar"); - errors += testPathFunction("abspath", abspath, "/blahfoo/bar/", "/blahfoo/bar"); - - errors += testPathFunction("parentDirectory", parentDirectory, "", platform_error()); - errors += testPathFunction("parentDirectory", parentDirectory, "/", platform_error()); - errors += testPathFunction("parentDirectory", parentDirectory, "//", platform_error()); - errors += testPathFunction("parentDirectory", parentDirectory, "/a", "/"); - errors += testPathFunction("parentDirectory", parentDirectory, "/a/", "/"); - errors += testPathFunction("parentDirectory", parentDirectory, "/a/b", "/a/"); - errors += testPathFunction("parentDirectory", parentDirectory, "/a/b/c/.././", "/a/"); - errors += testPathFunction("parentDirectory", parentDirectory, "a/b/c/.././", "a/"); +#ifndef _WIN32 + // Create some symlinks and test resolution (or non-resolution) of them + ASSERT(symlink("one/two", "simfdb/backups/four") == 0); + ASSERT(symlink("../backups/four", "simfdb/backups/five") == 0); + + errors += testPathFunction2("abspath", abspath, "simfdb/backups/four/../two", true, true, joinPath(cwd, "simfdb/backups/one/two")); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../two", true, true, joinPath(cwd, "simfdb/backups/one/two")); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../two", true, false, joinPath(cwd, "simfdb/backups/one/two")); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../three", true, true, platform_error()); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../three", true, false, joinPath(cwd, "simfdb/backups/one/three")); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../three/../four", true, false, joinPath(cwd, "simfdb/backups/one/four")); + + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/four/../two", true, true, joinPath(cwd, "simfdb/backups/one/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../two", true, true, joinPath(cwd, "simfdb/backups/one/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../two", true, false, joinPath(cwd, "simfdb/backups/one/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../three", true, true, platform_error()); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../three", true, false, joinPath(cwd, "simfdb/backups/one/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../three/../four", true, false, joinPath(cwd, "simfdb/backups/one/")); +#endif + + errors += testPathFunction2("abspath", abspath, "/", true, false, "/"); + errors += testPathFunction2("abspath", abspath, "", true, false, platform_error()); + errors += testPathFunction2("abspath", abspath, ".", true, false, cwd); + errors += testPathFunction2("abspath", abspath, "/a", true, false, "/a"); + errors += testPathFunction2("abspath", abspath, "one/two/three/four", false, true, platform_error()); + errors += testPathFunction2("abspath", abspath, "one/two/three/four", false, false, joinPath(cwd, "one/two/three/four")); + errors += testPathFunction2("abspath", abspath, "one/two/three/./four", false, false, joinPath(cwd, "one/two/three/four")); + errors += testPathFunction2("abspath", abspath, "one/two/three/./four", false, false, joinPath(cwd, "one/two/three/four")); + errors += testPathFunction2("abspath", abspath, "one/two/three/./four/..", false, false, joinPath(cwd, "one/two/three")); + errors += testPathFunction2("abspath", abspath, "one/./two/../three/./four", false, false, joinPath(cwd, "one/three/four")); + errors += testPathFunction2("abspath", abspath, "one/./two/../three/./four", false, true, platform_error()); + errors += testPathFunction2("abspath", abspath, "one/two/three/./four", false, true, platform_error()); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/one/two/three", false, true, joinPath(cwd, "simfdb/backups/one/two/three")); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/one/two/threefoo", false, true, platform_error()); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/four/../two", false, false, joinPath(cwd, "simfdb/backups/two")); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/four/../two", false, true, platform_error()); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../two", false, true, platform_error()); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../two", false, false, joinPath(cwd, "simfdb/backups/two")); + errors += testPathFunction2("abspath", abspath, "foo/./../foo2/./bar//", false, false, joinPath(cwd, "foo2/bar")); + errors += testPathFunction2("abspath", abspath, "foo/./../foo2/./bar//", false, true, platform_error()); + errors += testPathFunction2("abspath", abspath, "foo/./../foo2/./bar//", true, false, joinPath(cwd, "foo2/bar")); + errors += testPathFunction2("abspath", abspath, "foo/./../foo2/./bar//", true, true, platform_error()); + + errors += testPathFunction2("parentDirectory", parentDirectory, "", true, false, platform_error()); + errors += testPathFunction2("parentDirectory", parentDirectory, "/", true, false, "/"); + errors += testPathFunction2("parentDirectory", parentDirectory, "/a", true, false, "/"); + errors += testPathFunction2("parentDirectory", parentDirectory, ".", false, false, cleanPath(joinPath(cwd, "..")) + "/"); + errors += testPathFunction2("parentDirectory", parentDirectory, "./foo", false, false, cleanPath(cwd) + "/"); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/two/three/four", false, true, platform_error()); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/two/three/four", false, false, joinPath(cwd, "one/two/three/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/two/three/./four", false, false, joinPath(cwd, "one/two/three/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/two/three/./four/..", false, false, joinPath(cwd, "one/two/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/./two/../three/./four", false, false, joinPath(cwd, "one/three/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/./two/../three/./four", false, true, platform_error()); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/two/three/./four", false, true, platform_error()); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/one/two/three", false, true, joinPath(cwd, "simfdb/backups/one/two/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/one/two/threefoo", false, true, platform_error()); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/four/../two", false, false, joinPath(cwd, "simfdb/backups/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/four/../two", false, true, platform_error()); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../two", false, true, platform_error()); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../two", false, false, joinPath(cwd, "simfdb/backups/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "foo/./../foo2/./bar//", false, false, joinPath(cwd, "foo2/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "foo/./../foo2/./bar//", false, true, platform_error()); + errors += testPathFunction2("parentDirectory", parentDirectory, "foo/./../foo2/./bar//", true, false, joinPath(cwd, "foo2/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "foo/./../foo2/./bar//", true, true, platform_error()); printf("%d errors.\n", errors); + ASSERT(errors == 0); return Void(); } -#endif diff --git a/flow/Platform.h b/flow/Platform.h index 09926ae46e..2c34900cbd 100644 --- a/flow/Platform.h +++ b/flow/Platform.h @@ -320,19 +320,29 @@ void writeFile(std::string const& filename, std::string const& content); std::string joinPath( std::string const& directory, std::string const& filename ); -// Removes ./, ../, and duplicate separators from path, converts all separators to CANONICAL_PATH_SEPARATOR, -// but does NOT resolve symlinks and does not check the filesystem for the existence of any components -std::string cleanPath( std::string const& filename ); +// cleanPath() does a 'logical' resolution of the given path string to a canonical form *without* +// following symbolic links or verifying the existence of any path components, removing redundant +// "." references and duplicate separators, and resolving any ".." references. +// Relative paths remain relative and are NOT rebased on the current working directory. +std::string cleanPath( std::string const& path ); -// Returns an cleaned path (see cleanPath()) that also has symbolic links resolved. -std::string abspath( std::string const& filename ); +// abspath() resolves the given path to a canonical form. +// If path is relative, the result will be based on the current working directory. +// If resolveLinks is true then symbolic links will be expanded BEFORE resolving '..' references. +// An empty path or a non-existent path when mustExist is true will result in a platform_error() exception. +// Upon success, all '..' references will be resolved with the assumption that non-existent components +// are NOT symbolic links. +std::string abspath( std::string const& path, bool resolveLinks = true, bool mustExist = false ); + +// parentDirectory() returns the parent directory of the given file or directory in a canonical form, +// with a single trailing path separator. +// It uses absPath() with the same bool options to initially obtain a canonical form, and upon success +// removes the final path component, if present. +std::string parentDirectory( std::string const& path, bool resolveLinks = true, bool mustExist = false); // Returns the portion of the path following the last path separator (e.g. the filename or directory name) std::string basename( std::string const& filename ); -// Returns the parent directory of the given file or directory with a single trailing separator -std::string parentDirectory( std::string const& filename ); - // Returns the home directory of the current user std::string getUserHomeDirectory(); From 644a88e8b42207ce913679231fc6ebffc8de4ec4 Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Wed, 20 Mar 2019 23:18:30 -0700 Subject: [PATCH 07/12] Updated fdbmonitor to use modified/minimal implementations of new abspath() and parentDirectory() implementations from flow. --- fdbmonitor/fdbmonitor.cpp | 127 ++++++++++++++++++++++++++++++-------- 1 file changed, 100 insertions(+), 27 deletions(-) diff --git a/fdbmonitor/fdbmonitor.cpp b/fdbmonitor/fdbmonitor.cpp index c4852b7daa..818bc500ab 100644 --- a/fdbmonitor/fdbmonitor.cpp +++ b/fdbmonitor/fdbmonitor.cpp @@ -223,40 +223,113 @@ std::string joinPath(std::string const& directory, std::string const& filename) return d + CANONICAL_PATH_SEPARATOR + f; } -std::string abspath(std::string const& filename) { - // Returns an absolute path canonicalized to use only CANONICAL_PATH_SEPARATOR - char result[PATH_MAX]; - auto r = realpath( filename.c_str(), result ); - if (!r) { - if (errno == ENOENT) { - int sep = filename.find_last_of( CANONICAL_PATH_SEPARATOR ); - if (sep != std::string::npos) { - return joinPath( abspath( filename.substr(0, sep) ), filename.substr(sep) ); +std::string cleanPath(std::string const &path) { + std::vector finalParts; + bool absolute = !path.empty() && path[0] == CANONICAL_PATH_SEPARATOR; + + int i = 0; + while(i < path.size()) { + int sep = path.find((char)CANONICAL_PATH_SEPARATOR, i); + if(sep == path.npos) { + sep = path.size(); + } + std::string part = path.substr(i + 1, sep - i); + if(part.size() == 0 || (part.size() == 1 && part[0] == '.')) + continue; + if(part == "..") { + if(!finalParts.empty() && finalParts.back() != "..") { + finalParts.pop_back(); + continue; } - else if (filename.find("~") == std::string::npos) { - return joinPath( abspath( "." ), filename ); + if(absolute) { + continue; } } + finalParts.push_back(part); + } - log_err("realpath", errno, "Unable to get real path for %s", filename.c_str()); + std::string result; + result.reserve(PATH_MAX); + if(absolute) { + result.append(1, CANONICAL_PATH_SEPARATOR); + } + + for(int i = 0; i < finalParts.size(); ++i) { + if(i != 0) { + result.append(1, CANONICAL_PATH_SEPARATOR); + } + result.append(finalParts[i]); + } + + return result.empty() ? "." : result; +} + +// Removes the last component from a path string (if possible) and returns the result with one trailing separator. +std::string popPath(const std::string &path) { + int i = path.size() - 1; + // Skip over any trailing separators + while(i >= 0 && path[i] == CANONICAL_PATH_SEPARATOR) { + --i; + } + // Skip over non separators + while(i >= 0 && path[i] != CANONICAL_PATH_SEPARATOR) { + --i; + } + // Skip over trailing separators again + bool foundSeparator = false; + while(i >= 0 && path[i] == CANONICAL_PATH_SEPARATOR) { + --i; + foundSeparator = true; + } + + if(foundSeparator) { + ++i; + } + else { + // If absolute then we popped off the only path component so return "/" + if(!path.empty() && path.front() == CANONICAL_PATH_SEPARATOR) { + return "/"; + } + } + return path.substr(0, i + 1); +} + +std::string abspath( std::string const& path, bool resolveLinks = true) { + if(path.empty()) { + return ""; + } + + if(!resolveLinks) { + std::string clean = cleanPath(joinPath(abspath(".", true), path)); + return clean; + } + + char result[PATH_MAX]; + // Must resolve links, so first try realpath on the whole thing + const char *r = realpath( path.c_str(), result ); + if(r == nullptr) { + // If the error was ENOENT and the path doesn't have to exist, + // try to resolve symlinks in progressively shorter prefixes of the path + if(errno == ENOENT && path != ".") { + std::string prefix = popPath(path); + std::string suffix = path.substr(prefix.size()); + if(prefix.empty()) { + prefix = "."; + } + return cleanPath(joinPath(abspath(prefix, true), suffix)); + } return ""; } return std::string(r); } -// Get the parent directory of a filename *without* resolving symbolic links so that fdbmonitor can support -// symbolic links updates as a way of atomitically changing the configuration file -std::string parentDirectory(std::string filename) { - size_t sep = filename.find_last_of( CANONICAL_PATH_SEPARATOR ); +std::string parentDirectory( std::string const& path, bool resolveLinks = true) { + auto abs = abspath(path, resolveLinks); + size_t sep = abs.find_last_of( CANONICAL_PATH_SEPARATOR ); if (sep == std::string::npos) { return ""; } - - while(filename.size() && (filename.back() == '/' || filename.back() == CANONICAL_PATH_SEPARATOR)) { - filename.resize(filename.size() - 1); - } - - return filename.substr(0, sep+1); + return abs.substr(0, sep + 1); } int mkdir(std::string const& directory) { @@ -853,7 +926,7 @@ void watch_conf_dir( int kq, int* confd_fd, std::string confdir ) { /* Find the nearest existing ancestor */ while( (*confd_fd = open( confdir.c_str(), O_EVTONLY )) < 0 && errno == ENOENT ) { child = confdir; - confdir = parentDirectory(confdir); + confdir = parentDirectory(confdir, false); } if ( *confd_fd >= 0 ) { @@ -945,7 +1018,7 @@ std::unordered_map> set_watches(std::string } } - std::string parent = parentDirectory(subpath); + std::string parent = parentDirectory(subpath, false); /* Watch the parent directory of the current path for changes */ int wd = inotify_add_watch(ifd, parent.c_str(), IN_CREATE | IN_MOVED_TO); @@ -1054,7 +1127,7 @@ int main(int argc, char** argv) { std::string confpath = p; // Will always succeed given an absolute path - std::string confdir = parentDirectory(confpath); + std::string confdir = parentDirectory(confpath, false); std::string conffile = confpath.substr(confdir.size()); #ifdef __linux__ @@ -1119,7 +1192,7 @@ int main(int argc, char** argv) { } /* open and lock our lockfile for mutual exclusion */ - std::string lockfileDir = parentDirectory(abspath(lockfile)); + std::string lockfileDir = parentDirectory(lockfile, true); if(lockfileDir.size() == 0) { log_msg(SevError, "Unable to determine parent directory of lockfile %s\n", lockfile.c_str()); exit(1); @@ -1446,7 +1519,7 @@ int main(int argc, char** argv) { confpath = redone_confpath; // Will always succeed given an absolute path - confdir = parentDirectory(confpath); + confdir = parentDirectory(confpath, false); conffile = confpath.substr(confdir.size()); // Remove all the old watches From c6d96498ca18bfa74f98ea4fb6c1a6469daeac9c Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Thu, 21 Mar 2019 16:56:36 -0700 Subject: [PATCH 08/12] parentDirectory() is now based on popPath(). Bug fix, abspath() would prepend current working directory even when not resolving symlinks. Added more unit tests. Ported path operation unit tests to fdbmonitor() since the path manipulation function implementations are significantly different. Clarified some comments. The flow project version of abspath() does not allow resolveLinks to be false, for now, because behavior of this on Windows is not well thought out or tested. --- fdbmonitor/fdbmonitor.cpp | 106 ++++++++++++++++++++++++++++++++++---- flow/Platform.cpp | 32 ++++++------ flow/Platform.h | 5 +- 3 files changed, 116 insertions(+), 27 deletions(-) diff --git a/fdbmonitor/fdbmonitor.cpp b/fdbmonitor/fdbmonitor.cpp index 818bc500ab..236d06aa00 100644 --- a/fdbmonitor/fdbmonitor.cpp +++ b/fdbmonitor/fdbmonitor.cpp @@ -233,7 +233,8 @@ std::string cleanPath(std::string const &path) { if(sep == path.npos) { sep = path.size(); } - std::string part = path.substr(i + 1, sep - i); + std::string part = path.substr(i, sep - i); + i = sep + 1; if(part.size() == 0 || (part.size() == 1 && part[0] == '.')) continue; if(part == "..") { @@ -300,8 +301,8 @@ std::string abspath( std::string const& path, bool resolveLinks = true) { } if(!resolveLinks) { - std::string clean = cleanPath(joinPath(abspath(".", true), path)); - return clean; + bool absolute = !path.empty() && path[0] == CANONICAL_PATH_SEPARATOR; + return cleanPath(absolute ? path : joinPath(abspath(".", true), path)); } char result[PATH_MAX]; @@ -318,18 +319,14 @@ std::string abspath( std::string const& path, bool resolveLinks = true) { } return cleanPath(joinPath(abspath(prefix, true), suffix)); } + log_err("realpath", errno, "Unable to get real path for %s", path.c_str()); return ""; } return std::string(r); } std::string parentDirectory( std::string const& path, bool resolveLinks = true) { - auto abs = abspath(path, resolveLinks); - size_t sep = abs.find_last_of( CANONICAL_PATH_SEPARATOR ); - if (sep == std::string::npos) { - return ""; - } - return abs.substr(0, sep + 1); + return popPath(abspath(path, resolveLinks)); } int mkdir(std::string const& directory) { @@ -1066,6 +1063,97 @@ std::unordered_map> set_watches(std::string } #endif +int testPathFunction(const char *name, std::function fun, std::string a, std::string b) { + std::string o = fun(a); + bool r = b == o; + printf("%s: %s(%s) = %s expected %s\n", r ? "PASS" : "FAIL", name, a.c_str(), o.c_str(), b.c_str()); + return r ? 0 : 1; +} + +int testPathFunction2(const char *name, std::function fun, std::string a, bool x, std::string b) { + std::string o = fun(a, x); + bool r = b == o; + printf("%s: %s(%s, %d) => %s expected %s\n", r ? "PASS" : "FAIL", name, a.c_str(), x, o.c_str(), b.c_str()); + return r ? 0 : 1; +} + +void testPathOps() { + int errors = 0; + + errors += testPathFunction("popPath", popPath, "a", ""); + errors += testPathFunction("popPath", popPath, "a/", ""); + errors += testPathFunction("popPath", popPath, "a///", ""); + errors += testPathFunction("popPath", popPath, "a///..", "a/"); + errors += testPathFunction("popPath", popPath, "a///../", "a/"); + errors += testPathFunction("popPath", popPath, "a///..//", "a/"); + errors += testPathFunction("popPath", popPath, "/", "/"); + errors += testPathFunction("popPath", popPath, "/a", "/"); + errors += testPathFunction("popPath", popPath, "/a/b", "/a/"); + errors += testPathFunction("popPath", popPath, "/a/b/", "/a/"); + errors += testPathFunction("popPath", popPath, "/a/b/..", "/a/b/"); + + errors += testPathFunction("cleanPath", cleanPath, "/", "/"); + errors += testPathFunction("cleanPath", cleanPath, "..", ".."); + errors += testPathFunction("cleanPath", cleanPath, "../.././", "../.."); + errors += testPathFunction("cleanPath", cleanPath, "///.///", "/"); + errors += testPathFunction("cleanPath", cleanPath, "/a/b/.././../c/./././////./d/..//", "/c"); + errors += testPathFunction("cleanPath", cleanPath, "a/b/.././../c/./././////./d/..//", "c"); + errors += testPathFunction("cleanPath", cleanPath, "a/b/.././../c/./././////./d/..//..", "."); + errors += testPathFunction("cleanPath", cleanPath, "a/b/.././../c/./././////./d/..//../..", ".."); + errors += testPathFunction("cleanPath", cleanPath, "../a/b/..//", "../a"); + errors += testPathFunction("cleanPath", cleanPath, "/..", "/"); + errors += testPathFunction("cleanPath", cleanPath, "/../foo/bar///", "/foo/bar"); + errors += testPathFunction("cleanPath", cleanPath, "/a/b/../.././../", "/"); + errors += testPathFunction("cleanPath", cleanPath, ".", "."); + + mkdir("simfdb/backups/one/two/three"); + std::string cwd = abspath(".", true); + + // Create some symlinks and test resolution (or non-resolution) of them + symlink("one/two", "simfdb/backups/four"); + symlink("../backups/four", "simfdb/backups/five"); + + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../two", true, joinPath(cwd, "simfdb/backups/one/two")); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../three", true, joinPath(cwd, "simfdb/backups/one/three")); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../three/../four", true, joinPath(cwd, "simfdb/backups/one/four")); + + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../two", true, joinPath(cwd, "simfdb/backups/one/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../three", true, joinPath(cwd, "simfdb/backups/one/")); + + errors += testPathFunction2("abspath", abspath, "/", false, "/"); + errors += testPathFunction2("abspath", abspath, "/foo//bar//baz/.././", false, "/foo/bar"); + errors += testPathFunction2("abspath", abspath, "/", true, "/"); + errors += testPathFunction2("abspath", abspath, "", true, ""); + errors += testPathFunction2("abspath", abspath, ".", true, cwd); + errors += testPathFunction2("abspath", abspath, "/a", true, "/a"); + errors += testPathFunction2("abspath", abspath, "one/two/three/four", false, joinPath(cwd, "one/two/three/four")); + errors += testPathFunction2("abspath", abspath, "one/two/three/./four", false, joinPath(cwd, "one/two/three/four")); + errors += testPathFunction2("abspath", abspath, "one/two/three/./four/..", false, joinPath(cwd, "one/two/three")); + errors += testPathFunction2("abspath", abspath, "one/./two/../three/./four", false, joinPath(cwd, "one/three/four")); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/four/../two", false, joinPath(cwd, "simfdb/backups/two")); + errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../two", false, joinPath(cwd, "simfdb/backups/two")); + errors += testPathFunction2("abspath", abspath, "foo/./../foo2/./bar//", false, joinPath(cwd, "foo2/bar")); + errors += testPathFunction2("abspath", abspath, "foo/./../foo2/./bar//", true, joinPath(cwd, "foo2/bar")); + + errors += testPathFunction2("parentDirectory", parentDirectory, "", true, ""); + errors += testPathFunction2("parentDirectory", parentDirectory, "/", true, "/"); + errors += testPathFunction2("parentDirectory", parentDirectory, "/a", true, "/"); + errors += testPathFunction2("parentDirectory", parentDirectory, ".", false, cleanPath(joinPath(cwd, "..")) + "/"); + errors += testPathFunction2("parentDirectory", parentDirectory, "./foo", false, cleanPath(cwd) + "/"); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/two/three/four", false, joinPath(cwd, "one/two/three/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/two/three/./four", false, joinPath(cwd, "one/two/three/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/two/three/./four/..", false, joinPath(cwd, "one/two/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "one/./two/../three/./four", false, joinPath(cwd, "one/three/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/four/../two", false, joinPath(cwd, "simfdb/backups/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../two", false, joinPath(cwd, "simfdb/backups/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "foo/./../foo2/./bar//", false, joinPath(cwd, "foo2/")); + errors += testPathFunction2("parentDirectory", parentDirectory, "foo/./../foo2/./bar//", true, joinPath(cwd, "foo2/")); + + printf("%d errors.\n", errors); + if(errors) + exit(-1); +} + int main(int argc, char** argv) { std::string lockfile = "/var/run/fdbmonitor.pid"; std::string _confpath = "/etc/foundationdb/foundationdb.conf"; diff --git a/flow/Platform.cpp b/flow/Platform.cpp index b6e4a27aa6..404a0ae969 100644 --- a/flow/Platform.cpp +++ b/flow/Platform.cpp @@ -1840,7 +1840,7 @@ std::string cleanPath(std::string const &path) { // Removes the last component from a path string (if possible) and returns the result with one trailing separator. // If there is only one path component, the result will be "" for relative paths and "/" for absolute paths. -// Note that this is NOT the same as getting the parant of path, as the final component could be ".." +// Note that this is NOT the same as getting the parent of path, as the final component could be ".." // or "." and it would still be simply removed. // ALL of the following inputs will yield the result "/a/" // /a/b @@ -1893,8 +1893,9 @@ std::string abspath( std::string const& path, bool resolveLinks, bool mustExist if(!resolveLinks) { // TODO: Not resolving symbolic links does not yet behave well on Windows because of drive letters // and network names, so it's not currently allowed here (but it is allowed in fdbmonitor which is unix-only) - ASSERT(g_network->isSimulated()); - std::string clean = cleanPath(joinPath(platform::getWorkingDirectory(), path)); + ASSERT(false); + bool absolute = !path.empty() && path[0] == CANONICAL_PATH_SEPARATOR; + std::string clean = cleanPath(absolute ? path : joinPath(platform::getWorkingDirectory(), path)); if(mustExist && !fileExists(clean)) { Error e = systemErrorCodeToError(); Severity sev = e.code() == error_code_io_error ? SevError : SevWarnAlways; @@ -1945,16 +1946,7 @@ std::string abspath( std::string const& path, bool resolveLinks, bool mustExist } std::string parentDirectory( std::string const& path, bool resolveLinks, bool mustExist ) { - auto abs = abspath(path, resolveLinks, mustExist); - size_t sep = abs.find_last_of( CANONICAL_PATH_SEPARATOR ); - if (sep == std::string::npos) { - Error e = platform_error(); - TraceEvent(SevWarnAlways, "GetParentDirectory") - .detail("File", path) - .error(e); - throw e; - } - return abs.substr(0, sep + 1); + return popPath(abspath(path, resolveLinks, mustExist)); } std::string basename( std::string const& filename ) { @@ -2961,12 +2953,18 @@ int testPathFunction(const char *name, std::function f return r ? 0 : 1; } -int testPathFunction2(const char *name, std::function fun, std::string a, bool x, bool y, ErrorOr b) { +int testPathFunction2(const char *name, std::function fun, std::string a, bool resolveLinks, bool mustExist, ErrorOr b) { + // Skip tests with resolveLinks set to false as the implementation is not complete + if(resolveLinks == false) { + printf("SKIPPED: %s('%s', %d, %d)\n", name, a.c_str(), resolveLinks, mustExist); + return 0; + } + ErrorOr result; - try { result = fun(a, x, y); } catch(Error &e) { result = e; } + try { result = fun(a, resolveLinks, mustExist); } catch(Error &e) { result = e; } bool r = result.isError() == b.isError() && (b.isError() || b.get() == result.get()) && (!b.isError() || b.getError().code() == result.getError().code()); - printf("%s: %s('%s', %d, %d) -> %s", r ? "PASS" : "FAIL", name, a.c_str(), x, y, result.isError() ? result.getError().what() : format("'%s'", result.get().c_str()).c_str()); + printf("%s: %s('%s', %d, %d) -> %s", r ? "PASS" : "FAIL", name, a.c_str(), resolveLinks, mustExist, result.isError() ? result.getError().what() : format("'%s'", result.get().c_str()).c_str()); if(!r) { printf(" *ERROR* expected %s", b.isError() ? b.getError().what() : format("'%s'", b.get().c_str()).c_str()); } @@ -3027,6 +3025,8 @@ TEST_CASE("/flow/Platform/directoryOps") { errors += testPathFunction2("parentDirectory", parentDirectory, "simfdb/backups/five/../three/../four", true, false, joinPath(cwd, "simfdb/backups/one/")); #endif + errors += testPathFunction2("abspath", abspath, "/", false, false, "/"); + errors += testPathFunction2("abspath", abspath, "/foo//bar//baz/.././", false, false, "/foo/bar"); errors += testPathFunction2("abspath", abspath, "/", true, false, "/"); errors += testPathFunction2("abspath", abspath, "", true, false, platform_error()); errors += testPathFunction2("abspath", abspath, ".", true, false, cwd); diff --git a/flow/Platform.h b/flow/Platform.h index 2c34900cbd..2574770927 100644 --- a/flow/Platform.h +++ b/flow/Platform.h @@ -321,8 +321,9 @@ void writeFile(std::string const& filename, std::string const& content); std::string joinPath( std::string const& directory, std::string const& filename ); // cleanPath() does a 'logical' resolution of the given path string to a canonical form *without* -// following symbolic links or verifying the existence of any path components, removing redundant -// "." references and duplicate separators, and resolving any ".." references. +// following symbolic links or verifying the existence of any path components. It removes redundant +// "." references and duplicate separators, and resolves any ".." references that can be resolved +// using the preceding path components. // Relative paths remain relative and are NOT rebased on the current working directory. std::string cleanPath( std::string const& path ); From a6b598f2367914079d4829d6125a0d78a2e46647 Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Thu, 21 Mar 2019 17:51:00 -0700 Subject: [PATCH 09/12] Missing include. --- fdbmonitor/fdbmonitor.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/fdbmonitor/fdbmonitor.cpp b/fdbmonitor/fdbmonitor.cpp index 236d06aa00..a043b8dd1c 100644 --- a/fdbmonitor/fdbmonitor.cpp +++ b/fdbmonitor/fdbmonitor.cpp @@ -52,6 +52,7 @@ #include #include #include +#include #include #include From 6c42e42334ed3a03cd0d1c0bb7a0a48615c07772 Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Thu, 21 Mar 2019 18:05:30 -0700 Subject: [PATCH 10/12] Fix compiler complaint about ignored return codes being ignored. Added commented call to testPathOps() for convenience. --- fdbmonitor/fdbmonitor.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/fdbmonitor/fdbmonitor.cpp b/fdbmonitor/fdbmonitor.cpp index a043b8dd1c..0d9985204c 100644 --- a/fdbmonitor/fdbmonitor.cpp +++ b/fdbmonitor/fdbmonitor.cpp @@ -1111,8 +1111,12 @@ void testPathOps() { std::string cwd = abspath(".", true); // Create some symlinks and test resolution (or non-resolution) of them - symlink("one/two", "simfdb/backups/four"); - symlink("../backups/four", "simfdb/backups/five"); + int rc; + // Ignoring return codes, if symlinks fail tests below will fail + rc = unlink("simfdb/backups/four"); + rc = unlink("simfdb/backups/five"); + rc = symlink("one/two", "simfdb/backups/four") == 0 ? 0 : 1; + rc = symlink("../backups/four", "simfdb/backups/five") ? 0 : 1; errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../two", true, joinPath(cwd, "simfdb/backups/one/two")); errors += testPathFunction2("abspath", abspath, "simfdb/backups/five/../three", true, joinPath(cwd, "simfdb/backups/one/three")); @@ -1156,6 +1160,8 @@ void testPathOps() { } int main(int argc, char** argv) { + // testPathOps(); return -1; + std::string lockfile = "/var/run/fdbmonitor.pid"; std::string _confpath = "/etc/foundationdb/foundationdb.conf"; From 524a66629041f2d8dd7c53df04c6d478c39ab80f Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Fri, 22 Mar 2019 11:44:46 -0700 Subject: [PATCH 11/12] Added back previous handling of ~ in paths, with the improvement that it only treats ~ as special if it is the first character. --- fdbmonitor/fdbmonitor.cpp | 8 ++++++-- flow/Platform.cpp | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/fdbmonitor/fdbmonitor.cpp b/fdbmonitor/fdbmonitor.cpp index 0d9985204c..597870ea99 100644 --- a/fdbmonitor/fdbmonitor.cpp +++ b/fdbmonitor/fdbmonitor.cpp @@ -302,7 +302,8 @@ std::string abspath( std::string const& path, bool resolveLinks = true) { } if(!resolveLinks) { - bool absolute = !path.empty() && path[0] == CANONICAL_PATH_SEPARATOR; + // Treat paths starting with ~ or separator as absolute, meaning they shouldn't be appended to the current working dir + bool absolute = !path.empty() && (path[0] == CANONICAL_PATH_SEPARATOR || path[0] == '~'); return cleanPath(absolute ? path : joinPath(abspath(".", true), path)); } @@ -318,7 +319,10 @@ std::string abspath( std::string const& path, bool resolveLinks = true) { if(prefix.empty()) { prefix = "."; } - return cleanPath(joinPath(abspath(prefix, true), suffix)); + // Home directory references via ~ are not handled + if(prefix[0] != '~') { + return cleanPath(joinPath(abspath(prefix, true), suffix)); + } } log_err("realpath", errno, "Unable to get real path for %s", path.c_str()); return ""; diff --git a/flow/Platform.cpp b/flow/Platform.cpp index 404a0ae969..be0dbcaba8 100644 --- a/flow/Platform.cpp +++ b/flow/Platform.cpp @@ -1894,7 +1894,8 @@ std::string abspath( std::string const& path, bool resolveLinks, bool mustExist // TODO: Not resolving symbolic links does not yet behave well on Windows because of drive letters // and network names, so it's not currently allowed here (but it is allowed in fdbmonitor which is unix-only) ASSERT(false); - bool absolute = !path.empty() && path[0] == CANONICAL_PATH_SEPARATOR; + // Treat paths starting with ~ or separator as absolute, meaning they shouldn't be appended to the current working dir + bool absolute = !path.empty() && (path[0] == CANONICAL_PATH_SEPARATOR || path[0] == '~'); std::string clean = cleanPath(absolute ? path : joinPath(platform::getWorkingDirectory(), path)); if(mustExist && !fileExists(clean)) { Error e = systemErrorCodeToError(); @@ -1932,7 +1933,10 @@ std::string abspath( std::string const& path, bool resolveLinks, bool mustExist if(prefix.empty()) { prefix = "."; } - return cleanPath(joinPath(abspath(prefix, true, false), suffix)); + // Home directory references via ~ are not handled + if(prefix[0] != '~') { + return cleanPath(joinPath(abspath(prefix, true, false), suffix)); + } } Error e = systemErrorCodeToError(); Severity sev = e.code() == error_code_io_error ? SevError : SevWarnAlways; From 382a7bdc5f5347046a72581e85caf977100a2987 Mon Sep 17 00:00:00 2001 From: Stephen Atherton Date: Fri, 22 Mar 2019 16:21:12 -0700 Subject: [PATCH 12/12] Changed behavior regarding ~ and ~user paths to treat them as unresolvable symbolic links. --- fdbmonitor/fdbmonitor.cpp | 11 +++++++---- flow/Platform.cpp | 5 ++--- flow/Platform.h | 3 +++ 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/fdbmonitor/fdbmonitor.cpp b/fdbmonitor/fdbmonitor.cpp index 597870ea99..d5eaa3e909 100644 --- a/fdbmonitor/fdbmonitor.cpp +++ b/fdbmonitor/fdbmonitor.cpp @@ -316,12 +316,15 @@ std::string abspath( std::string const& path, bool resolveLinks = true) { if(errno == ENOENT && path != ".") { std::string prefix = popPath(path); std::string suffix = path.substr(prefix.size()); - if(prefix.empty()) { + if(prefix.empty() && (suffix.empty() || suffix[0] != '~')) { prefix = "."; } - // Home directory references via ~ are not handled - if(prefix[0] != '~') { - return cleanPath(joinPath(abspath(prefix, true), suffix)); + if(!prefix.empty()) { + std::string abs = abspath(prefix, true); + // An empty return from abspath() means there was an error. + if(abs.empty()) + return abs; + return cleanPath(joinPath(abs, suffix)); } } log_err("realpath", errno, "Unable to get real path for %s", path.c_str()); diff --git a/flow/Platform.cpp b/flow/Platform.cpp index be0dbcaba8..b42b40c56c 100644 --- a/flow/Platform.cpp +++ b/flow/Platform.cpp @@ -1930,11 +1930,10 @@ std::string abspath( std::string const& path, bool resolveLinks, bool mustExist if(errno == ENOENT && !mustExist) { std::string prefix = popPath(path); std::string suffix = path.substr(prefix.size()); - if(prefix.empty()) { + if(prefix.empty() && (suffix.empty() || suffix[0] != '~')) { prefix = "."; } - // Home directory references via ~ are not handled - if(prefix[0] != '~') { + if(!prefix.empty()) { return cleanPath(joinPath(abspath(prefix, true, false), suffix)); } } diff --git a/flow/Platform.h b/flow/Platform.h index 2574770927..3dc0f5e42d 100644 --- a/flow/Platform.h +++ b/flow/Platform.h @@ -333,6 +333,9 @@ std::string cleanPath( std::string const& path ); // An empty path or a non-existent path when mustExist is true will result in a platform_error() exception. // Upon success, all '..' references will be resolved with the assumption that non-existent components // are NOT symbolic links. +// User directory references such as '~' or '~user' are effectively treated as symbolic links which +// are impossible to resolve, so resolveLinks=true results in failure and resolveLinks=false results +// in the reference being left in-tact prior to resolving '..' references. std::string abspath( std::string const& path, bool resolveLinks = true, bool mustExist = false ); // parentDirectory() returns the parent directory of the given file or directory in a canonical form,