Semmle: Protect against theoretical race in recursive directory… (#3559)

In between stat at the start of the loop and unlink/rmdir at the end the
item that the filename references might have changed. In some cases this
can be a security bug, but since we only delete the file/directory it
should not be for us as far as I can tell. It could in theory still
cause errors though if the a file is changed into a directory by some
other process. This commit makes the code robust against that, by not
using stat and only rely on error codes and retries.

(cherry picked from commit ca8f7119fe)

src/backend/distributed/worker/worker_partition_protocol.c

@@ -85,7 +85,8 @@ static uint32 RangePartitionId(Datum partitionValue, Oid partitionCollation,
 static uint32 HashPartitionId(Datum partitionValue, Oid partitionCollation,
 							  const void *context);
 static StringInfo UserPartitionFilename(StringInfo directoryName, uint32 partitionId);
-static bool FileIsLink(const char *filename, struct stat filestat);
+static bool TryUnlink(const char *filename);
+static bool TryRmdir(const char *filename);
 
 
 /* exports for SQL callable functions */
@@ -688,25 +689,6 @@ CitusCreateDirectory(StringInfo directoryName)
 }
 
 
-#ifdef WIN32
-static bool
-FileIsLink(char *filename, struct stat filestat)
-{
-	return pgwin32_is_junction(filename);
-}
-
-
-#else
-static bool
-FileIsLink(const char *filename, struct stat filestat)
-{
-	return S_ISLNK(filestat.st_mode);
-}
-
-
-#endif
-
-
 /*
  * CitusRemoveDirectory first checks if the given directory exists. If it does, the
  * function recursively deletes the contents of the given directory, and then
@@ -719,42 +701,34 @@ CitusRemoveDirectory(const char *filename)
 	/* files may be added during execution, loop when that occurs */
 	while (true)
 	{
-		struct stat fileStat;
-		int removed = 0;
-
-		int fileStated = stat(filename, &fileStat);
-		if (fileStated < 0)
+		if (TryUnlink(filename) || TryRmdir(filename))
 		{
-			if (errno == ENOENT)
-			{
-				return;  /* if file does not exist, return */
-			}
-			else
-			{
-				ereport(ERROR, (errcode_for_file_access(),
-								errmsg("could not stat file \"%s\": %m", filename)));
-			}
+			return;
 		}
 
 		/*
-		 * If this is a directory, iterate over all its contents and for each
-		 * content, recurse into this function. Also, make sure that we do not
-		 * recurse into symbolic links.
+		 * If both of these failed then filename is a non empty directory.
+		 * Iterate over all its contents and for each content, recurse into
+		 * this function. Also, make sure that we do not recurse into symbolic
+		 * links.
 		 */
-		if (S_ISDIR(fileStat.st_mode) && !FileIsLink(filename, fileStat))
-		{
 		const char *directoryName = filename;
 
 		DIR *directory = AllocateDir(directoryName);
-			if (directory == NULL)
+		if (errno == ENOENT)
 		{
-				ereport(ERROR, (errcode_for_file_access(),
-								errmsg("could not open directory \"%s\": %m",
-									   directoryName)));
+			/* If directory was removed from under us we're done */
+			return;
+		}
+		if (errno == ENOTDIR)
+		{
+			/* If directory changed to a file underneath us, loop again to remove it with unlink */
+			continue;
 		}
 
-			StringInfo fullFilename = makeStringInfo();
+		/* ReadDir handles other errno from AllocateDir */
 		struct dirent *directoryEntry = ReadDir(directory, directoryName);
+		StringInfo fullFilename = makeStringInfo();
 		for (; directoryEntry != NULL; directoryEntry = ReadDir(directory,
 																directoryName))
 		{
@@ -776,30 +750,53 @@ CitusRemoveDirectory(const char *filename)
 		FreeStringInfo(fullFilename);
 		FreeDir(directory);
 	}
-
-		/* we now have an empty directory or a regular file, remove it */
-		if (S_ISDIR(fileStat.st_mode))
-		{
-			removed = rmdir(filename);
-
-			if (errno == ENOTEMPTY || errno == EEXIST)
-			{
-				continue;
-			}
-		}
-		else
-		{
-			removed = unlink(filename);
 }
 
-		if (removed != 0 && errno != ENOENT)
+
+/*
+ * TryUnlink returns true if file is removed and false if the file is a
+ * directory. If an error occurs trying to remove the file this function calls
+ * ereport.
+ */
+static bool
+TryUnlink(const char *filename)
 {
+	errno = 0;
+	int exitcode = unlink(filename);
+	if (exitcode == 0 || errno == ENOENT)
+	{
+		return true;
+	}
+	if (errno == EISDIR)
+	{
+		return false;
+	}
 	ereport(ERROR, (errcode_for_file_access(),
 					errmsg("could not remove file \"%s\": %m", filename)));
 }
 
-		return;
+
+/*
+ * TryRmdir returns true if the directory is removed. It returns false if the
+ * filename points to a file or symlink. It also returns false if the directory
+ * is not empty. The difference in these cases can be found by checking errno.
+ * For all other errors ereport is called.
+ */
+static bool
+TryRmdir(const char *filename)
+{
+	errno = 0;
+	int exitcode = rmdir(filename);
+	if (exitcode == 0 || errno == ENOENT)
+	{
+		return true;
 	}
+	if (errno == ENOTDIR || errno == ENOTEMPTY || errno == EEXIST)
+	{
+		return false;
+	}
+	ereport(ERROR, (errcode_for_file_access(),
+					errmsg("could not remove directory \"%s\": %m", filename)));
 }