Skip to content
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

gh-116622: Don't expose FICLONE ioctl on Android #122522

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Jul 31, 2024

Although this ioctl exists in the system headers, it's blocked by SELinux with a message like this:

type=1400 audit(0.0:18729): avc: denied { ioctl } for path=2F646174612F646174612F6F72672E707974686F6E2E746573746265642F63616368652F746573745F707974686F6E5F776F726B65725F36373532C3A62F40746573745F363735325F746D70C3A62F636F707941 dev="dm-39" ino=369303 ioctlcmd=0x9409 scontext=u:r:untrusted_app:s0:c225,c256,c512,c768 tcontext=u:object_r:app_data_file:s0:c225,c256,c512,c768 tclass=file permissive=0 app=org.python.testbed

On Python 3.14 this breaks the test for Path.copy, which was added in #119058:

======================================================================
ERROR: test_copytree_to_existing_directory_dirs_exist_ok (test.test_pathlib.test_pathlib.PosixPathTest.test_copytree_to_existing_directory_dirs_exist_ok)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/test/test_pathlib/test_pathlib_abc.py", line 1899, in test_copytree_to_existing_directory_dirs_exist_ok
    source.copytree(target, dirs_exist_ok=True)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/pathlib/_abc.py", line 870, in copytree
    on_error(err)
    ~~~~~~~~^^^^^
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/pathlib/_abc.py", line 848, in on_error
    raise err
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/pathlib/_abc.py", line 868, in copytree
    on_error(err)
    ~~~~~~~~^^^^^
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/pathlib/_abc.py", line 848, in on_error
    raise err
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/pathlib/_abc.py", line 864, in copytree
    source.copy(target_dir.joinpath(source.name),
    ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                follow_symlinks=follow_symlinks,
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                preserve_metadata=preserve_metadata)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/pathlib/_abc.py", line 827, in copy
    copyfileobj(source_f, target_f)
    ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/pathlib/_os.py", line 174, in copyfileobj
    raise err
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/pathlib/_os.py", line 164, in copyfileobj
    raise err
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/pathlib/_os.py", line 160, in copyfileobj
    clonefd(source_fd, target_fd)
    ~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/pathlib/_os.py", line 58, in clonefd
    fcntl.ioctl(target_fd, fcntl.FICLONE, source_fd)
    ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
PermissionError: [Errno 13] Permission denied: '/data/data/org.python.testbed/cache/test_python_worker_7506æ/@test_7506_tmpæ/dirC/fileC' -> '/data/data/org.python.testbed/cache/test_python_worker_7506æ/@test_7506_tmpæ/copyC/fileC'

On Python 3.13 I don't think Python ever uses this ioctl itself, but it's still worth backporting this PR for the benefit of user code.

Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

A question and a comment:

Q: Where is the FICLONE and FICLONERANGE constant coming from? Could this be managed as an autoconf check, rather than an explicit Android #ifdef?

Comment: This needs docs noting the feature exclusion on Android.

@bedevere-app
Copy link

bedevere-app bot commented Jul 31, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@mhsmith
Copy link
Member Author

mhsmith commented Jul 31, 2024

Where is the FICLONE and FICLONERANGE constant coming from? Could this be managed as an autoconf check, rather than an explicit Android #ifdef?

They're coming from the operating system headers. fcntlmodule.c has #ifdefs for each one, so they don't need autoconf checks.

This needs docs noting the feature exclusion on Android.

I think this is already covered by the statement in the fcntl docs that "The values used for cmd are operating system dependent". There is no list of the available values in the Python documentation.

Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Ok - those both make sense.

@freakboy3742 freakboy3742 enabled auto-merge (squash) August 1, 2024 00:02
@freakboy3742 freakboy3742 merged commit 06656e2 into python:main Aug 1, 2024
37 checks passed
@miss-islington-app
Copy link

Thanks @mhsmith for the PR, and @freakboy3742 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 1, 2024
…2522)

Don't expose `FICLONE` ioctl on Android

(cherry picked from commit 06656e2)

Co-authored-by: Malcolm Smith <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Aug 1, 2024

GH-122539 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Aug 1, 2024
freakboy3742 added a commit that referenced this pull request Aug 16, 2024
…#122539)

gh-116622: Don't expose `FICLONE` ioctl on Android (GH-122522)

Don't expose `FICLONE` ioctl on Android

(cherry picked from commit 06656e2)

Co-authored-by: Malcolm Smith <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
Don't expose `FICLONE` ioctl on Android

Co-authored-by: Russell Keith-Magee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants