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

wayland: add extra sockets that are used by older toolkits (e.g. gtk3) #5660

Merged
merged 1 commit into from
Aug 17, 2018

Conversation

gerboland
Copy link
Contributor

@gerboland
Copy link
Contributor Author

I removed "wayland-shared-*" as I cannot find what was using it, and as the built-in apparmor profile doesn't have it either, concluded it was an error.

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

LGTM

@mvo5 mvo5 requested a review from jdstrand August 16, 2018 09:38
@mvo5
Copy link
Contributor

mvo5 commented Aug 16, 2018

Adding Jamie to look at this from the security POV

@gerboland
Copy link
Contributor Author

I forgot to fix the test, done now

@codecov-io
Copy link

codecov-io commented Aug 16, 2018

Codecov Report

Merging #5660 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5660      +/-   ##
==========================================
- Coverage   78.96%   78.96%   -0.01%     
==========================================
  Files         522      522              
  Lines       39711    39711              
==========================================
- Hits        31358    31356       -2     
- Misses       5807     5808       +1     
- Partials     2546     2547       +1
Impacted Files Coverage Δ
interfaces/builtin/wayland.go 100% <ø> (ø) ⬆️
overlord/hookstate/hookmgr.go 73.55% <0%> (-0.97%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f466e4...7c3b649. Read the comment docs.

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

The policy itself looks okay.

/run/user/[0-9]*/wayland-shared-* rw,
/run/user/[0-9]*/wayland-cursor-shared-* rw,
/run/user/[0-9]*/xwayland-shared-* rw,
/run/user/[0-9]*/{mesa,mutter,sdl,wayland-cursor,weston,xwayland}-shared-* rw,
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks okay though ideally it wayland would store those sockets in some private space since they are actually handled to clients over a IPC call that carries the file descriptor. Still, this is okay just annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These sockets/files are can also be created by clients to share data with the server - passed by FD as you rightly say. AFAICS it is an informal standard that $XDG_RUNTIME_DIR is the place for such things, and that's hardcoded in all the toolkits I've looked at.

owner /run/user/[0-9]*/###PLUG_SECURITY_TAGS###/wayland-shared-* rw,
owner /run/user/[0-9]*/###PLUG_SECURITY_TAGS###/wayland-cursor-shared-* rw,
owner /run/user/[0-9]*/###PLUG_SECURITY_TAGS###/xwayland-shared-* rw,
owner /run/user/[0-9]*/###PLUG_SECURITY_TAGS###/{mesa,mutter,sdl,wayland-cursor,weston,xwayland}-shared-* rw,
Copy link
Contributor

Choose a reason for hiding this comment

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

This matches the list above so OK

Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

+1. Thanks!

@mvo5 mvo5 merged commit 1806a59 into canonical:master Aug 17, 2018
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.

5 participants