Skip to content

bpo-28684: asyncio tests now handle PermissionError raised when bindi…#4503

Merged
xdegaye merged 6 commits intopython:masterfrom
xdegaye:bpo-28684
Nov 24, 2017
Merged

bpo-28684: asyncio tests now handle PermissionError raised when bindi…#4503
xdegaye merged 6 commits intopython:masterfrom
xdegaye:bpo-28684

Conversation

@xdegaye
Copy link
Copy Markdown
Contributor

@xdegaye xdegaye commented Nov 22, 2017 •
edited by bedevere-bot
Loading

@xdegaye xdegaye added needs backport to 3.6 tests Tests in the Lib/test dir labels Nov 22, 2017
@xdegaye xdegaye requested a review from 1st1 November 22, 2017 17:00
1st1
1st1 requested changes Nov 22, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

I would prefer to use @unittest.skipIf(android) approach instead. PermissionError can occur on a normal Linux machine as a result of some regression in asyncio. I don't want such cases to slip through our CI just because it sometimes happens on Android.

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Nov 22, 2017

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.

@xdegaye
Copy link
Copy Markdown
Contributor Author

xdegaye commented Nov 23, 2017

@1st1 I understand your concern. Instead of @unittest.skipIf(android) I think it would be better to use a new decorator @support.skip_unless_bind_unix_socket to widen the scope of the changes:

  • The root user on Android does not raise PermissionError, so it should be possible to run the asyncio test suite in that case without skipping those tests.
  • Android runs now SElinux in enforcing mode and one of its policies restricts the access to unix sockets [1], so the problem is not Android specific and may impact other linux platforms depending on the way they are configured. As a confirmation of the fact that SElinux is involved, the remote command adb logcat prints this kind of AVC SElinux record whenever one of the asyncio tests fails with PermissionError:
11-23 15:36:11.859  2919  2919 W python  : type=1400 audit(0.0:20): avc: denied { create }
for name="tmpug8hwp0r" scontext=u:r:shell:s0 tcontext=u:object_r:shell_data_file:s0
tclass=sock_file permissive=0

[1] An interesting academic paper The Misuse of Android Unix Domain Sockets and Security Implications.

@1st1
Copy link
Copy Markdown
Member

1st1 commented Nov 23, 2017

SGTM, let's go for @support.skip_unless_bind_unix_socket

@xdegaye
Use instead a new skip_unless_bind_unix_socket() decorator
b5073f5 
The test.support.skip_unless_bind_unix_socket() decorator is used to
skip asyncio tests that fail because the platform lacks a functional
bind() function for unix domain sockets (as it is the case for non root
users on the recent Android versions that run now SELinux in enforcing
mode).
@xdegaye
Merge remote-tracking branch 'origin/master' into bpo-28684
e376db6
@xdegaye
The decorator must be applied last
62e26d4
@xdegaye
Only one decorator is needed
b58c5ab
1st1
1st1 reviewed Nov 24, 2017
View reviewed changes
Comment thread Lib/test/support/__init__.py Outdated
with socket.socket(socket.AF_UNIX) as sock:
try:
sock.bind(path)
_bind_nix_socket_error = ''
Copy link
Copy Markdown
Member

@1st1 1st1 Nov 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

_bind_nix_socket_error = False

Copy link
Copy Markdown
Contributor Author

@xdegaye xdegaye Nov 24, 2017

Choose a reason for hiding this comment

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

Yes, much better.

@1st1
Copy link
Copy Markdown
Member

1st1 commented Nov 24, 2017

LGTM

@xdegaye
Copy link
Copy Markdown
Contributor Author

xdegaye commented Nov 24, 2017

I am not sure that these changes need to be backported to 3.6.

@xdegaye xdegaye merged commit 0f86cd3 into python:master Nov 24, 2017
@xdegaye xdegaye deleted the bpo-28684 branch November 24, 2017 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st1 1st1 1st1 approved these changes

Assignees

No one assigned

Labels

tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xdegaye @bedevere-bot @1st1 @the-knights-who-say-ni