diff mbox series

[dunfell,3/5] systemd-systemctl: fix instance template WantedBy symlink construction

Message ID f5ebbb87943bc583049909f4216ca1d73a794261.1686751371.git.steve@sakoman.com
State New, archived
Headers show
Series [dunfell,1/5] libwebp: Fix CVE-2023-1999 | expand

Commit Message

Steve Sakoman June 14, 2023, 2:04 p.m. UTC 
From: Martin Siegumfeldt <mns@gomspace.com>

Fix issue of the below instance template systemd service dependency

[Install]
WantedBy=svc-wants@%i.service

creating the symlink (instance "a" example)

/etc/systemd/system/svc-wants@%i.service.wants/svc-wanted-by@a.service

which should be

/etc/systemd/system/svc-wants@a.service.wants/svc-wanted-by@a.service

as implemented by this change.

The functionality appears regressed just after "thud" baseline when the
logic was refactored from shell script into python (commit
925e30cb104ece7bfa48b78144e758a46dc9ec3f)

(From OE-Core rev: 308397f0bb3d6f3d4e9ec2c6a10823184049c9b5)

Signed-off-by: Martin Siegumfeldt <mns@gomspace.com>
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Signed-off-by: Steve Sakoman <steve@sakoman.com>
(cherry picked from commit 372b29c8ad270d4d430c26a4e614976c7029afaf)
Signed-off-by: Steve Sakoman <steve@sakoman.com>
---
 .../systemd/systemd-systemctl/systemctl             | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Peter Kjellerstedt June 15, 2023, 8:40 p.m. UTC | #1 
> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-
> core@lists.openembedded.org> On Behalf Of Steve Sakoman
> Sent: den 14 juni 2023 16:05
> To: openembedded-core@lists.openembedded.org
> Subject: [OE-core][dunfell 3/5] systemd-systemctl: fix instance template
> WantedBy symlink construction
> 
> From: Martin Siegumfeldt <mns@gomspace.com>
> 
> Fix issue of the below instance template systemd service dependency
> 
> [Install]
> WantedBy=svc-wants@%i.service
> 
> creating the symlink (instance "a" example)
> 
> /etc/systemd/system/svc-wants@%i.service.wants/svc-wanted-by@a.service
> 
> which should be
> 
> /etc/systemd/system/svc-wants@a.service.wants/svc-wanted-by@a.service
> 
> as implemented by this change.
> 
> The functionality appears regressed just after "thud" baseline when the
> logic was refactored from shell script into python (commit
> 925e30cb104ece7bfa48b78144e758a46dc9ec3f)
> 
> (From OE-Core rev: 308397f0bb3d6f3d4e9ec2c6a10823184049c9b5)
> 
> Signed-off-by: Martin Siegumfeldt <mns@gomspace.com>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> Signed-off-by: Steve Sakoman <steve@sakoman.com>
> (cherry picked from commit 372b29c8ad270d4d430c26a4e614976c7029afaf)
> Signed-off-by: Steve Sakoman <steve@sakoman.com>
> ---
>  .../systemd/systemd-systemctl/systemctl             | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/meta/recipes-core/systemd/systemd-systemctl/systemctl
> b/meta/recipes-core/systemd/systemd-systemctl/systemctl
> index 6aa2e20465..577c373181 100755
> --- a/meta/recipes-core/systemd/systemd-systemctl/systemctl
> +++ b/meta/recipes-core/systemd/systemd-systemctl/systemctl
> @@ -182,12 +182,19 @@ class SystemdUnit():
> 
>          raise SystemdUnitNotFoundError(self.root, unit)
> 
> -    def _process_deps(self, config, service, location, prop, dirstem):
> +    def _process_deps(self, config, service, location, prop, dirstem, instance):
>          systemdir = self.root / SYSCONFDIR / "systemd" / "system"
> 
>          target = ROOT / location.relative_to(self.root)
>          try:
>              for dependent in config.get('Install', prop):
> +                # determine whether or not dependent is a template with an actual
> +                # instance (i.e. a '@%i')
> +                dependent_is_template = re.match(r"[^@]+@(?P<instance>[^\.]*)\.", dependent)
> +                if dependent_is_template:
> +                    # if so, replace with the actual instance to achieve
> +                    # svc-wants@a.service.wants/svc-wanted-by@a.service
> +                    dependent = re.sub(dependent_is_template.group('instance'), instance, dependent, 1)
>                  wants = systemdir / "{}.{}".format(dependent, dirstem) / service
>                  add_link(wants, target)
> 
> @@ -227,8 +234,8 @@ class SystemdUnit():
>          else:
>              service = self.unit
> 
> -        self._process_deps(config, service, path, 'WantedBy', 'wants')
> -        self._process_deps(config, service, path, 'RequiredBy', 'requires')
> +        self._process_deps(config, service, path, 'WantedBy', 'wants', instance)
> +        self._process_deps(config, service, path, 'RequiredBy', 'requires', instance)
> 
>          try:
>              for also in config.get('Install', 'Also'):
> --
> 2.34.1

