Merge pull request #533 from ajbeamon/fix-parent-directory

Fixes to parentDirectory() and abspath()

fdbmonitor/fdbmonitor.cpp

@@ -52,6 +52,7 @@
 #include <string>
 #include <sstream>
 #include <iterator>
+#include <functional>
 
 #include <string.h>
 #include <errno.h>
@@ -223,34 +224,117 @@ std::string joinPath(std::string const& directory, std::string const& filename)
 	return d + CANONICAL_PATH_SEPARATOR + f;
 }
 
-std::string abspath(std::string const& filename) {
+std::string cleanPath(std::string const &path) {
-	// Returns an absolute path canonicalized to use only CANONICAL_PATH_SEPARATOR
+	std::vector<std::string> finalParts;
-	char result[PATH_MAX];
+	bool absolute = !path.empty() && path[0] == CANONICAL_PATH_SEPARATOR;
-	auto r = realpath( filename.c_str(), result );
+
-	if (!r) {
+	int i = 0;
-		if (errno == ENOENT) {
+	while(i < path.size()) {
-			int sep = filename.find_last_of( CANONICAL_PATH_SEPARATOR );
+		int sep = path.find((char)CANONICAL_PATH_SEPARATOR, i);
-			if (sep != std::string::npos) {
+		if(sep == path.npos) {
-				return joinPath( abspath( filename.substr(0, sep) ), filename.substr(sep) );
+			sep = path.size();
+		}
+		std::string part = path.substr(i, sep - i);
+		i = sep + 1;
+		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) {
+			if(absolute) {
-				return joinPath( abspath( "." ), filename );
+				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) {
+		// 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));
+	}
+
+	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() && (suffix.empty() || suffix[0] != '~')) {
+				prefix = ".";
+			}
+			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());
 		return "";
 	}
 	return std::string(r);
 }
 
-std::string parentDirectory(std::string const& filename) {
+std::string parentDirectory( std::string const& path, bool resolveLinks = true) {
-	size_t sep = filename.find_last_of( CANONICAL_PATH_SEPARATOR );
+	return popPath(abspath(path, resolveLinks));
-	if (sep == std::string::npos) {
-		return "";
-	}
-
-	return filename.substr(0, sep);
 }
 
 int mkdir(std::string const& directory) {
@@ -847,7 +931,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 ) {
@@ -939,7 +1023,7 @@ std::unordered_map<int, std::unordered_set<std::string>> 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);
@@ -950,7 +1034,7 @@ std::unordered_map<int, std::unordered_set<std::string>> 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 */
@@ -961,7 +1045,7 @@ std::unordered_map<int, std::unordered_set<std::string>> 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;
 			}
 
@@ -987,7 +1071,104 @@ std::unordered_map<int, std::unordered_set<std::string>> set_watches(std::string
 }
 #endif
 
+int testPathFunction(const char *name, std::function<std::string(std::string)> 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<std::string(std::string, bool)> 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
+	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"));
+	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) {
+	// testPathOps(); return -1;
+
 	std::string lockfile = "/var/run/fdbmonitor.pid";
 	std::string _confpath = "/etc/foundationdb/foundationdb.conf";
 
@@ -1048,8 +1229,8 @@ 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()+1);
+	std::string conffile = confpath.substr(confdir.size());
 
 #ifdef __linux__
 	// Setup inotify
@@ -1113,7 +1294,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);
@@ -1440,7 +1621,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

fdbserver/worker.actor.cpp

@@ -635,6 +635,8 @@ ACTOR Future<Void> workerServer( Reference<ClusterConnectionFile> connFile, Refe
 
 	state WorkerInterface interf( locality );
 
+	folder = abspath(folder);
+
 	if(metricsPrefix.size() > 0) {
 		if( metricsConnFile.size() > 0) {
 			try {
@@ -725,7 +727,7 @@ ACTOR Future<Void> workerServer( Reference<ClusterConnectionFile> 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 );
 				const DiskQueueVersion dqv = s.tLogOptions.version >= TLogVersion::V3 ? DiskQueueVersion::V1 : DiskQueueVersion::V0;
 				const int64_t diskQueueWarnSize = s.tLogOptions.spillType == TLogSpillType::VALUE ? 10*SERVER_KNOBS->TARGET_BYTES_PER_TLOG : -1;

flow/Platform.cpp

@@ -1633,7 +1633,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;
 }
 
@@ -1666,7 +1666,7 @@ void atomicReplace( std::string const& path, std::string const& content, bool te
 	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 = textmode ? fopen( tempfilename.c_str(), "wt" ) : fopen(tempfilename.c_str(), "wb");
 		if(!f)
 			throw io_error();
@@ -1835,15 +1835,122 @@ bool createDirectory( std::string const& directory ) {
 
 }; // namespace platform
 
-std::string abspath( std::string const& filename ) {
+const uint8_t separatorChar = CANONICAL_PATH_SEPARATOR;
+StringRef separator(&separatorChar, 1);
+StringRef dotdot = LiteralStringRef("..");
+
+std::string cleanPath(std::string const &path) {
+	std::vector<StringRef> 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) {
+				continue;
+			}
+		}
+		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.empty() ? "." : result;
+}
+
+// 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 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
+//   /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(false);
+		// 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();
+			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...
@@ -1852,20 +1959,26 @@ 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 );
+	// Must resolve links, so first try realpath on the whole thing
-	if (!r) {
+	const char *r = realpath( path.c_str(), result );
-		if (errno == ENOENT) {
+	if(r == nullptr) {
-			int sep = filename.find_last_of( CANONICAL_PATH_SEPARATOR );
+		// If the error was ENOENT and the path doesn't have to exist,
-			if (sep != std::string::npos) {
+		// try to resolve symlinks in progressively shorter prefixes of the path
-				return joinPath( abspath( filename.substr(0, sep) ), filename.substr(sep) );
+		if(errno == ENOENT && !mustExist) {
+			std::string prefix = popPath(path);
+			std::string suffix = path.substr(prefix.size());
+			if(prefix.empty() && (suffix.empty() || suffix[0] != '~')) {
+				prefix = ".";
 			}
-			else if (filename.find("~") == std::string::npos) {
+			if(!prefix.empty()) {
-				return joinPath( abspath( "." ), 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);
@@ -1874,6 +1987,10 @@ std::string abspath( std::string const& filename ) {
 #endif
 }
 
+std::string parentDirectory( std::string const& path, bool resolveLinks, bool mustExist ) {
+	return popPath(abspath(path, resolveLinks, mustExist));
+}
+
 std::string basename( std::string const& filename ) {
 	auto abs = abspath(filename);
 	size_t sep = abs.find_last_of( CANONICAL_PATH_SEPARATOR );
@@ -1881,18 +1998,6 @@ std::string basename( std::string const& filename ) {
 	return abs.substr(sep+1);
 }
 
-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")
-			.detail("File", filename)
-			.GetLastError();
-		throw platform_error();
-	}
-	return abs.substr(0, sep);
-}
-
 std::string getUserHomeDirectory() {
 #if defined(__unixish__)
 	const char* ret = getenv( "HOME" );
@@ -2877,3 +2982,142 @@ TEST_CASE("/flow/Platform/getMemoryInfo") {
 	return Void();
 }
 #endif
+
+int testPathFunction(const char *name, std::function<std::string(std::string)> fun, std::string a, ErrorOr<std::string> b) {
+	ErrorOr<std::string> 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') -> %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("\n");
+	return r ? 0 : 1;
+}
+
+int testPathFunction2(const char *name, std::function<std::string(std::string, bool, bool)> fun, std::string a, bool resolveLinks, bool mustExist, ErrorOr<std::string> 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<std::string> result;
+	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(), 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());
+	}
+	printf("\n");
+	return r ? 0 : 1;
+}
+
+TEST_CASE("/flow/Platform/directoryOps") {
+	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("popPath", popPath, "/a/b///..//", "/a/b/");
+
+	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, "/..", "/");
+	errors += testPathFunction("cleanPath", cleanPath, "/../foo/bar///", "/foo/bar");
+	errors += testPathFunction("cleanPath", cleanPath, "/a/b/../.././../", "/");
+	errors += testPathFunction("cleanPath", cleanPath, ".", ".");
+
+	// Creating this directory in backups avoids some sanity checks
+	platform::createDirectory("simfdb/backups/one/two/three");
+	std::string cwd = platform::getWorkingDirectory();
+
+#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, "/", 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);
+	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();
+}

flow/Platform.h

@@ -320,15 +320,33 @@ 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
+// cleanPath() does a 'logical' resolution of the given path string to a canonical form *without*
-std::string abspath( std::string const& filename );
+// 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 );
+
+// 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.
+// 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,
+// 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
-std::string parentDirectory( std::string const& filename );
-
 // Returns the home directory of the current user
 std::string getUserHomeDirectory();