Make escapeSystemdPath implement the correct systemd escaping algorithm #177273

dali99 · 2022-06-11T10:38:38Z

Description of changes

There's a regression in regards to systemd mount units on 22.05:
Since #152372 we validate unit names but that validation isn't the same as what escapeSystemdPath does, notably it doesn't replace ( ) in my case
filesSystems uses escapeSystemdPath to convert the mountpoint to the unit name.

I've added a few functions to help convert the paths to the proper C escapes implementing the algorithm described in man systemd.unit But this is my first time working or changing anything significant in lib, so I'd be really happy for pointers on how to do this better utilizing functions we maybe already have?

I tested this on my configuration (cherry-picked to 22.05) and it seems fine... but this is a basically-8 year old function I'm changing which is pretty scary...

I also only did ASCII special chars, but without something like a builtins function for the UTF-8 conversion stuff I don't know if it's possible to generalize

Things done

ajs124 · 2022-06-12T10:47:24Z

Just commenting so this isn't only documented on matrix. I also tried to fix this recently, in #176115.

So far, we've figured out that this PR here properly translates characters, while mine handles the / special case and deduplicates consecutive /s.

I don't really have any preference over which PR should be merged into which one or how else to proceed, as long as we have an implementation that's as complete as possible, in the end.

nixos/lib/utils.nix

Mindavi · 2022-06-19T19:46:57Z

Can we add tests to make the behavior more clear and to document use cases? Also can help fixing any issues we find after getting this in, by improving the tests then.

dali99 · 2022-06-23T20:53:17Z

@Mindavi We don't seem to have any testing infrastructure for nixos/lib So I've only added tests for the new string functions (and only positive ones at that)

I feel like the escapeSystemdPath is complicated enough to warrant some testing but I'm not sure if this PR is the right place to add tests to nixos/lib

tadfisher · 2022-06-26T01:12:44Z

An issue should be opened in nix to provide a built-in for string formatting. So much of this would be a simple printf in C, even codepoint conversion.

symphorien · 2022-07-03T13:34:14Z

lib/strings.nix