You might want to be careful with this one. It broke systemctl for us when it 
hit Mickledore and we have had to backport the version from Langdale for now.

The problem is that it does not handle non-instanced units depending on 
an instanced unit. In our case we have a unit foo.service that contains 
a RequiredBy=bar@0.service, which causes an error like this:

Traceback (most recent call last):
  File ".../recipe-sysroot-native/usr/bin/systemctl", line 366, in <module>
    main()
  File ".../recipe-sysroot-native/usr/bin/systemctl", line 352, in main
    SystemdUnit(root, service).enable()
  File ".../recipe-sysroot-native/usr/bin/systemctl", line 250, in enable
    self._process_deps(config, service, path, 'WantedBy', 'wants', instance)
  File ".../recipe-sysroot-native/usr/bin/systemctl", line 210, in _process_deps
    dependent = re.sub(dependent_is_template.group('instance'), instance, dependent, 1)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py", line 185, in sub
    return _compile(pattern, flags).sub(repl, string, count)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py", line 317, in _subx
    template = _compile_repl(template, pattern)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py", line 308, in _compile_repl
    return _parser.parse_template(repl, pattern)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../recipe-sysroot-native/usr/lib/python3.11/re/_parser.py", line 1000, in parse_template
    s = Tokenizer(source)
        ^^^^^^^^^^^^^^^^^
  File ".../recipe-sysroot-native/usr/lib/python3.11/re/_parser.py", line 226, in __init__
    string = str(string, 'latin1')
             ^^^^^^^^^^^^^^^^^^^^^
TypeError: decoding to str: need a bytes-like object, NoneType found

There are probably other cases too when different kinds of units are 
involved (at least as far as I could gather from the systemd manual).

We have an internal issue to fix this, but I am going on vacation on 
Monday, so I will unfortunately not have any time to look at it until 
after summer.

