lix-project/lix

Fork 54

Allow ipc-sysv* in the Darwin sandbox #623

New issue

Closed

opened 2025-01-12 20:23:06 +00:00 by al3xtjames · 5 comments

al3xtjames commented

2025-01-12 20:23:06 +00:00

The sandbox profile in CppNix was updated in https://github.com/NixOS/nix/pull/10878 to allow the shmget syscall, which is used by the postgresql tests.

Describe the solution you'd like

It would be nice if 372d5a441e could be backported to Lix:

From 06b2cc1984fd92b05a8f62ad6b3727c998463c57 Mon Sep 17 00:00:00 2001
From: Kirill Radzikhovskyy <kirillrdy@gmail.com>
Date: Mon, 10 Jun 2024 11:17:41 +1000
Subject: [PATCH] darwin: allow ipc-sysv* in sandbox

---
 lix/libstore/build/sandbox-defaults.sb | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lix/libstore/build/sandbox-defaults.sb b/lix/libstore/build/sandbox-defaults.sb
index 25ec11285..f4518ec7f 100644
--- a/lix/libstore/build/sandbox-defaults.sb
+++ b/lix/libstore/build/sandbox-defaults.sb
@@ -17,6 +17,9 @@ R""(
 ; Allow POSIX semaphores and shared memory.
 (allow ipc-posix*)
 
+; Allow SYSV semaphores and shared memory.
+(allow ipc-sysv*)
+
 ; Allow socket creation.
 (allow system-socket)
 
-- 
2.47.0

Describe alternatives you've considered

The postgresql derivation in nixpkgs could be updated to set sandboxProfile (though this would be redundant for CppNix):

diff --git a/pkgs/servers/sql/postgresql/generic.nix b/pkgs/servers/sql/postgresql/generic.nix
index 2effd687e8e4..98864c6b465c 100644
--- a/pkgs/servers/sql/postgresql/generic.nix
+++ b/pkgs/servers/sql/postgresql/generic.nix
@@ -392,6 +392,11 @@ let
         !(stdenv'.hostPlatform.isDarwin && olderThan "15") && !(stdenv'.hostPlatform.isStatic);
       installCheckTarget = "check-world";
 
+      sandboxProfile = ''
+        ; Allow SYSV semaphores and shared memory.
+        (allow ipc-sysv*)
+      '';
+
       passthru =
         let
           this = self.callPackage generic args;

Additional context

I ran into this while debugging a build failure (not specific to Lix) in nixpkgs: https://github.com/NixOS/nixpkgs/issues/371242

## Is your feature request related to a problem? Please describe. The sandbox profile in CppNix was updated in https://github.com/NixOS/nix/pull/10878 to allow the `shmget` syscall, which is used by the postgresql tests. ## Describe the solution you'd like It would be nice if https://github.com/NixOS/nix/commit/372d5a441e1a6fe61740609ac622f53dc77229a3 could be backported to Lix: ```patch From 06b2cc1984fd92b05a8f62ad6b3727c998463c57 Mon Sep 17 00:00:00 2001 From: Kirill Radzikhovskyy <kirillrdy@gmail.com> Date: Mon, 10 Jun 2024 11:17:41 +1000 Subject: [PATCH] darwin: allow ipc-sysv* in sandbox --- lix/libstore/build/sandbox-defaults.sb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lix/libstore/build/sandbox-defaults.sb b/lix/libstore/build/sandbox-defaults.sb index 25ec11285..f4518ec7f 100644 --- a/lix/libstore/build/sandbox-defaults.sb +++ b/lix/libstore/build/sandbox-defaults.sb @@ -17,6 +17,9 @@ R""( ; Allow POSIX semaphores and shared memory. (allow ipc-posix*) +; Allow SYSV semaphores and shared memory. +(allow ipc-sysv*) + ; Allow socket creation. (allow system-socket) -- 2.47.0 ``` ## Describe alternatives you've considered The postgresql derivation in nixpkgs could be updated to set `sandboxProfile` (though this would be redundant for CppNix): ```nix diff --git a/pkgs/servers/sql/postgresql/generic.nix b/pkgs/servers/sql/postgresql/generic.nix index 2effd687e8e4..98864c6b465c 100644 --- a/pkgs/servers/sql/postgresql/generic.nix +++ b/pkgs/servers/sql/postgresql/generic.nix @@ -392,6 +392,11 @@ let !(stdenv'.hostPlatform.isDarwin && olderThan "15") && !(stdenv'.hostPlatform.isStatic); installCheckTarget = "check-world"; + sandboxProfile = '' + ; Allow SYSV semaphores and shared memory. + (allow ipc-sysv*) + ''; + passthru = let this = self.callPackage generic args; ``` ## Additional context I ran into this while debugging a build failure (not specific to Lix) in nixpkgs: https://github.com/NixOS/nixpkgs/issues/371242

jade commented

2025-01-13 04:34:58 +00:00

Owner

I looked into why postgres was using APIs which are jank on every POSIX platform (POSIX shared memory is preferred on basically every other platform as well) and it seems that it is because they need it for some kind of synchronization purposes that cannot be avoided: 102de3be73/src/backend/port/sysv_shmem.c (L43-L49)

However, all of this together makes me think that postgres really should be the special case here, and we shouldn't allow derivations to do things that are 1. uncommon, 2. liable to be incompatible with e.g. app sandbox, 3. which allow arbitrary communication with the rest of the system, without the derivation specifying it needs them. However, I am not a macOS maintainer and I might be missing some details here.

I do know that sandboxProfile is only permitted when sandbox is relaxed in nix.conf, which is a non-ideal restriction for something which probably be stuck in there forever, but weh. idk.

@lilyball may know more/have other thoughts

I don't know if this should be trivially just bypassed in the sandbox: sysv IPC API is effectively deprecated on macOS and should probably not be used to begin with https://forums.developer.apple.com/forums/thread/719897 I looked into why postgres was using APIs which are jank on every POSIX platform (POSIX shared memory is preferred on basically every other platform as well) and it seems that it is because they need it for some kind of synchronization purposes that cannot be avoided: https://github.com/postgres/postgres/blob/102de3be73345a8de443355e055c10762b08cc4c/src/backend/port/sysv_shmem.c#L43-L49 However, all of this together makes me think that postgres really should be the special case here, and we shouldn't allow derivations to do things that are 1. uncommon, 2. liable to be incompatible with e.g. app sandbox, 3. which allow arbitrary communication with the rest of the system, without the derivation specifying it needs them. However, I am not a macOS maintainer and I might be missing some details here. I do know that `sandboxProfile` is only permitted when `sandbox` is `relaxed` in `nix.conf`, which is a non-ideal restriction for something which probably be stuck in there forever, but weh. idk. @lilyball may know more/have other thoughts

al3xtjames commented

2025-01-13 05:35:45 +00:00

Author

That makes sense. AFAICT sysv IPC was added to CppNix primarily to support running the postgres tests. The postgres tests were recently disabled so it's likely unnecessary (no other derivations need it, AFAIK).

That makes sense. AFAICT sysv IPC was added to CppNix primarily to support running the postgres tests. [The postgres tests were recently disabled](https://github.com/NixOS/nixpkgs/pull/371534) so it's likely unnecessary (no other derivations need it, AFAIK).

jade added the

OS/macOS

Area/store

labels

2025-01-23 00:20:43 +00:00

jade added the

Status

wontfix

label

2025-02-12 18:11:50 +00:00

jade commented

2025-02-12 18:12:14 +00:00

Owner

I think we can just close this wontfix then, since it doesn't cause any other known problems (I think?).

jade closed this issue

2025-02-12 18:12:14 +00:00

wolfgangwalther commented

2025-02-22 20:55:18 +00:00

The postgres tests were recently disabled so it's likely unnecessary (no other derivations need it, AFAIK).

In fact we were able to enable them in August thanks to the sandbox change in CppNix. We only disabled them recently again, because we hit #691 and didn't understand it fully, yet.

I'd really like to enable PostgreSQL tests again on Darwin in nixpkgs. It's not only the tests for PostgreSQL itself, but also a lot of packages depending on it and running some tests against it in their checkPhase. Currently, almost all of them are special cased for darwin - and then they disable all their tests.

This really creates a big gap of testing on darwin. I'm sure @ma27 agrees with me here: As the nixpkgs' postgres maintainers, we would like to provide first class support for MacOS as well.

I do know that sandboxProfile is only permitted when sandbox is relaxed in nix.conf, which is a non-ideal restriction for something which probably be stuck in there forever, but weh.

Yeah, not good. Additionally, the knowledge would have to be spread over all kinds of places in nixpkgs. Hard to maintain, really.

Of course, now that we know about #691, it makes sense to only do the backport if we can fix that issue as well. The status quo, for lix, is that it gives a hard error (permission denied), which is predictable. The status quo for CppNix is that it fails in very unpredictable ways, because of that "leak".

> [The postgres tests were recently disabled](https://github.com/NixOS/nixpkgs/pull/371534) so it's likely unnecessary (no other derivations need it, AFAIK). In fact we were able to *enable* them in August thanks to the sandbox change in CppNix. We only disabled them recently again, because we hit #691 and didn't understand it fully, yet. I'd really like to enable PostgreSQL tests again on Darwin in nixpkgs. It's not only the tests for PostgreSQL itself, but also a lot of packages depending on it and running some tests against it in their checkPhase. Currently, almost all of them are special cased for darwin - and then they disable **all** their tests. This really creates a big gap of testing on darwin. I'm sure @ma27 agrees with me here: As the nixpkgs' postgres maintainers, we would like to provide first class support for MacOS as well. > I do know that `sandboxProfile` is only permitted when `sandbox` is `relaxed` in `nix.conf`, which is a non-ideal restriction for something which probably be stuck in there forever, but weh. Yeah, not good. Additionally, the knowledge would have to be spread over all kinds of places in nixpkgs. Hard to maintain, really. --- Of course, now that we know about https://git.lix.systems/lix-project/lix/issues/691, it makes sense to only do the backport if we can fix that issue as well. The status quo, for lix, is that it gives a hard error (permission denied), which is predictable. The status quo for CppNix is that it fails in very unpredictable ways, because of that "leak".

wolfgangwalther commented

2025-02-22 20:57:00 +00:00

@wolfgangwalther wrote in #623 (comment):

The postgres tests were recently disabled so it's likely unnecessary (no other derivations need it, AFAIK).

In fact we were able to enable them in August thanks to the sandbox change in CppNix. We only disabled them recently again, because we hit #691 and didn't understand it fully, yet.

In fact, we only disabled the tests for x86_64-darwin, not for aarch64-darwin. So currently lix can not build nixpkgs' postgresql derivation on aarch64-darwin.

@wolfgangwalther wrote in https://git.lix.systems/lix-project/lix/issues/623#issuecomment-8909: > > [The postgres tests were recently disabled](https://github.com/NixOS/nixpkgs/pull/371534) so it's likely unnecessary (no other derivations need it, AFAIK). > > In fact we were able to _enable_ them in August thanks to the sandbox change in CppNix. We only disabled them recently again, because we hit #691 and didn't understand it fully, yet. In fact, we only disabled the tests for x86_64-darwin, not for aarch64-darwin. **So currently lix can not build nixpkgs' postgresql derivation on aarch64-darwin.**