SystemNative_CopyFile() call to fchmod() introduced in Mono 6.0 is not compatible with the external storage location on some Android OS versions and devices

[brendanzagaeski]

This is a continuation of xamarin/xamarin-android#3426, narrowed down to the problematic system call.

This pinpointed test case does not demonstrate a regression between Mono versions because it explicitly invokes the problematic system call itself. But the real user-facing scenario from xamarin/xamarin-android#3426 is a regression starting in Xamarin.Android 9.4 (mono/2019-02) because Xamarin.Android 9.3 (mono/2018-08) and earlier did not yet use SystemNative_CopyFile().

It looks like SystemNative_CopyFile() might need to add some special handling for the unusual way that permissions behave in the external storage directory on Android.

Steps to reproduce

1. Launch the attached test case in the Debug configuration on an Android 7.1 Nougat (API level 25) x86 or x86_64 Google APIs emulator.
2. Allow access to the device storage when prompted in the app.
3. Tap the envelope icon in the lower right corner of the screen.

Test case: AndroidExternalStorageFchmod.zip

The core of the test case is:

string temp = Path.Combine (Android.OS.Environment.ExternalStorageDirectory.AbsolutePath, "Temp");
Directory.CreateDirectory (temp);

string inFile = Path.Combine (temp, "in.txt");
string outFile = Path.Combine (temp, "out.txt");
File.WriteAllText (inFile, "text");
File.WriteAllText (outFile, "text");

int ret;
int errno;
Stat sourceStat;

int inFd = Syscall.open (inFile, OpenFlags.O_RDONLY);
int outFd = Syscall.open (outFile, OpenFlags.O_RDWR);
ret = Syscall.fstat (inFd, out sourceStat);
ret = Syscall.fchmod (outFd, sourceStat.st_mode & (FilePermissions.S_IRWXU | FilePermissions.S_IRWXG | FilePermissions.S_IRWXO));
errno = Marshal.GetLastWin32Error();

if (ret < 0 && errno == (int)Errno.EPERM)
{
	throw new UnauthorizedAccessException ();
}

The intention is to demonstrate the result of the fchmod() call in SystemNative_CopyFile():
https://github.com/mono/corefx/blob/cb2c46b3f2b0097ea4ccb71de296b7760eeda26e/src/Native/Unix/System.Native/pal_io.c#L1340

Problematic behavior on Android 7.1 Nougat (API level 25) x86 or x86_64 Google APIs emulator

The fchmod() call returns -1 with an errno of Mono.Unix.Native.Errno.EPERM, so the test case throws UnauthorizedAccessException.

After you run the test case, you also can optionally confirm the permissions restriction by hand using adb:

1. Open Tools > Android > Android Adb Command Prompt.

