Android storage: Never make numbered duplicates, report real errors

Given a taken name, the storage provider makes "name (1)" rather than
fail. Renaming onto an existing file did exactly that and reported
success, so a game's sceIoRename onto a save file left a duplicate
instead of failing like on a PSP. Creating, renaming and copying now
check the name the document ended up with, undo a renamed one, and
return ALREADY_EXISTS. A new storage test checks that no operation onto
a taken name leaves a duplicate.

The Java side only ever returned UNKNOWN. It now maps a missing file to
NOT_FOUND and a full disk to DISK_FULL, and OpenFD/OpenCFile set errno
from those, so DirectoryFileSystem reports a full disk on content URIs
like on native paths, instead of "file not found".

That also lets writing open first, and only create when the file is
missing: overwriting a file is one call to the provider instead of two
(18.6 -> 13.2 ms in the storage test on an SD card).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Common/File/FileUtil.cpp

@@ -142,6 +142,46 @@ void InitLibretroVFS(const struct retro_vfs_interface_info *vfs) noexcept {
 }
 #endif
 
+// For callers that look at errno, as they do with native paths.
+static int ErrnoFromStorageError(StorageError error) {
+	switch (error) {
+	case StorageError::SUCCESS: return 0;
+	case StorageError::NOT_FOUND: return ENOENT;
+	case StorageError::DISK_FULL: return ENOSPC;
+	case StorageError::ALREADY_EXISTS: return EEXIST;
+	default: return EIO;
+	}
+}
+
+// Opens a content URI for writing, creating the file if it's missing. Opening first saves the existence check
+// when the file is there, the usual case. Creating never leaves a numbered duplicate: given a taken name,
+// Android_CreateFile fails with ALREADY_EXISTS (someone created it meanwhile), and then we just open it.
+static int OpenContentUriForWrite(const Path &path, Android_OpenContentUriMode mode) {
+	int fd = Android_OpenContentUriFd(path.ToString(), mode);
+	if (fd >= 0 || StorageErrorFromInt(fd) != StorageError::NOT_FOUND) {
+		if (fd < 0) {
+			errno = ErrnoFromStorageError(StorageErrorFromInt(fd));
+		}
+		return fd;
+	}
+	if (!path.CanNavigateUp()) {
+		errno = ENOENT;
+		return -1;
+	}
+	const Path parent = path.NavigateUp();
+	const StorageError created = Android_CreateFile(parent.ToString(), path.GetFilename());
+	if (created != StorageError::SUCCESS && created != StorageError::ALREADY_EXISTS) {
+		WARN_LOG(Log::IO, "Failed to create '%s' in '%s': %s", path.GetFilename().c_str(), parent.c_str(), Android_ErrorToString(created));
+		errno = ErrnoFromStorageError(created);
+		return -1;
+	}
+	fd = Android_OpenContentUriFd(path.ToString(), mode);
+	if (fd < 0) {
+		errno = ErrnoFromStorageError(StorageErrorFromInt(fd));
+	}
+	return fd;
+}
+
 FILE *OpenCFile(const Path &path, const char *mode) {
 	if (LOG_IO) {
 		INFO_LOG(Log::IO, "OpenCFile %s, %s", path.c_str(), mode);
@@ -160,31 +200,11 @@ FILE *OpenCFile(const Path &path, const char *mode) {
 			// Read, let's support this - easy one.
 			int descriptor = Android_OpenContentUriFd(path.ToString(), Android_OpenContentUriMode::READ);
 			if (descriptor < 0) {
+				errno = ErrnoFromStorageError(StorageErrorFromInt(descriptor));
 				return nullptr;
 			}
 			return fdopen(descriptor, "rb");
 		} else if (!strcmp(mode, "w") || !strcmp(mode, "wb") || !strcmp(mode, "wt") || !strcmp(mode, "at") || !strcmp(mode, "a")) {
-			// Need to be able to create the file here if it doesn't exist.
-			// NOTE: The existance check is important, otherwise Android will create a numbered file by the side!
-			// This is also a terrible possible data race, ugh. Anyway...
-			// Not exactly sure which abstractions are best, let's start simple.
-			if (!File::Exists(path)) {
-				DEBUG_LOG(Log::IO, "OpenCFile(%s): Opening content file for write. Doesn't exist, creating empty and reopening.", path.c_str());
-				std::string name = path.GetFilename();
-				if (path.CanNavigateUp()) {
-					Path parent = path.NavigateUp();
-					if (Android_CreateFile(parent.ToString(), name) != StorageError::SUCCESS) {
-						WARN_LOG(Log::IO, "Failed to create file '%s' in '%s'", name.c_str(), parent.c_str());
-						return nullptr;
-					}
-				} else {
-					INFO_LOG(Log::IO, "Failed to navigate up to create file: %s", path.c_str());
-					return nullptr;
-				}
-			} else {
-				DEBUG_LOG(Log::IO, "OpenCFile(%s): Opening existing content file for write (truncating). Requested mode: '%s'", path.c_str(), mode);
-			}
-
 			// TODO: Support append modes and stuff... For now let's go with the most common one.
 			Android_OpenContentUriMode openMode = Android_OpenContentUriMode::READ_WRITE_TRUNCATE;
 			const char *fmode = "wb";
@@ -192,7 +212,7 @@ FILE *OpenCFile(const Path &path, const char *mode) {
 				openMode = Android_OpenContentUriMode::READ_WRITE;
 				fmode = "ab";
 			}
-			int descriptor = Android_OpenContentUriFd(path.ToString(), openMode);
+			int descriptor = OpenContentUriForWrite(path, openMode);
 			if (descriptor < 0) {
 				INFO_LOG(Log::IO, "Opening '%s' for write failed", path.ToString().c_str());
 				return nullptr;
@@ -284,29 +304,6 @@ int OpenFD(const Path &path, OpenFlag flags) {
 		return -1;
 	}
 
-	bool knownExists = false;
-
-	if (flags & OPEN_CREATE) {
-		if (!File::Exists(path)) {
-			INFO_LOG(Log::IO, "OpenFD(%s): Creating file.", path.c_str());
-			std::string name = path.GetFilename();
-			if (path.CanNavigateUp()) {
-				Path parent = path.NavigateUp();
-				if (Android_CreateFile(parent.ToString(), name) != StorageError::SUCCESS) {
-					WARN_LOG(Log::IO, "OpenFD: Failed to create file '%s' in '%s'", name.c_str(), parent.c_str());
-					return -1;
-				}
-				knownExists = true;
-			} else {
-				INFO_LOG(Log::IO, "Failed to navigate up to create file: %s", path.c_str());
-				return -1;
-			}
-		} else {
-			INFO_LOG(Log::IO, "OpenCFile(%s): Opening existing content file ('%s')", path.c_str(), OpenFlagToString(flags).c_str());
-			knownExists = true;
-		}
-	}
-
 	Android_OpenContentUriMode mode;
 	if (flags == OPEN_READ) {  // Intentionally not a bitfield check.
 		mode = Android_OpenContentUriMode::READ;
@@ -324,14 +321,17 @@ int OpenFD(const Path &path, OpenFlag flags) {
 	}
 
 	DEBUG_LOG(Log::IO, "Android_OpenContentUriFd: %s (%s)", path.c_str(), OpenFlagToString(flags).c_str());
-	int descriptor = Android_OpenContentUriFd(path.ToString(), mode);
-	if (descriptor < 0) {
-		// File probably just doesn't exist. No biggie.
-		if (knownExists) {
-			ERROR_LOG(Log::IO, "Android_OpenContentUriFd failed for existing file: '%s'", path.c_str());
-		} else {
-			INFO_LOG(Log::IO, "Android_OpenContentUriFd failed, probably doesn't exist: '%s'", path.c_str());
+	int descriptor;
+	if (flags & OPEN_CREATE) {
+		descriptor = OpenContentUriForWrite(path, mode);
+	} else {
+		descriptor = Android_OpenContentUriFd(path.ToString(), mode);
+		if (descriptor < 0) {
+			errno = ErrnoFromStorageError(StorageErrorFromInt(descriptor));
 		}
+	}
+	if (descriptor < 0) {
+		INFO_LOG(Log::IO, "Opening '%s' (%s) failed: %s", path.c_str(), OpenFlagToString(flags).c_str(), strerror(errno));
 	} else if (flags & OPEN_APPEND) {
 		// Simply seek to the end of the file to simulate append mode.
 		lseek(descriptor, 0, SEEK_END);

Common/File/FileUtil.h

@@ -244,9 +244,8 @@ void InitLibretroVFS(const struct retro_vfs_interface_info *vfs) noexcept;
 // Whole-file reading/writing
 bool WriteStringToFile(bool textFile, std::string_view str, const Path &filename);
 bool WriteDataToFile(bool textFile, const void* data, size_t size, const Path &filename);
-// For a file that's known not to exist yet. On Android content URIs, this saves the existence
-// check WriteDataToFile makes, a round trip to the storage provider - but if the file does exist,
-// the provider makes a renamed copy next to it instead of overwriting it.
+// For a file that's known not to exist yet. On Android content URIs, this saves a round trip to the
+// storage provider, and fails if the file does exist.
 bool WriteDataToNewFile(const void *data, size_t size, const Path &filename);
 
 bool ReadFileToStringOptions(bool textFile, bool allowShort, const Path &path, std::string *str);

Core/FileSystems/DirectoryFileSystem.cpp

@@ -286,10 +286,15 @@ bool DirectoryFileHandle::Open(const Path &basePath, std::string &fileName, File
 			// Success
 			return true;
 		} else {
-			ERROR_LOG(Log::FileSystem, "File::OpenFD returned an error");
-			// TODO: Need better error codes from OpenFD so we can distinguish
-			// disk full. Just set not found for now.
-			error = SCE_KERNEL_ERROR_ERRNO_FILE_NOT_FOUND;
+			ERROR_LOG(Log::FileSystem, "File::OpenFD returned an error: %s", strerror(errno));
+			// OpenFD sets errno, like the native path below.
+			if (errno == ENOSPC) {
+				auto err = GetI18NCategory(I18NCat::ERRORS);
+				g_OSD.Show(OSDType::MESSAGE_ERROR, err->T("Disk full while writing data"), 0.0f, "diskfull");
+				error = SCE_KERNEL_ERROR_ERRNO_NO_PERM;
+			} else {
+				error = SCE_KERNEL_ERROR_ERRNO_FILE_NOT_FOUND;
+			}
 			return false;
 		}
 	}

Core/Util/StorageTest.cpp

@@ -19,6 +19,7 @@
 #include <vector>
 
 #include "Common/Data/Text/Parsers.h"
+#include "Common/File/AndroidStorage.h"
 #include "Common/File/DirListing.h"
 #include "Common/File/DiskFree.h"
 #include "Common/File/FileUtil.h"
@@ -170,6 +171,61 @@ int RunStorageTests(const Path &root, const std::function<void(const StorageTest
 		return "";
 	});
 
+	run("NoDuplicates", [&]() -> std::string {
+		// Android's storage provider makes "name (1)" rather than fail, when asked to create something
+		// whose name is taken. Nothing may ever do that to the user's files.
+		const Path dir = scratch / "dup";
+		CHECK(File::CreateFullPath(dir / "sub"));
+		CHECK(Write(dir / "a.txt", "a"));
+		CHECK(Write(dir / "b.txt", "b"));
+		CHECK(Write(dir / "sub/a.txt", "old"));
+		auto duplicates = [](const Path &folder) {
+			std::vector<File::FileInfo> files;
+			File::GetFilesInDir(folder, &files);
+			std::string found;
+			for (const File::FileInfo &f : files) {
+				if (f.name.find(" (") != std::string::npos) {
+					found += f.name + " ";
+				}
+			}
+			return found;
+		};
+		std::vector<std::string> outcomes;
+
+		// The memory stick move merges into existing data with this, so it has to overwrite.
+		const bool copied = File::Copy(dir / "a.txt", dir / "sub/a.txt");
+		CHECK(duplicates(dir / "sub").empty());
+		CHECK(copied && ReadAll(dir / "sub/a.txt") == "a");
+
+		// Rename and move onto a taken name differ between platforms (POSIX overwrites, Windows refuses), so
+		// only duplicates are wrong. Note what happened.
+		const bool renamed = File::Rename(dir / "b.txt", dir / "a.txt");
+		CHECK(duplicates(dir).empty());
+		outcomes.push_back(std::string("rename ") + (renamed ? (ReadAll(dir / "a.txt") == "b" ? "overwrote" : "?") : "refused"));
+		if (!renamed) {
+			CHECK(ReadAll(dir / "a.txt") == "a" && ReadAll(dir / "b.txt") == "b");
+		}
+
+		CHECK(Write(dir / "c.txt", "c"));
+		const bool moved = File::MoveIfFast(dir / "c.txt", dir / "sub/a.txt");
+		CHECK(duplicates(dir / "sub").empty());
+		outcomes.push_back(std::string("move ") + (moved ? (ReadAll(dir / "sub/a.txt") == "c" ? "overwrote" : "?") : "refused"));
+
+		// Creating what's there already, below the File functions (which check first).
+		if (contentURI) {
+			const StorageError file = Android_CreateFile(dir.ToString(), "sub");
+			const StorageError folder = Android_CreateDirectory(dir.ToString(), "sub");
+			CHECK(duplicates(dir).empty());
+			outcomes.push_back(StringFromFormat("create file %s, folder %s", Android_ErrorToString(file), Android_ErrorToString(folder)));
+		}
+		note = "";
+		for (const std::string &o : outcomes) {
+			note += (note.empty() ? "" : ", ") + o;
+		}
+		CHECK(File::DeleteDirRecursively(dir));
+		return "";
+	});
+
 	run("Names", [&]() -> std::string {
 		// Characters that need escaping in a content URI, and outside ASCII.
 		const char *name = "100% #1 & \xC3\xA4.txt";

android/src/org/ppsspp/ppsspp/ContentUri.java

@@ -45,6 +45,33 @@ public class ContentUri {
 	private static final int STORAGE_ERROR_DISK_FULL = -3;
 	private static final int STORAGE_ERROR_ALREADY_EXISTS = -4;
 
+	// So the native side can tell a missing file or a full disk from other failures.
+	private static int errorFromException(Exception e) {
+		if (e instanceof java.io.FileNotFoundException) {
+			return STORAGE_ERROR_NOT_FOUND;
+		}
+		final String message = e.getMessage();
+		if (message != null && (message.contains("ENOSPC") || message.contains("No space left"))) {
+			return STORAGE_ERROR_DISK_FULL;
+		}
+		return STORAGE_ERROR_UNKNOWN;
+	}
+
+	private static final String EXTERNAL_STORAGE_AUTHORITY = "com.android.externalstorage.documents";
+
+	// The name a document has. For shared storage it's the last part of the document ID ("primary:PSP/SAVEDATA"),
+	// other providers' IDs can be anything, so those are asked.
+	private static String documentName(Context context, Uri documentUri) {
+		if (EXTERNAL_STORAGE_AUTHORITY.equals(documentUri.getAuthority())) {
+			final String id = DocumentsContract.getDocumentId(documentUri);
+			final int slash = id.lastIndexOf('/');
+			return id.substring(slash >= 0 ? slash + 1 : id.indexOf(':') + 1);
+		}
+		try (Cursor c = context.getContentResolver().query(documentUri, new String[] { DocumentsContract.Document.COLUMN_DISPLAY_NAME }, null, null, null)) {
+			return c != null && c.moveToFirst() ? c.getString(0) : null;
+		}
+	}
+
 	// Without a client of our own, every query and DocumentsContract call acquires the storage provider and
 	// releases it again, and when our reference count drops to zero, each of those is another binder call,
 	// to the system. Holding one keeps the count up. Unstable, so the provider dying doesn't take our process
@@ -110,21 +137,21 @@ public class ContentUri {
 					// I'd expect an exception to happen before we get here, so this is probably
 					// never reached.
 					Log.e(TAG, "Failed to get file descriptor for " + uriString);
-					return -1;
+					return STORAGE_ERROR_UNKNOWN;
 				}
 				// Review note: We intentionally "leak" the fd here, the caller takes ownership and closes
 				// it when done with it.
 				return filePfd.detachFd();
 			}
-		} catch (java.lang.IllegalArgumentException e) {
-			// This exception is long and ugly and really just means file not found.
+		} catch (java.io.FileNotFoundException | IllegalArgumentException e) {
+			// The IllegalArgumentException is long and ugly and really just means file not found too.
 			// We don't log anything (the caller can log).
-			return -1;
+			return STORAGE_ERROR_NOT_FOUND;
 		} catch (Exception e) {
 			// Don't know when this might happen. Let's log. Still, the result is just a
 			// failure that the caller may additionally log.
 			Log.e(TAG, "Unexpected openContentUri exception: " + e);
-			return -1;
+			return errorFromException(e);
 		}
 	}
 
@@ -269,46 +296,42 @@ public class ContentUri {
 		}
 	}
 
-	@Keep
-	@SuppressWarnings("unused")
-	public static int contentUriCreateDirectory(Activity activity, String rootTreeUri, String dirName) {
+	// Given a taken name, the provider makes "name (1)" instead of failing. Nothing may leave such a duplicate of
+	// the user's files, so that's undone here and reported as ALREADY_EXISTS.
+	private static int createDocument(Activity activity, String parentUriString, String mimeType, String name) {
 		try {
-			Uri uri = Uri.parse(rootTreeUri);
-			DocumentFile documentFile = DocumentFile.fromTreeUri(activity, uri);
-			if (documentFile != null) {
-				DocumentFile createdDir = documentFile.createDirectory(dirName);
-				return createdDir != null ? STORAGE_ERROR_SUCCESS : STORAGE_ERROR_UNKNOWN;
-			} else {
-				Log.e(TAG, "contentUriCreateDirectory: fromTreeUri returned null");
+			Uri parentUri = Uri.parse(parentUriString);
+			if (!DocumentsContract.isDocumentUri(activity, parentUri)) {
+				parentUri = DocumentsContract.buildDocumentUriUsingTree(parentUri, DocumentsContract.getTreeDocumentId(parentUri));
+			}
+			final ContentResolver resolver = activity.getContentResolver();
+			final Uri created = DocumentsContract.createDocument(resolver, parentUri, mimeType, name);
+			if (created == null) {
 				return STORAGE_ERROR_UNKNOWN;
 			}
+			if (!name.equals(documentName(activity, created))) {
+				Log.w(TAG, "createDocument: '" + name + "' is taken, removing the renamed copy " + created);
+				DocumentsContract.deleteDocument(resolver, created);
+				return STORAGE_ERROR_ALREADY_EXISTS;
+			}
+			return STORAGE_ERROR_SUCCESS;
 		} catch (Exception e) {
-			Log.e(TAG, "contentUriCreateDirectory exception: " + e);
-			return STORAGE_ERROR_UNKNOWN;
+			Log.e(TAG, "createDocument exception: " + e);
+			return errorFromException(e);
 		}
 	}
 
 	@Keep
 	@SuppressWarnings("unused")
-	// NOTE: If the file already exists, this will NOT return an error, instead the OS will create a new
-	// file appending an incrementing number, for example (1), to the filename.
+	public static int contentUriCreateDirectory(Activity activity, String rootTreeUri, String dirName) {
+		return createDocument(activity, rootTreeUri, DocumentsContract.Document.MIME_TYPE_DIR, dirName);
+	}
+
+	// Fails with ALREADY_EXISTS if the name is taken.
+	@Keep
+	@SuppressWarnings("unused")
 	public static int contentUriCreateFile(Activity activity, String rootTreeUri, String fileName) {
-		try {
-			Uri uri = Uri.parse(rootTreeUri);
-			DocumentFile documentFile = DocumentFile.fromTreeUri(activity, uri);
-			if (documentFile != null) {
-				// TODO: Check the file extension and choose MIME type appropriately.
-				// Or actually, let's not bother.
-				DocumentFile createdFile = documentFile.createFile("application/octet-stream", fileName);
-				return createdFile != null ? STORAGE_ERROR_SUCCESS : STORAGE_ERROR_UNKNOWN;
-			} else {
-				Log.e(TAG, "contentUriCreateFile: fromTreeUri returned null");
-				return STORAGE_ERROR_UNKNOWN;
-			}
-		} catch (Exception e) {
-			Log.e(TAG, "contentUriCreateFile exception: " + e);
-			return STORAGE_ERROR_UNKNOWN;
-		}
+		return createDocument(activity, rootTreeUri, "application/octet-stream", fileName);
 	}
 
 	@Keep
@@ -324,7 +347,7 @@ public class ContentUri {
 			}
 		} catch (Exception e) {
 			Log.e(TAG, "contentUriRemoveFile exception: " + e);
-			return STORAGE_ERROR_UNKNOWN;
+			return errorFromException(e);
 		}
 	}
 
@@ -341,10 +364,22 @@ public class ContentUri {
 		try {
 			Uri srcUri = Uri.parse(srcFileUri);
 			Uri dstParentUri = Uri.parse(dstParentDirUri);
-			return DocumentsContract.copyDocument(activity.getContentResolver(), srcUri, dstParentUri) != null ? STORAGE_ERROR_SUCCESS : STORAGE_ERROR_UNKNOWN;
+			final ContentResolver resolver = activity.getContentResolver();
+			final Uri copy = DocumentsContract.copyDocument(resolver, srcUri, dstParentUri);
+			if (copy == null) {
+				return STORAGE_ERROR_UNKNOWN;
+			}
+			// A taken name gets a renamed copy, see createDocument.
+			final String name = documentName(activity, srcUri);
+			if (name != null && !name.equals(documentName(activity, copy))) {
+				Log.w(TAG, "contentUriCopyFile: '" + name + "' is taken, removing the renamed copy " + copy);
+				DocumentsContract.deleteDocument(resolver, copy);
+				return STORAGE_ERROR_ALREADY_EXISTS;
+			}
+			return STORAGE_ERROR_SUCCESS;
 		} catch (Exception e) {
 			Log.e(TAG, "contentUriCopyFile exception: " + e);
-			return STORAGE_ERROR_UNKNOWN;
+			return errorFromException(e);
 		}
 	}
 
@@ -368,7 +403,7 @@ public class ContentUri {
 			return result;
 		} catch (Exception e) {
 			Log.e(TAG, "contentUriMoveFile exception: " + e);
-			return STORAGE_ERROR_UNKNOWN;
+			return errorFromException(e);
 		}
 	}
 
@@ -380,13 +415,19 @@ public class ContentUri {
 			// Due to a design flaw, we can't use DocumentFile.renameTo().
 			// Instead we use the DocumentsContract API directly.
 			// See https://stackoverflow.com/questions/37168200/android-5-0-new-sd-card-access-api-documentfile-renameto-unsupportedoperation.
-			Uri newUri = DocumentsContract.renameDocument(activity.getContentResolver(), uri, newName);
-			// NOTE: we don't use the returned newUri for anything right now.
+			final ContentResolver resolver = activity.getContentResolver();
+			final String oldName = documentName(activity, uri);
+			final Uri newUri = DocumentsContract.renameDocument(resolver, uri, newName);
+			// A taken name gets "name (1)" instead, see createDocument. Put the old name back.
+			if (newUri != null && oldName != null && !newName.equals(documentName(activity, newUri))) {
+				Log.w(TAG, "contentUriRenameFileTo: '" + newName + "' is taken, renaming back to '" + oldName + "'");
+				DocumentsContract.renameDocument(resolver, newUri, oldName);
+				return STORAGE_ERROR_ALREADY_EXISTS;
+			}
 			return STORAGE_ERROR_SUCCESS;
 		} catch (Exception e) {
-			// TODO: More detailed exception processing.
 			Log.e(TAG, "contentUriRenameFile exception: " + e);
-			return STORAGE_ERROR_UNKNOWN;
+			return errorFromException(e);
 		}
 	}