From d5d4e37d1cd40804e1a89c14309f96e335ce99d0 Mon Sep 17 00:00:00 2001 From: Hammed Abass Date: Sun, 12 Jul 2026 04:40:10 +0200 Subject: [PATCH] fix(android): make default receive path actually writable Android emulated storage rejects hard-link commit even when canWrite() reports success. Fall back to exclusive rename for publish, probe real writes during folder validation, and label the default app downloads dir clearly so it is not confused with shared system Downloads. --- crates/vnidrop/CORE_FLOW.md | 13 +- crates/vnidrop/src/filesystem.rs | 128 +++++++++++++++++- crates/vnidrop/src/tests/filesystem.rs | 19 +++ .../app/core/FileSystemService.android.kt | 27 +++- 4 files changed, 174 insertions(+), 13 deletions(-) diff --git a/crates/vnidrop/CORE_FLOW.md b/crates/vnidrop/CORE_FLOW.md index 96cf5bd..0d24374 100644 --- a/crates/vnidrop/CORE_FLOW.md +++ b/crates/vnidrop/CORE_FLOW.md @@ -59,9 +59,16 @@ bytes through Kotlin memory. - Persisted shares are restored only when their root collection is complete and readable. Missing or corrupt roots fail closed and emit a recovery event. - Receive destinations use a no-overwrite policy. Rust writes a uniquely named - temporary file in the destination directory, syncs it, and atomically - publishes it with a no-clobber hard link. Failure or cancellation removes the - temporary file. Stale VniDrop temporary files are cleaned on later writes. + temporary file in the destination directory, syncs it, and publishes it with + a no-clobber hard link when the filesystem supports it. On platforms that + reject hard links (notably Android emulated external storage), publication + falls back to an exclusive rename (`renameat2(RENAME_NOREPLACE)` / + `renamex_np(RENAME_EXCL)`). Failure or cancellation removes the temporary + file. Stale VniDrop temporary files are cleaned on later writes. +- Android defaults to the app-specific external Downloads directory + (`getExternalFilesDir`), which is always writable by the process. Shared + system folders require a SAF tree URI via the folder picker; those receives + stream through `ReceiveOutputSink` instead of raw filesystem paths. - Foreign output sinks receive exactly one terminal callback after a successful `start_file`: `finish_file` or `abort_file`. diff --git a/crates/vnidrop/src/filesystem.rs b/crates/vnidrop/src/filesystem.rs index 44d0056..0d3c9a6 100644 --- a/crates/vnidrop/src/filesystem.rs +++ b/crates/vnidrop/src/filesystem.rs @@ -101,18 +101,134 @@ impl AtomicOutputFile { } pub(crate) fn commit(mut self) -> Result<()> { - // Creating a hard link is an atomic no-clobber publication on the same - // filesystem. It fails if another writer created the destination. - std::fs::hard_link(&self.temporary, &self.target) + publish_temp_as_final(&self.temporary, &self.target) .with_context(|| format!("failed to commit {}", self.target.display()))?; self.committed = true; - if let Err(error) = std::fs::remove_file(&self.temporary) { - tracing::warn!(%error, path = %self.temporary.display(), "failed to remove committed temporary file"); - } Ok(()) } } +/// Publish a fully written temporary file as the final destination without +/// clobbering an existing peer. +/// +/// Prefer a same-directory hard link (atomic no-clobber on most Unix volumes). +/// Android's emulated external storage and some FUSE mounts reject hard links +/// even when ordinary create/write/rename work, so fall back to an exclusive +/// rename when the link is unsupported. +fn publish_temp_as_final(temporary: &Path, target: &Path) -> io::Result<()> { + match std::fs::hard_link(temporary, target) { + Ok(()) => { + if let Err(error) = std::fs::remove_file(temporary) { + tracing::warn!( + %error, + path = %temporary.display(), + "failed to remove committed temporary file" + ); + } + Ok(()) + } + Err(error) if is_hard_link_unsupported(&error) => rename_no_replace(temporary, target), + Err(error) => Err(error), + } +} + +fn is_hard_link_unsupported(error: &io::Error) -> bool { + match error.raw_os_error() { + Some(code) + if code == libc::EPERM + || code == libc::EACCES + || code == libc::EOPNOTSUPP + || code == libc::ENOTSUP + || code == libc::EXDEV + || code == libc::EINVAL + || code == libc::ENOSYS => + { + true + } + _ => matches!( + error.kind(), + io::ErrorKind::Unsupported | io::ErrorKind::PermissionDenied + ), + } +} + +fn rename_no_replace(from: &Path, to: &Path) -> io::Result<()> { + #[cfg(any(target_os = "linux", target_os = "android"))] + { + match renameat2_noreplace(from, to) { + Ok(()) => return Ok(()), + Err(error) + if error.raw_os_error() == Some(libc::ENOSYS) + || error.raw_os_error() == Some(libc::EINVAL) => {} + Err(error) => return Err(error), + } + } + + #[cfg(target_os = "macos")] + { + match renamex_np_excl(from, to) { + Ok(()) => return Ok(()), + Err(error) if error.raw_os_error() == Some(libc::ENOTSUP) => {} + Err(error) => return Err(error), + } + } + + // Last resort: refuse an existing destination, then rename. There is a + // small race versus concurrent writers, but this path only runs when the + // platform lacks both hard links and exclusive rename. + if std::fs::symlink_metadata(to).is_ok() { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!("destination already exists: {}", to.display()), + )); + } + std::fs::rename(from, to) +} + +#[cfg(any(target_os = "linux", target_os = "android"))] +fn renameat2_noreplace(from: &Path, to: &Path) -> io::Result<()> { + use std::ffi::CString; + use std::os::unix::ffi::OsStrExt; + + let from_c = CString::new(from.as_os_str().as_bytes()) + .map_err(|error| io::Error::new(io::ErrorKind::InvalidInput, error))?; + let to_c = CString::new(to.as_os_str().as_bytes()) + .map_err(|error| io::Error::new(io::ErrorKind::InvalidInput, error))?; + // Android defines RENAME_NOREPLACE as c_int while renameat2 takes c_uint. + let flags = libc::RENAME_NOREPLACE as libc::c_uint; + let rc = unsafe { + libc::renameat2( + libc::AT_FDCWD, + from_c.as_ptr(), + libc::AT_FDCWD, + to_c.as_ptr(), + flags, + ) + }; + if rc == 0 { + Ok(()) + } else { + Err(io::Error::last_os_error()) + } +} + +#[cfg(target_os = "macos")] +fn renamex_np_excl(from: &Path, to: &Path) -> io::Result<()> { + use std::ffi::CString; + use std::os::unix::ffi::OsStrExt; + + let from_c = CString::new(from.as_os_str().as_bytes()) + .map_err(|error| io::Error::new(io::ErrorKind::InvalidInput, error))?; + let to_c = CString::new(to.as_os_str().as_bytes()) + .map_err(|error| io::Error::new(io::ErrorKind::InvalidInput, error))?; + let rc = unsafe { libc::renamex_np(from_c.as_ptr(), to_c.as_ptr(), libc::RENAME_EXCL) }; + if rc == 0 { + Ok(()) + } else { + Err(io::Error::last_os_error()) + } +} + pub(crate) fn cleanup_stale_temporary_files( directory: &Path, minimum_age: Duration, diff --git a/crates/vnidrop/src/tests/filesystem.rs b/crates/vnidrop/src/tests/filesystem.rs index 38c9937..93f4b5c 100644 --- a/crates/vnidrop/src/tests/filesystem.rs +++ b/crates/vnidrop/src/tests/filesystem.rs @@ -117,6 +117,25 @@ fn atomic_output_commits_without_overwriting() { ); } +#[test] +fn atomic_output_commit_does_not_clobber_existing_peer() { + let output = tempfile::tempdir().unwrap(); + let target = output.path().join("peer.txt"); + std::fs::write(&target, b"original").unwrap(); + + let (pending, mut file) = AtomicOutputFile::create(output.path(), "other.txt").unwrap(); + std::io::Write::write_all(&mut file, b"new").unwrap(); + drop(file); + pending.commit().unwrap(); + + // Existing peer must stay intact while a different file commits. + assert_eq!(std::fs::read(&target).unwrap(), b"original"); + assert_eq!( + std::fs::read(output.path().join("other.txt")).unwrap(), + b"new" + ); +} + #[test] fn dropped_atomic_output_removes_partial_file() { let output = tempfile::tempdir().unwrap(); diff --git a/shared/src/androidMain/kotlin/com/vnidrop/app/core/FileSystemService.android.kt b/shared/src/androidMain/kotlin/com/vnidrop/app/core/FileSystemService.android.kt index 5f3d8ee..5818c7a 100644 --- a/shared/src/androidMain/kotlin/com/vnidrop/app/core/FileSystemService.android.kt +++ b/shared/src/androidMain/kotlin/com/vnidrop/app/core/FileSystemService.android.kt @@ -22,14 +22,17 @@ private class AndroidFileSystemService( private val context: Context, ) : FileSystemService { override fun defaultReceiveFolder(): ReceiveFolder { + // App-specific external storage is always writable without SAF or + // legacy storage permissions. It is NOT the shared system Downloads + // gallery — that still requires "Choose folder" (tree URI). val path = context .getExternalFilesDir(Environment.DIRECTORY_DOWNLOADS) ?.absolutePath - ?: (System.getProperty("java.io.tmpdir") ?: "/data/local/tmp/vnidrop-receive") + ?: context.filesDir.resolve("Downloads").apply { mkdirs() }.absolutePath return ReceiveFolder( kind = ReceiveFolderKind.FileSystemPath, value = path, - displayName = "Downloads", + displayName = "App downloads", ) } @@ -64,11 +67,27 @@ private class AndroidFileSystemService( } } + /** + * Probe a real create/write/delete instead of [java.io.File.canWrite]. + * + * Scoped storage often reports public directories as writable even when + * the process cannot create files there. A probe matches what receive needs. + */ private fun validatePath(path: String): FolderAccessStatus = runCatching { val directory = java.io.File(path) - if (!directory.exists()) directory.mkdirs() - if (directory.isDirectory && directory.canWrite()) FolderAccessStatus.Writable else FolderAccessStatus.Unavailable + if (!directory.exists() && !directory.mkdirs()) { + return FolderAccessStatus.Unavailable + } + if (!directory.isDirectory) return FolderAccessStatus.Unavailable + val probe = java.io.File(directory, ".vnidrop-write-test-${UUID.randomUUID()}") + try { + probe.outputStream().use { stream -> stream.write(1) } + if (!probe.exists()) return FolderAccessStatus.Unavailable + FolderAccessStatus.Writable + } finally { + probe.delete() + } }.getOrDefault(FolderAccessStatus.Unavailable) private fun validateTreeUri(value: String): FolderAccessStatus {