@@ -305,6 +330,18 @@ rec {
  */
  escape = list: replaceChars list (map (c: "\\${c}") list);

+  /* Escape occurence of the element of `list` in `string` by
+     converting to it's ASCII value and prefixing it with \\x.


mention that it only works with printable ascii characters. For non printable ones, I wonder if it is correct to map the caracter 7 to \x7 instead of \x07.

I've now documented that it only works for printable characters, You're quite right about the non-printable ones as well, but I think we would need a nix builtin to deal with those anyways

nixos/lib/utils.nix

symphorien · 2022-09-03T15:48:07Z

@GrahamcOfBorg test systemd

symphorien · 2022-09-03T15:48:48Z

(note that github does not always send notifications on force-push, so when you address a review, please post a full-fledged comment)

symphorien

this breaks nixosTest.systemd (it looks "currently building" but the test vm is just stuck failing to boot)
problem is: the vm uses the autoformat feature, but the unit running mkfs does not run, I don't know why.

dedupes extranous /'s in filepaths Co-authored-by: Daniel Olsen <daniel.olsen99@gmail.com>

…ping names in systemd units Co-authored-by: ajs124 <git@ajs124.de>

dali99 · 2022-10-20T18:31:43Z

@GrahamcOfBorg test systemd

symphorien · 2022-10-20T19:15:39Z

thank you!

zowoq · 2022-10-22T00:48:57Z

nix-build -A nixosTests.simple --max-jobs 0 --eval-store auto --store ssh-ng://**** --system x86_64-linux
error: invalid regular expression ''

       at /Users/zowoq/src/nixpkgs/lib/strings.nix:540:51:

          539|       s = builtins.unsafeDiscardStringContext _s;
          540|       splits = builtins.filter builtins.isString (builtins.split (escapeRegex sep) s);
             |                                                   ^
          541|     in
(use '--show-trace' to show detailed location information)

Only seems to be a problem when running nixosTests remotely? Works again after reverting 3251123.

symphorien · 2022-10-22T06:34:36Z

If this happens only when evaluating on Darwin but not on linux it's probably a discrepancy between the glicxx and libcxx implem of std::regex in nix. I don't have a darwin system to investigate further, could you do it as you seem to have both kinds of systems, @zowoq ? 22 oct. 2022 02:49:17 zowoq ***@***.***>:

…

*nix-build -A nixosTests.simple --max-jobs 0 --eval-store auto --store ssh-ng://**** --system x86_64-linux error: invalid regular expression '' at /Users/zowoq/src/nixpkgs/lib/strings.nix:540:51: 539| s = builtins.unsafeDiscardStringContext _s; 540| splits = builtins.filter builtins.isString (builtins.split (escapeRegex sep) s); | ^ 541| in (use '--show-trace' to show detailed location information) * Only seems to be a problem when running nixosTests remotely? Works again after reverting 3251123[3251123]. — Reply to this email directly, view it on GitHub[#177273 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ADADGA5TE4MX7KLM2FEUGJDWEM2YJANCNFSM5YP4G32Q]. You are receiving this because you modified the open/close state.[Image de pistage][https://github.com/notifications/beacon/ADADGA3IR2CHNXZM7D3FT43WEM2YJA5CNFSM5YP4G322YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOJS7KMMA.gif]

zowoq · 2022-10-22T22:06:42Z

Yeah, only seems to be darwin->linux. If it is a nix bug I doubt it will be quick or simple to fix. I'd like to revert this PR until it is resolved.

dali99 · 2022-10-22T22:35:44Z

I'd be okay with that if we also reverted #152372 I don't want to maintain a nixpkgs channel with this patch for this coming release as well

symphorien · 2022-10-23T08:43:29Z

So the code in this PR does not involve regex at all except with the hard coded regex "/" and "" which are valid, could you report the error with a stacktrace and possibly with --debugger so that we know the value of the faulty regex?

symphorien · 2022-10-23T08:45:05Z

(besides this is a bugfix in nixos code that is only faulty when evaluating code on mac, which is more niche than the use cases it fixes imo, so I'd rather fix this than revert it).

zowoq · 2022-10-23T09:01:45Z

Can you suggest another workaround apart from reverting this locally on every branch that I'm working on?

I don't see that it matters if we revert it for a bit so we aren't introducing new breakage.

dali99 · 2022-10-23T09:34:49Z

This fixes a (at the time new) breakage on actual NixOS systems which IMO probably takes priority over evaluating nixos tests from darwin. I believe one can just ssh to a Linux box and run tests there in the mean time.

This exposing something broken in nix on darwin hopefully incentives someone who has access and cares about darwin to investigate and follow up the bug.

zowoq · 2022-10-23T09:57:18Z

This fixes a (at the time new) breakage on actual NixOS systems which IMO probably takes priority over evaluating nixos tests from darwin.

It is old code that has been broken for months, there certainly didn't seem to be any urgency in merging this PR. I don't see the problem with reverting it for a bit.

I believe one can just ssh to a Linux box and run tests there in the mean time.

Doesn't seem to be that simple as would need to sync code as well?

This exposing something broken in nix on darwin hopefully incentives someone who has access and cares about darwin to investigate and follow up the bug.

Yes, I'll try to do that. Not having my current workflows broken would mean I have more time to try to fix it?

presumably due to using libc++'s regex lib instead of libstdc++ on linux Fixes NixOS#177273 (comment)

symphorien · 2022-10-23T13:29:24Z

@zowoq: can you try #197374 ?

symphorien · 2022-10-23T13:43:32Z

The discrepancy is problematic so I opened NixOS/nix#7208

presumably due to using libc++'s regex lib instead of libstdc++ on linux Fixes #177273 (comment)

zowoq · 2022-10-24T02:25:44Z

Seems this is a recurring problem.

NixOS/nix#1537 (comment)
Empty regular expressions are not allowed according to the POSIX extended regex grammar

presumably due to using libc++'s regex lib instead of libstdc++ on linux Fixes NixOS/nixpkgs#177273 (comment)

github-actions bot added the 6.topic: nixos label Jun 11, 2022

dali99 force-pushed the escape-systemd branch from 3ac5905 to 049276e Compare June 11, 2022 10:45

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Jun 11, 2022

dali99 force-pushed the escape-systemd branch 3 times, most recently from 71dbed9 to dc8223a Compare June 12, 2022 12:43

dali99 requested a review from ajs124 June 12, 2022 12:50

dali99 changed the title ~~escape systemd paths maybe~~ Make escapeSystemdPath implement the correct systemd escaping algorithm Jun 12, 2022

dali99 marked this pull request as ready for review June 12, 2022 13:31

dali99 requested review from edolstra, nbp and infinisil as code owners June 12, 2022 13:31

dali99 commented Jun 12, 2022

View reviewed changes

nixos/lib/utils.nix Show resolved Hide resolved

dali99 force-pushed the escape-systemd branch from dc8223a to 800f8d4 Compare June 12, 2022 13:40

dali99 force-pushed the escape-systemd branch from 7715549 to 4f008be Compare June 23, 2022 20:50

ajs124 mentioned this pull request Jun 24, 2022

utils.escapeSystemdPath: actually try to implement the specification #176115

Closed

13 tasks

dali99 mentioned this pull request Jun 26, 2022

Builtin for formatted strings NixOS/nix#6725

Open

symphorien reviewed Jul 3, 2022

View reviewed changes

dali99 force-pushed the escape-systemd branch 2 times, most recently from eab9341 to 83ac459 Compare August 19, 2022 01:34

dali99 requested a review from symphorien September 3, 2022 15:05

symphorien requested changes Sep 3, 2022

View reviewed changes

lib.strings: Add function to convert character to number

a08741f

dali99 requested a review from symphorien October 20, 2022 18:10

dali99 and others added 4 commits October 20, 2022 20:12

lib.strings: Add function to do C-style escaping

4c420ee

lib.strings: Add normalizePath

4284ac9

dedupes extranous /'s in filepaths Co-authored-by: Daniel Olsen <daniel.olsen99@gmail.com>

nixos/lib.escapeSystemdPath: Implement the correct algorithm for esca…

3251123

…ping names in systemd units Co-authored-by: ajs124 <git@ajs124.de>

lib/tests/misc: Add tests for charToInt, escapeC, and normalizePath

23c1754

dali99 force-pushed the escape-systemd branch from d20b62c to 23c1754 Compare October 20, 2022 18:12

symphorien merged commit 91a3819 into NixOS:master Oct 20, 2022

symphorien added a commit to symphorien/nixpkgs that referenced this pull request Oct 23, 2022

lib.normalizePath: the empty string is not a valid regex on darwin

3648b33

presumably due to using libc++'s regex lib instead of libstdc++ on linux Fixes NixOS#177273 (comment)

symphorien mentioned this pull request Oct 23, 2022

lib.normalizePath: the empty string is not a valid regex on darwin #197374

Merged

13 tasks

symphorien mentioned this pull request Oct 23, 2022

the empty string is only a valid regex when built with libstdc++ and not with libc++ NixOS/nix#7208

Open

zowoq pushed a commit that referenced this pull request Oct 24, 2022

lib.normalizePath: the empty string is not a valid regex on darwin

956e35d

presumably due to using libc++'s regex lib instead of libstdc++ on linux Fixes #177273 (comment)

github-actions bot pushed a commit to arcnmx/nixpkgs-lib that referenced this pull request Oct 25, 2022

lib.normalizePath: the empty string is not a valid regex on darwin

7334b6d

presumably due to using libc++'s regex lib instead of libstdc++ on linux Fixes NixOS/nixpkgs#177273 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make escapeSystemdPath implement the correct systemd escaping algorithm #177273

Make escapeSystemdPath implement the correct systemd escaping algorithm #177273

dali99 commented Jun 11, 2022 •

edited

ajs124 commented Jun 12, 2022

Mindavi commented Jun 19, 2022

dali99 commented Jun 23, 2022

tadfisher commented Jun 26, 2022

symphorien Jul 3, 2022

dali99 Oct 20, 2022

symphorien commented Sep 3, 2022

symphorien commented Sep 3, 2022

symphorien left a comment

dali99 commented Oct 20, 2022

symphorien commented Oct 20, 2022

zowoq commented Oct 22, 2022

symphorien commented Oct 22, 2022 via email

zowoq commented Oct 22, 2022

dali99 commented Oct 22, 2022

symphorien commented Oct 23, 2022

symphorien commented Oct 23, 2022

zowoq commented Oct 23, 2022

dali99 commented Oct 23, 2022

zowoq commented Oct 23, 2022

symphorien commented Oct 23, 2022

symphorien commented Oct 23, 2022

zowoq commented Oct 24, 2022

Make escapeSystemdPath implement the correct systemd escaping algorithm #177273

Make escapeSystemdPath implement the correct systemd escaping algorithm #177273

Conversation

dali99 commented Jun 11, 2022 • edited

Description of changes

Things done

ajs124 commented Jun 12, 2022

Mindavi commented Jun 19, 2022

dali99 commented Jun 23, 2022

tadfisher commented Jun 26, 2022

symphorien Jul 3, 2022

Choose a reason for hiding this comment

dali99 Oct 20, 2022

Choose a reason for hiding this comment

symphorien commented Sep 3, 2022

symphorien commented Sep 3, 2022

symphorien left a comment

Choose a reason for hiding this comment

dali99 commented Oct 20, 2022

symphorien commented Oct 20, 2022

zowoq commented Oct 22, 2022

symphorien commented Oct 22, 2022 via email

zowoq commented Oct 22, 2022

dali99 commented Oct 22, 2022

symphorien commented Oct 23, 2022

symphorien commented Oct 23, 2022

zowoq commented Oct 23, 2022

dali99 commented Oct 23, 2022

zowoq commented Oct 23, 2022

symphorien commented Oct 23, 2022

symphorien commented Oct 23, 2022

zowoq commented Oct 24, 2022

dali99 commented Jun 11, 2022 •

edited