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.

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;
+		if (TryUnlink(filename) || TryRmdir(filename))
-		int removed = 0;
-
-		int statOK = stat(filename, &fileStat);
-		if (statOK < 0)
 		{
-			if (errno == ENOENT)
+			return;
-			{
-				return;  /* if file does not exist, return */
-			}
-			else
-			{
-				ereport(ERROR, (errcode_for_file_access(),
-								errmsg("could not stat file \"%s\": %m", filename)));
-			}
 		}
 
 		/*
-		 * If this is a directory, iterate over all its contents and for each
+		 * If both of these failed then filename is a non empty directory.
-		 * content, recurse into this function. Also, make sure that we do not
+		 * Iterate over all its contents and for each content, recurse into
-		 * recurse into symbolic links.
+		 * 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(),
+			/* If directory was removed from under us we're done */
-								errmsg("could not open directory \"%s\": %m",
+			return;
-									   directoryName)));
+		}
+		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)));
 }