2. Run the following command:

   C:\> adb shell ls -l /storage/emulated/0/Temp/*.txt

   Result:

   -rw-rw---- 1 root sdcard_rw 4 2019-09-30 23:40 /storage/emulated/0/Temp/in.txt
   -rw-rw---- 1 root sdcard_rw 4 2019-09-30 23:40 /storage/emulated/0/Temp/out.txt
   

   (Note that the files are owned by root rather than the app itself.)

3. Run the following command:

   C:\> adb shell run-as com.companyname.androidapp1 chmod g+w /storage/emulated/0/Temp/out.txt

   Result:

   chmod: chmod '/storage/emulated/0/Temp/out.txt' to 100660: Operation not permitted
   

Non-problematic behavior on Android 9 Pie (API level 28) x86 Google APIs emulator

The fchmod() call returns 0, so the test case does not throw any exceptions.

As expected, a similar adb command also completes successfully:

C:\> adb shell run-as com.companyname.androidapp1 chmod g+w /storage/emulated/0/Temp/out.txt

Result: Success (no output).

On which platforms did you notice this

[x] Android 7.1 Nougat (API level 25) x86 and x86_64 Google APIs emulators
[x] Android 4.1 Jelly Bean (API level 16) armeabi-v7a LG Optimus L9

I could not reproduce the permissions problem on:

• Android 9 Pie (API level 28) x86 Google APIs emulator
• Android 9 Pie (API level 28) arm64-v8a Google Pixel 3
• Android 7.1 Nougat (API level 25) armeabi-v7a Motorola Moto E4
• Android 6.0 Marshmallow (API level 23) armeabi-v7a Motorola Moto G3

Version Used:

Xamarin.Android 9.4.0.51 (mono/2019-02@e6f5369c2d24aa2de511a52c8a58956c3283c4e4)
Xamarin.Android 10.0.0.43 (mono/2019-06@7af64d1ebe9)

[marek-safar]

/cc @steveisok

[marek-safar]

Did you try set ?

<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"></uses-permission>

[marek-safar]

Also, what is the filesystem on the storage?

[brendanzagaeski]

Jonathan Pryor identified an interesting point about this issue. Part of the documented behavior of System.IO.File.Copy() is:

The attributes of the original file are retained in the copied file.

The documentation does not specifically mention whether the copied file also retains the permissions, mode, or access control list of the original file, but it seems like a potentially valid design choice for File.Copy() to set the destination file to have the same read, write, and execute permission mode bits as the original file.

If the team agrees that File.Copy() and File.Move() should fail in cases where they cannot set the permission bits on the destination file, then it might be acceptable to close this issue as an intentional breaking change.

The recommendation for Android developers would then be to use another option like File.ReadAllBytes() plus File.WriteAllBytes() to copy or move files to destinations on external storage.

Possible changes to SystemNative_CopyFile()

To brainstorm about possible changes to SystemNative_CopyFile() that might be interesting to consider:

1. Call fstat_() on outFd, and skip the call to fchmod() if the resulting st_mode field for outFd already has the same (S_IRWXU | S_IRWXG | S_IRWXO) bits as the st_mode field obtained for inFd. That is, something roughly like:

   while ((ret = fstat_(outFd, &destStat)) < 0 && errno == EINTR);
   if (ret != 0)
   {
       return -1;
   }
   if ((destStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO)) != (sourceStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO)))
       while ((ret = fchmod(outFd, sourceStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO))) < 0 && errno == EINTR);

   This would allow File.Copy() to copy from one file on external storage to another file on external storage without error. It would still fail for some other scenarios that previously worked, such as copying a file from the app's private data directory into external storage when the source file has the default permissions as set by File.WriteAllText() or File.Create().

2. OR do almost the same thing, but only check if the st_mode field for outFd has at least the same bits as the st_mode field for inFd That is, something roughly like:

   while ((ret = fstat_(outFd, &destStat)) < 0 && errno == EINTR);
   if (ret != 0)
   {
       return -1;
   }
   if (((destStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO)) & (sourceStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO))) != (sourceStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO)))
       while ((ret = fchmod(outFd, sourceStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO))) < 0 && errno == EINTR);

   This would allow File.Copy() to succeed in a few more cases, including copying a file from the app's private data directory into external storage when the source file has the default permissions as set by File.WriteAllText() or File.Create().

   On the other hand, I have a feeling this approach might introduce more potential confusion than it would be worth. It would mean that if a source file happened to have additional permissions that weren't present by default on the destination file, then an exception could still happen.

Answers about WRITE_EXTERNAL_STORAGE and external storage filesystem

Did you try set ?

<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"></uses-permission>

Yes, the test case includes the android.permission.WRITE_EXTERNAL_STORAGE permission.

what is the filesystem on the storage?

Good question. It looks like the filesystem type that causes trouble might consistently be fuse.

Devices where fchmod() returns -1 due to Errno.EPERM

Android 7.1 Nougat (API level 25) x86 Google APIs emulator:

/dev/fuse on /storage/emulated type fuse (rw,nosuid,nodev,noexec,noatime,user_id=1023,group_id=1023,default_permissions,allow_other)

Android 4.1 Jelly Bean (API level 16) armeabi-v7a LG Optimus L9

/dev/fuse /storage/sdcard0 fuse rw,nosuid,nodev,relatime,user_id=1023,group_id=1023,default_permissions,allow_other 0 0

Devices where fchmod() succeeds

Android 9 Pie (API level 29) arm64-v8a Google Pixel 3
and
Android 9 Pie (API level 29) x86 Google APIs emulator:

/data/media on /storage/emulated type sdcardfs (rw,nosuid,nodev,noexec,noatime,fsuid=1023,fsgid=1023,gid=1015,multiuser,mask=6,derive_gid,default_normal)

Android 7.1 Nougat (API level 25) armeabi-v7a Motorola Moto E4:

/data/media on /storage/emulated type esdfs (rw,nosuid,nodev,noexec,relatime,upper=0:1015:660:771,derive=multi,noconfine)

Android 6.0 Marshmallow (API level 23) armeabi-v7a Motorola Moto G3:

/mnt/expand/e8d0fad8-9f6f-48d5-ac89-f874f8927531/media /storage/emulated esdfs rw,nosuid,nodev,noexec,relatime,upper=0:1015:660:771,derive=multi,noconfine 0 0

[brendanzagaeski]

Cross-reference: The earlier issue #16573 reported for Linux raises essentially the same question about the intended behavior of File.Copy().

[brendanzagaeski]

Another interesting detail about this issue is that currently the behavior of SystemNative_CopyFile() is different on macOS because it uses fcopyfile() to copy both content and metadata, and fcopyfile() returns 0 even if it cannot set the permission mode bits on the destination file. So the scenario from #16573 does not throw any exceptions on macOS.

In contrast, for the same scenario of overwriting a file owned by another user, the command line commands cp --preserve=mode on Linux and cp -p on macOS both complete with non-zero exit status and print error messages to stderr, but like File.Copy() on Linux (and Android), they do successfully copy the content to the destination file despite the errors.

[tobiasschulz]

If the team agrees that File.Copy() and File.Move() should fail in cases where they cannot set the permission bits on the destination file, then it might be acceptable to close this issue as an intentional breaking change.

The recommendation for Android developers would then be to use another option like File.ReadAllBytes() plus File.WriteAllBytes() to copy or move files to destinations on external storage.

That would be crazy. You can't possibly be considering this. Not only would that make 3rd party libraries that may call File.Copy unusable on android. Developers are gonna release apps that work in their devices and they will likely fail in production, just because of the sheer variety of Android devices.

This permission behavior happens on maybe 20% of devices we've encountered so far. Which is a very substantial part of our customer base, but we don't necessarily encounter it during testing.

It should not matter how File.Copy was originally defined. This is basically an Android "security feature" preventing you to set file permissions, and it needs to be gracefully handled by Xamarin, not by the user.

I have to justify using Xamarin now, because my co-workers say it is always buggy. Which sickens me, because I really like to use C#. But you seriously cannot do breaking changes to basic things like File.Copy after it worked for years. You also cannot mindlessly migrate .NET Core code into mono like that. People actually use your stuff in production, guys!!!

[marek-safar]

My feeling is that due to historical reasons we should look at this API as one which tries to copy as much as it can but if it cannot copy/set the permissions it should not fail. We can also propose a new API or a new overload which can be more Linux/Unix oriented.

[baulig]

What I really don't like about that SystemNative_CopyFile() implementation is that it reports an error although the actual copy operation did succeed - so you'd end up having the target file, but with wrong permissions. But would Android even allow you to delete it again?

Also - unrelated to this particular issue - that function should never have been rolled out into production without breaking down those loops. They're not thread-abort / runtime shutdown safe.

[tobiasschulz]

Android doesn't allow setting permissions on that virtual filesystem. It's not needed, and not supported. If mono just didn't run fchmod, everything would work exactly as it should.

Also, user ids on android are for distinguishing apps, not users; and group ids are for distinguishing certain kinds of permissions that apps can have.

The most responsible way to handle this on android would probably be to put "#ifndef ANDROID" around the fchmod, or to ignore it when it fails.

[grendello]

I agree with @marek-safar's idea of a Linux/Unix specific overload - for desktop and server apps - and I second @tobiasschulz's sentiment that Android should ignore (although not remove completely) any errors coming from fchmod. The copy should be "best effort" and as long as the data is copied correctly, the rest is good to have but not completely necessary.

[divil5000]

We are hitting this issue in production because we made the huge mistake of upgrading our Xamarin developer tools to their latest version. We should have known better.

The most unbelievable part of this is that this breaking change was ever allowed to ship. You shipped a change where a File.Copy operation would fail on external storage, and judging by our telemetry on a significant proportion of Android devices. It is unforgivable, and so is the delay in addressing your mistake. It used to be the case that Microsoft shipped higher quality code than this into their stable releases.

[steveisok]

@divil5000 (and everyone else at large) I do apologize for the issue. We are actively working on it and will try to get the fix out to you as soon as we can.

[derekcroprecords]

Is this fixed yet? This problem was originally reported August 1st.
It's not like its in some rarely used code. File.Copy and File.Move are important.
We've used Xamarin for almost 7 years and I'm a the point where I'm actually considering having to rewrite everything in some other language/platform because Microsoft obviously doesn't care about this product if File.Copy can be broken for 12+ weeks.

[tobiasschulz]

They don't actually seem to get it. :(

All they need to do is ignore the return value of the chmod function.

[steveisok]

We are working to get it fixed in time for the d16-4 preview 3 or 4 Xamarin release. More when I know it.

[derekcroprecords]

So what date is d16-4 going to be ready?
I don't think you guys are understanding the amount of problems that bugs like this cause.
If basic math was broken how fast would you be able to get a fix out?
This is the kind of bug where no-one should be going home until its fixed.
A release process where it takes weeks or months to fix important bugs is just unacceptable.

[brendanzagaeski]

I can chime in with one bit of information about release timing for Visual Studio 2019 version 16.4. Apologies in advance that I don't have a specific date. I think the best information available at the moment might be that historically the time between Visual Studio 2019 version 16.x release versions has been about 1.5 or 2 months, so if that pattern holds, the Visual Studio 2019 version 16.4 release will be available in November.

[tobiasschulz]

Okay, but it has not been fixed in mono yet, has it? Which mono version is gonna be in 16.4?

[brendanzagaeski]

If that question is in reply to my comment, then I can clarify that my comment about the release timeline of Visual Studio 2019 version 16.4 is only relevant in the context of this issue if a fix does indeed get included in the Mono version used by the Xamarin.Android SDK for Visual Studio 2019 version 16.4 Preview 3 or Preview 4.

For now, steveisok's comment about the work toward a fix is the more important status information.

[baulig]

"simply ignoring the return value" is, of course, completely unacceptable and not going to happen.

We are going to hard fail on all platforms if the fchmod() call fails - with the exception that on Android, it will be permissible to continue if the target file's permissions are at least as restrictive as the source file's.

PR will be up shortly, but I don't have any Android devices, so I will need some help testing this.

[tobiasschulz]

You don't seem to understand what permissions on android mean in that case. It's a fuse file system for external storage. Apps are not supposed to set permissions on external storage. All files are always gonna have the permissions that the virtual file system wants the app to see.

[tobiasschulz]

So if you copy a file from internal storage (where you can set you own permissions) to external storage (where you cannot), and the permissions are less restrictive on the external storage because the fuse virtual file system enforces that, the File.Copy call is still gonna fail, it that what you are saying?

Because that is what this looks like to me, which means that File.Copy would still be complete unusable. And that is what's completely unacceptable.

Example:

Source file:
-rw------- 1 appname foobar 4 2019-09-30 23:40 /data/data/abc.appname/files/foo.txt

Target file:
-rw-rw---- 1 root sdcard_rw 4 2019-09-30 23:40 /storage/emulated/0/bar.txt

The target file's permissions are less restrictive than the source file's. So this is still gonna fail?

Considering that you cannot set the permissions on external storage, it would not be possible to copy a file without group permissions to external storage, where the group can always read/write?

[divil5000]

The correct solution to this would be to revert the code to how it was before you catastrophically broke it, letting down all your customers who were relying on it.

[baulig]

So if you copy a file from internal storage (where you can set you own permissions) to external storage (where you cannot), and the permissions are less restrictive on the external storage because the fuse virtual file system enforces that, the File.Copy call is still gonna fail, it that what you are saying?
Because that is what this looks like to me, which means that File.Copy would still be complete unusable. And that is what's completely unacceptable.
Example:
Source file:
-rw------- 1 appname foobar 4 2019-09-30 23:40 /data/data/abc.appname/files/foo.txt
Target file:
-rw-rw---- 1 root sdcard_rw 4 2019-09-30 23:40 /storage/emulated/0/bar.txt
The target file's permissions are less restrictive than the source file's. So this is still gonna fail?
Considering that you cannot set the permissions on external storage, it would not be possible to copy a file without group permissions to external storage, where the group can always read/write?

Thank you so much for this very valuable feedback. I edited my PR accordingly:
mono/corefx#364

[Taloth]

@baulig Other platforms (for example mounted exfat/ntfs/cloud filesystems) don't allow modifying permissions either. So it would likely fail there too and not just android. In mono 6.0 it would fail after the write, now the problem simply got moved to the top of CopyFile and you end up with a 0 length file.
The exact same problem still persists for #16573 (it was discussed here, but not addressed)

The fundamental issue is the assumption that chmod should succeed here. In mono < 6.0, File.Copy didn't even copy permissions. Now it does, and fails if it can't.
This is essentially an upstream corefx issue ever since dotnet core 1.0, but it effectively broke CopyFile (and MoveFile cross-filesystems) for mono 6+.
Please realize the gravity of this. Anyone using certain filesystem mounts like, cloud, fat, ntfs, etc. (The likely only reason why you might not have heard about these issues more frequently is that the end user doesn't know the underlying problem and just downgrades to mono 5.x)

The workaround (using the managed CopyTo) is pure insanity. Every app would have to write it's own replacement for File.Copy and File.Move, they won't be able to use the faster kernel-level operations that SystemNative_CopyFile implements.

Maybe worth noting that on Windows, copying a file in Windows Explorer doesn't copy the file ACLs, neither does a net462 & netcore app.

File.Copy should not preserve permissions. File.Move should preserve permissions (even if it has to fall back to a Copy on cross-filesystem), but should not fail doing so if it can't.
Any advanced situation where the user wants the permissions to be copied requires a new api.

[habskilla]

Is this still broke for Linux systems?