//Peter
Richard Purdie June 15, 2023, 8:54 p.m. UTC | #2 
On Thu, 2023-06-15 at 20:40 +0000, Peter Kjellerstedt wrote:
> > -----Original Message-----
> > From: openembedded-core@lists.openembedded.org <openembedded-
> > core@lists.openembedded.org> On Behalf Of Steve Sakoman
> > Sent: den 14 juni 2023 16:05
> > To: openembedded-core@lists.openembedded.org
> > Subject: [OE-core][dunfell 3/5] systemd-systemctl: fix instance
> > template
> > WantedBy symlink construction
> > 
> > From: Martin Siegumfeldt <mns@gomspace.com>
> > 
> > Fix issue of the below instance template systemd service dependency
> > 
> > [Install]
> > WantedBy=svc-wants@%i.service
> > 
> > creating the symlink (instance "a" example)
> > 
> > /etc/systemd/system/svc-
> > wants@%i.service.wants/svc-wanted-by@a.service
> > 
> > which should be
> > 
> > /etc/systemd/system/svc-wants@a.service.wants
> > /svc-wanted-by@a.service
> > 
> > as implemented by this change.
> > 
> > The functionality appears regressed just after "thud" baseline when
> > the
> > logic was refactored from shell script into python (commit
> > 925e30cb104ece7bfa48b78144e758a46dc9ec3f)
> > 
> > (From OE-Core rev: 308397f0bb3d6f3d4e9ec2c6a10823184049c9b5)
> > 
> > Signed-off-by: Martin Siegumfeldt <mns@gomspace.com>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > Signed-off-by: Steve Sakoman <steve@sakoman.com>
> > (cherry picked from commit
> > 372b29c8ad270d4d430c26a4e614976c7029afaf)
> > Signed-off-by: Steve Sakoman <steve@sakoman.com>
> > ---
> >  .../systemd/systemd-systemctl/systemctl             | 13
> > ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > b/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > index 6aa2e20465..577c373181 100755
> > --- a/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > +++ b/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > @@ -182,12 +182,19 @@ class SystemdUnit():
> > 
> >          raise SystemdUnitNotFoundError(self.root, unit)
> > 
> > -    def _process_deps(self, config, service, location, prop,
> > dirstem):
> > +    def _process_deps(self, config, service, location, prop,
> > dirstem, instance):
> >          systemdir = self.root / SYSCONFDIR / "systemd" / "system"
> > 
> >          target = ROOT / location.relative_to(self.root)
> >          try:
> >              for dependent in config.get('Install', prop):
> > +                # determine whether or not dependent is a template
> > with an actual
> > +                # instance (i.e. a '@%i')
> > +                dependent_is_template =
> > re.match(r"[^@]+@(?P<instance>[^\.]*)\.", dependent)
> > +                if dependent_is_template:
> > +                    # if so, replace with the actual instance to
> > achieve
> > +                    #
> > svc-wants@a.service.wants/svc-wanted-by@a.service
> > +                    dependent =
> > re.sub(dependent_is_template.group('instance'), instance,
> > dependent, 1)
> >                  wants = systemdir / "{}.{}".format(dependent,
> > dirstem) / service
> >                  add_link(wants, target)
> > 
> > @@ -227,8 +234,8 @@ class SystemdUnit():
> >          else:
> >              service = self.unit
> > 
> > -        self._process_deps(config, service, path, 'WantedBy',
> > 'wants')
> > -        self._process_deps(config, service, path, 'RequiredBy',
> > 'requires')
> > +        self._process_deps(config, service, path, 'WantedBy',
> > 'wants', instance)
> > +        self._process_deps(config, service, path, 'RequiredBy',
> > 'requires', instance)
> > 
> >          try:
> >              for also in config.get('Install', 'Also'):
> > --
> > 2.34.1
> 
> You might want to be careful with this one. It broke systemctl for us
> when it 
> hit Mickledore and we have had to backport the version from Langdale
> for now.
> 
> The problem is that it does not handle non-instanced units depending
> on 
> an instanced unit. In our case we have a unit foo.service that
> contains 
> a RequiredBy=bar@0.service, which causes an error like this:
> 
> Traceback (most recent call last):
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 366, in
> <module>
>     main()
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 352, in
> main
>     SystemdUnit(root, service).enable()
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 250, in
> enable
>     self._process_deps(config, service, path, 'WantedBy', 'wants',
> instance)
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 210, in
> _process_deps
>     dependent = re.sub(dependent_is_template.group('instance'),
> instance, dependent, 1)
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py",
> line 185, in sub
>     return _compile(pattern, flags).sub(repl, string, count)
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py",
> line 317, in _subx
>     template = _compile_repl(template, pattern)
>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py",
> line 308, in _compile_repl
>     return _parser.parse_template(repl, pattern)
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/_parser.py",
> line 1000, in parse_template
>     s = Tokenizer(source)
>         ^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/_parser.py",
> line 226, in __init__
>     string = str(string, 'latin1')
>              ^^^^^^^^^^^^^^^^^^^^^
> TypeError: decoding to str: need a bytes-like object, NoneType found
> 
> There are probably other cases too when different kinds of units are 
> involved (at least as far as I could gather from the systemd manual).
> 
> We have an internal issue to fix this, but I am going on vacation on 
> Monday, so I will unfortunately not have any time to look at it until
> after summer.

