Skip to content

Skip mode check on Android tests when extracting tar files #74183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

carlossanlop
Copy link
Contributor

Test fix on Android platforms.
Originally written by @tmds in this PR commit: daed79e
We discovered this failure in runtime-extra-platforms, which doesn't get executed often. Android is changing the mode and we cannot reliably compare it.
Will backport it to RC1 after merging.

@carlossanlop carlossanlop self-assigned this Aug 18, 2022
@carlossanlop carlossanlop changed the title Skip mode check on Android when extracting tar files Skip mode check on Android tests when extracting tar files Aug 18, 2022
@ghost
Copy link

ghost commented Aug 18, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Test fix on Android platforms.
Originally written by @tmds in this PR commit: daed79e
We discovered this failure in runtime-extra-platforms, which doesn't get executed often. Android is changing the mode and we cannot reliably compare it.
Will backport it to RC1 after merging.

Author: carlossanlop
Assignees: carlossanlop
Labels:

area-System.IO

Milestone: -

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a long term fix? ie., we have no hope of verifying perms on Android? (Presumably we care what they are..)

@carlossanlop
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlop
Copy link
Contributor Author

carlossanlop commented Aug 18, 2022

Found in the 7.0-rc1 -> 7 PR: #74045

@@ -409,7 +409,8 @@ protected static void AssertEntryModeFromFileSystemEquals(TarEntry entry, UnixFi

protected static void AssertFileModeEquals(string path, UnixFileMode mode)
{
if (!PlatformDetection.IsWindows)
if (!PlatformDetection.IsWindows &&
!PlatformDetection.IsAndroid) // Android may change the requested permissions.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any docs/links/etc or more information on exactly what/why Android is changing the requested permissions?

@carlossanlop
Copy link
Contributor Author

carlossanlop commented Aug 18, 2022

@danmoseley - Is this a long term fix? ie., we have no hope of verifying perms on Android? (Presumably we care what they are..)
@eerhardt - Do you have any docs/links/etc or more information on exactly what/why Android is changing the requested permissions?

I found these explanations:

https://android.stackexchange.com/a/208927
https://android.stackexchange.com/a/205494
https://android.stackexchange.com/a/205780
https://android.stackexchange.com/a/208928

They are written by the same individual, and the cited sources eventually took me to this:

https://source.android.com/docs/core/storage/config

Which explains that "the external storage is emulated and permissionless".

I found the first stackexchange explanation quite helpful:

On all operating systems based on Linux kernel - like Android is - it's possible to set permissions on files (including directories) provided that filesystem supports UNIX permissions (uid, gid, mode). Common examples of such filesystems are ext4 and f2fs.

However Android's internal (confusingly called external) storage which is accessible by installed apps at /sdcard, is not an actual but virtual / emulated filesystem exposing /data/media (which is a real filesystem) through sdcardfs. sdcardfs and its predecessor FUSE expose the emulated filesystem with a fixed set of uid, gid and mask (mode). So the commands chmod and chown have no impact, whether executed through CLI (adb shell or terminal emulator) or GUI (file explorer).

It's possible to change permission bits of file on underlying actual filesystem, but accessing /data/media requires root access because only UID/GID 1023 (aid_media_rw) has read access to the directory. And still the permissions will remain same when viewed from emulated view /sdcard.

So you can't change permissions of user created files on Android, with or without root.

I am making the assumption that our tests are running in the external storage, since we can't seem to be able to change mode.

@eerhardt
Copy link
Member

Wouldn't that mean any test that is manipulating the file mode should be failing on Android?

For example, this test:

[PlatformSpecific(TestPlatforms.AnyUnix & ~TestPlatforms.Browser)]
[Theory]
[MemberData(nameof(AllowedUnixFileModes))]
public void SetThenGet(UnixFileMode mode)
{
if (GetModeNeedsReadableFile)
{
// Ensure the file remains readable.
mode |= UnixFileMode.UserRead;
}
string path = CreateTestItem();
SetMode(path, mode);
Assert.Equal(mode, GetMode(path));
}

@danmoseley
Copy link
Member

@steveisok

@tmds
Copy link
Member

tmds commented Aug 19, 2022

I think this is umask in combination with setgid being added to preserve group ownership.

I'll improve the change in #74002.

@jozkee
Copy link
Member

jozkee commented Aug 19, 2022

New extra-platforms run on #74002 doesn't show errors related to SetUnixFileMode, System.Formats.Tar.Tests, nor Directory.CreateDirectory, I think we can close this PR.

@jozkee jozkee closed this Aug 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 19, 2022
@carlossanlop carlossanlop deleted the AndroidTarTest branch July 28, 2023 15:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants