Use anonymous shared memory on FreeBSD #178

valpackett · 2017-11-06T18:18:07Z

It works in Capsicum capability mode (process sandbox).

dlrobertson

👍

dlrobertson · 2017-11-06T18:45:20Z

src/platform/unix/mod.rs

 fn create_shmem(name: CString, length: usize) -> c_int {
    unsafe {
-        // NB: the FreeBSD man page for shm_unlink states that it requires
-        // write permissions, but testing shows that read-write is required.
        let fd = libc::shm_open(name.as_ptr(),
                                libc::O_CREAT | libc::O_RDWR | libc::O_EXCL,
                                0o600);


You should be able to decrease this in the linux specific shm_open. Last time I tested this, FreeBSD required read-write, but it was not required on linux.

Yeah, I thought about that. It's not just for Linux though, it's for everything else, and I'm not sure where write-only would be ok. Linux has memfd anyway…

memfd usage is optional on Linux, though. (IIRC that was decided because memfd is a pretty recent addition, so it would fail on older Linux versions...)

As for other systems, it's entirely unclear. Actually, I'm not sure we can even rely on it Linux: while it seems more logical to require only write permissions, neither the Linux nor the POSIX man pages seem to mention anything about this at all... I guess it's safer to stick with the broader permissions -- it's not like it has any real downsides here, aside from being confusing.

Either way, I don't think it's appropriate to remove the comment entirely. Even if the code is no longer used on FreeBSD, it doesn't make the comment wrong -- and we need to keep some explanation for why the code is this way...

@antrik well I moved the comment to the #[cfg(target_os="freebsd")] section, I did not remove it entirely :)

@myfreeweb I am aware that you moved it -- but since you didn't modify the original code, it's still not valid to just drop the comment entirely... Feel free to change the wording though, if you feel the original comment would be confusing in view of the changed situation.

dlrobertson · 2017-11-06T18:45:37Z

src/platform/unix/mod.rs

+    unsafe {
+        // NB: the FreeBSD man page for shm_unlink states that it requires
+        // write permissions, but testing shows that read-write is required.
+        let fd = libc::shm_open(1 as *mut i8, // SHM_ANON


👍 Could we add SHM_ANON to libc

Sure — rust-lang/libc#837

What's the status on this? Could you perhaps request a new release to get this functionality exposed?...

antrik · 2017-11-07T02:55:29Z

src/platform/unix/mod.rs

@@ -962,6 +960,20 @@ fn create_shmem(name: CString, length: usize) -> c_int {
    }
 }

+#[cfg(target_os="freebsd")]
+fn create_shmem(_name: CString, length: usize) -> c_int {


This is a bit dirty: since the name construction has side effects, I doubt the compiler is smart enough to realise it can be omitted altogether...

Though to be honest, I'm not at all sure whether it's really worthwhile to refactor the code in order to avoid this -- so I guess that's just me thinking out loud, rather than an actual request to change anything :-)

valpackett · 2017-12-07T10:57:18Z

What do you think about pulling this code into a separate tiny crate? This exact functionality is needed in a lot of things (mostly Wayland-related). Would you merge a patch that replaces this code with a dependency?

dlrobertson · 2017-12-08T01:27:18Z

I think that is reasonable. memfd_create is only available post linux 3.17 and has no glibc wrapper. It must be called via syscall, so as long as that it is only used when a feature is enabled, I think a separate crate would be reasonable.

valpackett · 2017-12-09T13:11:19Z

Done! https://github.com/myfreeweb/shmemfdrs & updated this PR to use it

dlrobertson

+1 for the new crate.

dlrobertson · 2017-12-09T15:12:02Z

src/lib.rs

-          target_os="linux"))]
-#[macro_use]
-extern crate syscall;
+#[cfg(all(not(feature = "force-inprocess"), any(target_os = "linux",


Related to myfreeweb/shmemfdrs#1. The old implementation had a default that was used if the target os was not linux (with memfd) or freebsd.

The issue is no longer valid, but this still does not allow e.g. openbsd to use the shmemfdrs crate

You mean the hardcoded any(target_os="linux", target_os="freebsd") in here? Yeah, I would also like to replace them with… all(unix, not(any(target_os="macos", target_os="ios"))) I guess?

Actually, I think you are right again here. I seem to have done the same thing with extern crate mio and this is the same as the cfg for mod unix, so there should probably be a separate issue to possibly modify the cfgs.

This is a bit confusing, since the implementation of the unix back-end should work with any Unix-like OS, but the conditional selecting the unix back-end (in https://github.com/servo/ipc-channel/blob/master/src/platform/mod.rs ) only works on specific platforms :-(

The only real change here as far as I can tell is that the shmemfdrs dependency requires introducing (yet another) redundant copy of the unix conditional.

(A long-standing issue on my personal ToDo-list is to try to factor out the platform conditionals in a central place, so everything else would only have to test for a custom platform_unix conditional, or something along these lines...)

Yeah. Arguably, it's not another copy, it's syscall's copy :D

+1 that it's a separate issue

@dlrobertson oh, I see you already figured that out :-)

@myfreeweb nah, syscall doesn't have an exact copy of the unix conditional, since that one is (and always was) Linux-specific, so it didn't have the "not generic enough" problem. But yeah, separate issue.

antrik · 2017-12-09T15:45:25Z

Looks good to me. I'm not sure though what Servo's policy is for introducing new external dependencies?

@myfreeweb out of curiosity: how and for what purpose does WR need to use this?

@dlrobertson that's a bit tangential: but did we ever discuss checking for memfd support at run-time?...

valpackett · 2017-12-09T15:48:16Z

WR? WebRender, on its own? Does it use ipc-channel at all?

Servo needs SHM_ANON to run sandboxed on FreeBSD: servo/servo#11625 (comment)

dlrobertson · 2017-12-09T16:01:36Z

@antrik I think we discussed it but I think I forgot to create an issue for it and therefore forgot to investigate further.

antrik · 2017-12-09T16:50:26Z

@myfreeweb whoops, I guess I misread "Wayland" as "Webrender"...

So I'm a bit confused now: are there other users for shmemfdrs outside of ipc-channel?

valpackett · 2017-12-09T16:52:28Z

No existing users as the crate was created today :) But potential users like Smithay/wayland-window#14

antrik · 2017-12-09T17:11:22Z

@myfreeweb going by the discussion there, it seems like they actually need a way to select the specific mechanism depending on client requests, rather than an automatic abstraction? So it doesn't seem like this crate will really help there...

The reason I'm bringing this up is because I am somewhat reluctant -- and from what I gathered, other Servo developers are too -- to introduce a new crate for code that is not actually likely to be used outside of Servo. That would just increase maintenance burden.

valpackett · 2017-12-09T17:20:27Z

The mechanism being file descriptors or not file descriptors — not different ways of creating file descriptors :)

antrik · 2017-12-09T18:25:42Z

@myfreeweb I was pretty sure it was actually about temporary file based descriptors vs. anonymous descriptors? But then again, I'm not really familiar with the Wayland protocol -- so I guess I might be misreading it...

valpackett · 2017-12-09T18:35:54Z

I made similar changes work in weston, so I know something :) Of course no one cares how a file descriptor is made.

Shared memory file descriptors are used to pass software-rendered buffers from the client to the compositor. The primary "other way" is passing GPU (EGL) buffers. And a compositor theoretically could support EGL only.

bors-servo · 2018-02-15T02:22:54Z

☔ The latest upstream changes (presumably #187) made this pull request unmergeable. Please resolve the merge conflicts.

(which adds support for FreeBSD SHM_ANON)

bors-servo · 2019-01-09T20:32:48Z

☔ The latest upstream changes (presumably #216) made this pull request unmergeable. Please resolve the merge conflicts.

valpackett mentioned this pull request Nov 6, 2017

Servo to compile and run on FreeBSD servo/servo#11625

Open

dlrobertson reviewed Nov 6, 2017

View reviewed changes

antrik reviewed Nov 7, 2017

View reviewed changes

valpackett mentioned this pull request Nov 26, 2017

Use memfd instead of temporary files Smithay/wayland-window#14

Open

valpackett force-pushed the freebsd-shm-anon branch from 3c75675 to 8258b7a Compare December 9, 2017 13:04

highfive added the S-awaiting-review label Dec 9, 2017

valpackett force-pushed the freebsd-shm-anon branch from 8258b7a to 1fa55f7 Compare December 9, 2017 13:07

dlrobertson reviewed Dec 9, 2017

View reviewed changes

highfive added the S-needs-rebase label Feb 15, 2018

Extract create_shmem into the shmemfdrs crate

29c0ed9

(which adds support for FreeBSD SHM_ANON)

valpackett force-pushed the freebsd-shm-anon branch from 1fa55f7 to 29c0ed9 Compare June 9, 2018 15:17

linkmauve mentioned this pull request Oct 1, 2018

Use extracted shmemfdrs crate #207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use anonymous shared memory on FreeBSD #178

Use anonymous shared memory on FreeBSD #178

valpackett commented Nov 6, 2017

dlrobertson left a comment

dlrobertson Nov 6, 2017

valpackett Nov 6, 2017

dlrobertson Nov 6, 2017

antrik Nov 7, 2017

valpackett Nov 7, 2017

antrik Nov 12, 2017

dlrobertson Nov 6, 2017 •

edited

Loading

valpackett Nov 6, 2017

antrik Nov 12, 2017

antrik Nov 7, 2017

valpackett commented Dec 7, 2017

dlrobertson commented Dec 8, 2017

valpackett commented Dec 9, 2017

dlrobertson left a comment

dlrobertson Dec 9, 2017

dlrobertson Dec 9, 2017

valpackett Dec 9, 2017

dlrobertson Dec 9, 2017

antrik Dec 9, 2017

valpackett Dec 9, 2017

antrik Dec 9, 2017

antrik Dec 9, 2017 •

edited

Loading

antrik commented Dec 9, 2017

valpackett commented Dec 9, 2017 •

edited

Loading

dlrobertson commented Dec 9, 2017

antrik commented Dec 9, 2017

valpackett commented Dec 9, 2017

antrik commented Dec 9, 2017

valpackett commented Dec 9, 2017

antrik commented Dec 9, 2017

valpackett commented Dec 9, 2017

bors-servo commented Feb 15, 2018

bors-servo commented Jan 9, 2019

Use anonymous shared memory on FreeBSD #178

Are you sure you want to change the base?

Use anonymous shared memory on FreeBSD #178

Conversation

valpackett commented Nov 6, 2017

dlrobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valpackett commented Dec 7, 2017

dlrobertson commented Dec 8, 2017

valpackett commented Dec 9, 2017

dlrobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antrik Dec 9, 2017 • edited Loading

Choose a reason for hiding this comment

antrik commented Dec 9, 2017

valpackett commented Dec 9, 2017 • edited Loading

dlrobertson commented Dec 9, 2017

antrik commented Dec 9, 2017

valpackett commented Dec 9, 2017

antrik commented Dec 9, 2017

valpackett commented Dec 9, 2017

antrik commented Dec 9, 2017

valpackett commented Dec 9, 2017

bors-servo commented Feb 15, 2018

bors-servo commented Jan 9, 2019

dlrobertson Nov 6, 2017 •

edited

Loading

antrik Dec 9, 2017 •

edited

Loading

valpackett commented Dec 9, 2017 •

edited

Loading