SAF Create() omits O_RDWR and returns a non-writable file

[SoongVilda]

I was going through the SAF filesystem implementation and noticed that safFilesystem.Create() currently calls:

return sfs.OpenFile(name, os.O_CREATE|os.O_TRUNC, 0)

Neither os.O_WRONLY nor os.O_RDWR is included. Since os.O_RDONLY is zero, this effectively requests read-only access.

safNode.OpenFile() uses those flags for two separate purposes:

1. To choose the Android file descriptor mode.
2. To set canWrite on the returned safFile.

For Android's built-in external storage provider, the missing write flag selects mode "r". O_TRUNC then appends "t", producing "rt". Android's ParcelFileDescriptor.parseMode() accepts only "r", "w", "wt", "wa", "rw", and "rwt", so "rt" is invalid.

For other SAF providers, the underlying descriptor is opened as "rwt", but canWrite is still false because it is calculated only from O_WRONLY and O_RDWR.

Consequently, the built-in provider path fails while opening the descriptor, and the generic provider path returns a safFile whose Write(), WriteAt(), and Truncate() methods return os.ErrInvalid.

This also differs from Syncthing's BasicFilesystem.Create(), which opens the file with O_RDWR|O_CREATE|O_TRUNC.

The smallest fix appears to be:

return sfs.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0)

A regression test could call Create() using mock document URIs that exercise both the built-in external storage and generic-provider paths, verify that "rwt" is requested, and write a few bytes through the returned file.

I have not reproduced the failure on-device, but the implementation itself appears deterministically broken. Syncthing does call Filesystem.Create() through osutil.copyFileContents(), although I have not determined how readily that path becomes user-visible for SAF folders.

This is present in BasicSync 3.7 / current master at commit 5b159a7.

[chenxiaolong]

Thanks, this is fixed in #215.

This bug didn't actually break any functionality. It just sometimes made certain operations less efficient. The bug is only hit in 2 scenarios:

• Syncthing tries to rename a file across filesystems of different types. This is unreachable when using SAF because BasicSync's custom filesystem scheme makes it impossible for a single shared folder to cross the boundary of a SAF tree.
• When file versioning is enabled and a remote device renames a file, osutil.copyFileContents() is used in the happy path for renaming the file locally. However, if that fails, it still falls back to copy-and-delete.

For Android's built-in external storage provider, the missing write flag selects mode "r". O_TRUNC then appends "t", producing "rt". Android's ParcelFileDescriptor.parseMode() accepts only "r", "w", "wt", "wa", "rw", and "rwt", so "rt" is invalid.

It looks like Android only started enforcing this in Android 17. It used to just map each character in the mode to one of the O_* flags. https://android.googlesource.com/platform/frameworks/base/+/5b5468ef7ae91858b1fe485ade787e4c16058264%5E%21/

Older Android versions accept rt as valid and it ends up failing in a similar way as with !isAlwaysSeekable() SAF providers.

[SoongVilda]

I didn’t expect this to be fixed so quickly! I only realized afterward that this stricter behavior is new in Android 17.

Unfortunately, my only Android device is a Samsung Galaxy S25 Ultra running stable Android 16, so I couldn’t test it directly. I was relying on the logical conclusion from comparing the BasicSync code with the new Android 17 behavior.

Anyway, thank you for the quick fix and the detailed explanation!