NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.46k stars 13.66k forks source link

switch-to-configuration-ng fails to parse escaped newlines #342642

Open amarshall opened 3 days ago

amarshall commented 3 days ago

Describe the bug

See title. It also may fail to correctly parse comments within such blocks as well, but unsure.

Steps To Reproduce

  1. Create a systemd unit in config e.g.

    systemd.services.mytestsvc.ExecStart = ''
     foo \
     # ignored
     ; ignored
       bar \
       baz
    '';
  2. Try to switch configuration (though this might only happen on the second switch, trying to parse existing config?)
  3. Fails with

    Error: Failed to parse systemd unit file /etc/systemd/system/mytestsvc.service
    
    Caused by:
       0: Failed parse unit file as INI
       1: expecting "[Some('='), Some(':')]" but found EOF.

Expected behavior

Per man systemd.syntax, this is valid syntax:

Lines ending in a backslash are concatenated with the following line while reading and the backslash is replaced by a space character. … When a comment line or lines follow a line ending with a backslash, the comment block is ignored, so the continued line is concatenated with whatever follows the comment block.

The value of ExecStart should thus be parsed to foo bar baz (I may have gotten the spaces wrong there)

Screenshots

If applicable, add screenshots to help explain your problem.

Additional context

Patch for failing example test case (may not be perfect):

diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/src/src/main.rs b/pkgs/by-name/sw/switch-to-configuration-ng/src/src/main.rs
index 802b5e64f101..28a362ffdf0c 100644
--- a/pkgs/by-name/sw/switch-to-configuration-ng/src/src/main.rs
+++ b/pkgs/by-name/sw/switch-to-configuration-ng/src/src/main.rs
@@ -2141,4 +2141,35 @@ fn parse_systemd_ini() {
             );
         }
     }
+
+    #[test]
+    fn parse_systemd_ini_escaped_newline() {
+        // Ensure we don't attempt to unescape content in unit files.
+        // https://github.com/NixOS/nixpkgs/issues/315602
+        {
+            let mut unit_info = HashMap::new();
+
+            let test_unit = std::io::Cursor::new(
+                r#"[Unit]
+ExecStart=foo \
+# ignored
+bar \
+; ignored
+baz
+"#,
+            );
+            super::parse_systemd_ini(&mut unit_info, test_unit).unwrap();
+
+            assert_eq!(
+                unit_info
+                    .get("Unit")
+                    .unwrap()
+                    .get("ExecStart")
+                    .unwrap()
+                    .first()
+                    .unwrap(),
+                "foo  bar  baz"
+            );
+        }
+    }
 }

Notify maintainers

@jmbaur

also @emilazy due to authoring https://github.com/NixOS/nixpkgs/pull/339727

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

 - system: `"x86_64-linux"`
 - host os: `Linux 6.6.50, NixOS, 24.11 (Vicuna), 24.11.20240911.4f807e8`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.18.5`
 - nixpkgs: `/nix/store/2pw5bsq1r80lcvvh2wp89m142nln1zpl-source`

I was running dry-activate to a config using nixpkgs @ 99dc8785f6a0adac95f5e2ab05cc2e1bf666d172


Add a :+1: reaction to issues you find important.

jmbaur commented 2 days ago

@amarshall thanks for the report! I've looked a little into this so far and switch-to-configuration doesn't actually use that much of the unit file contents, which could be fine considering that systemd's DBus interface exposes almost all of their unit properties as DBus properties. However, since NixOS uses some custom unit properties (think X-StopIfChanged), these aren't exposed as DBus properties. So instead I've gone the route of adding support for multi-line values in the INI parser used for switch-to-configuration-ng, with the eventual goal of upstreaming this support to that crate. This isn't going to be something that's immediately ready though, I should have a PR for review within the coming days. For now, I would recommend opting out of the switch-to-configuration-ng implementation with the following config until it's fixed:

{
  system.switch.enableNg = false;
}
amarshall commented 2 days ago

The problem with INI in general is that there is no standard. The systemd syntax, after all, “is inspired by XDG Desktop Entry Specification .desktop files, which are in turn inspired by Microsoft Windows .ini files”.