There is a patch in master recently which may help?

Cheers,

Richard
Steve Sakoman June 15, 2023, 9:31 p.m. UTC | #3 
On Thu, Jun 15, 2023 at 10:41 AM Peter Kjellerstedt
<peter.kjellerstedt@axis.com> wrote:
>
> > -----Original Message-----
> > From: openembedded-core@lists.openembedded.org <openembedded-
> > core@lists.openembedded.org> On Behalf Of Steve Sakoman
> > Sent: den 14 juni 2023 16:05
> > To: openembedded-core@lists.openembedded.org
> > Subject: [OE-core][dunfell 3/5] systemd-systemctl: fix instance template
> > WantedBy symlink construction
> >
> > From: Martin Siegumfeldt <mns@gomspace.com>
> >
> > Fix issue of the below instance template systemd service dependency
> >
> > [Install]
> > WantedBy=svc-wants@%i.service
> >
> > creating the symlink (instance "a" example)
> >
> > /etc/systemd/system/svc-wants@%i.service.wants/svc-wanted-by@a.service
> >
> > which should be
> >
> > /etc/systemd/system/svc-wants@a.service.wants/svc-wanted-by@a.service
> >
> > as implemented by this change.
> >
> > The functionality appears regressed just after "thud" baseline when the
> > logic was refactored from shell script into python (commit
> > 925e30cb104ece7bfa48b78144e758a46dc9ec3f)
> >
> > (From OE-Core rev: 308397f0bb3d6f3d4e9ec2c6a10823184049c9b5)
> >
> > Signed-off-by: Martin Siegumfeldt <mns@gomspace.com>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > Signed-off-by: Steve Sakoman <steve@sakoman.com>
> > (cherry picked from commit 372b29c8ad270d4d430c26a4e614976c7029afaf)
> > Signed-off-by: Steve Sakoman <steve@sakoman.com>
> > ---
> >  .../systemd/systemd-systemctl/systemctl             | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > b/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > index 6aa2e20465..577c373181 100755
> > --- a/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > +++ b/meta/recipes-core/systemd/systemd-systemctl/systemctl
> > @@ -182,12 +182,19 @@ class SystemdUnit():
> >
> >          raise SystemdUnitNotFoundError(self.root, unit)
> >
> > -    def _process_deps(self, config, service, location, prop, dirstem):
> > +    def _process_deps(self, config, service, location, prop, dirstem, instance):
> >          systemdir = self.root / SYSCONFDIR / "systemd" / "system"
> >
> >          target = ROOT / location.relative_to(self.root)
> >          try:
> >              for dependent in config.get('Install', prop):
> > +                # determine whether or not dependent is a template with an actual
> > +                # instance (i.e. a '@%i')
> > +                dependent_is_template = re.match(r"[^@]+@(?P<instance>[^\.]*)\.", dependent)
> > +                if dependent_is_template:
> > +                    # if so, replace with the actual instance to achieve
> > +                    # svc-wants@a.service.wants/svc-wanted-by@a.service
> > +                    dependent = re.sub(dependent_is_template.group('instance'), instance, dependent, 1)
> >                  wants = systemdir / "{}.{}".format(dependent, dirstem) / service
> >                  add_link(wants, target)
> >
> > @@ -227,8 +234,8 @@ class SystemdUnit():
> >          else:
> >              service = self.unit
> >
> > -        self._process_deps(config, service, path, 'WantedBy', 'wants')
> > -        self._process_deps(config, service, path, 'RequiredBy', 'requires')
> > +        self._process_deps(config, service, path, 'WantedBy', 'wants', instance)
> > +        self._process_deps(config, service, path, 'RequiredBy', 'requires', instance)
> >
> >          try:
> >              for also in config.get('Install', 'Also'):
> > --
> > 2.34.1
>
> You might want to be careful with this one. It broke systemctl for us when it
> hit Mickledore and we have had to backport the version from Langdale for now.

Thanks for reviewing.  I'll drop this from dunfell for now and will
look for the potential fix in master for mickledore.

Steve

> The problem is that it does not handle non-instanced units depending on
> an instanced unit. In our case we have a unit foo.service that contains
> a RequiredBy=bar@0.service, which causes an error like this:
>
> Traceback (most recent call last):
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 366, in <module>
>     main()
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 352, in main
>     SystemdUnit(root, service).enable()
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 250, in enable
>     self._process_deps(config, service, path, 'WantedBy', 'wants', instance)
>   File ".../recipe-sysroot-native/usr/bin/systemctl", line 210, in _process_deps
>     dependent = re.sub(dependent_is_template.group('instance'), instance, dependent, 1)
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py", line 185, in sub
>     return _compile(pattern, flags).sub(repl, string, count)
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py", line 317, in _subx
>     template = _compile_repl(template, pattern)
>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/__init__.py", line 308, in _compile_repl
>     return _parser.parse_template(repl, pattern)
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/_parser.py", line 1000, in parse_template
>     s = Tokenizer(source)
>         ^^^^^^^^^^^^^^^^^
>   File ".../recipe-sysroot-native/usr/lib/python3.11/re/_parser.py", line 226, in __init__
>     string = str(string, 'latin1')
>              ^^^^^^^^^^^^^^^^^^^^^
> TypeError: decoding to str: need a bytes-like object, NoneType found
>
> There are probably other cases too when different kinds of units are
> involved (at least as far as I could gather from the systemd manual).
>
> We have an internal issue to fix this, but I am going on vacation on
> Monday, so I will unfortunately not have any time to look at it until
> after summer.
>
> //Peter
diff mbox series

Patch

diff --git a/meta/recipes-core/systemd/systemd-systemctl/systemctl b/meta/recipes-core/systemd/systemd-systemctl/systemctl
index 6aa2e20465..577c373181 100755
--- a/meta/recipes-core/systemd/systemd-systemctl/systemctl
+++ b/meta/recipes-core/systemd/systemd-systemctl/systemctl
@@ -182,12 +182,19 @@  class SystemdUnit():
 
         raise SystemdUnitNotFoundError(self.root, unit)
 
-    def _process_deps(self, config, service, location, prop, dirstem):
+    def _process_deps(self, config, service, location, prop, dirstem, instance):
         systemdir = self.root / SYSCONFDIR / "systemd" / "system"
 
         target = ROOT / location.relative_to(self.root)
         try:
             for dependent in config.get('Install', prop):
+                # determine whether or not dependent is a template with an actual
+                # instance (i.e. a '@%i')
+                dependent_is_template = re.match(r"[^@]+@(?P<instance>[^\.]*)\.", dependent)
+                if dependent_is_template:
+                    # if so, replace with the actual instance to achieve
+                    # svc-wants@a.service.wants/svc-wanted-by@a.service
+                    dependent = re.sub(dependent_is_template.group('instance'), instance, dependent, 1)
                 wants = systemdir / "{}.{}".format(dependent, dirstem) / service
                 add_link(wants, target)
 
@@ -227,8 +234,8 @@  class SystemdUnit():
         else:
             service = self.unit
 
-        self._process_deps(config, service, path, 'WantedBy', 'wants')
-        self._process_deps(config, service, path, 'RequiredBy', 'requires')
+        self._process_deps(config, service, path, 'WantedBy', 'wants', instance)
+        self._process_deps(config, service, path, 'RequiredBy', 'requires', instance)
 
         try:
             for also in config.get('Install', 'Also'):