Discussion:
[PATCH] sysv-generator: Replace Provides: symlinks with real units
(too old to reply)
Martin Pitt
2015-01-21 09:46:03 UTC
Permalink
Hello all,

while working on the sysv generator, some more cases came up where the
recently introduced "Provides:" symlink handling [1] causes trouble
[2]. As soon as you have backup files like /etc/init.d/foo.bak, you'll
get a "foo.service -> foo.bak.service" link which prevents the
creation of a real foo.service unit. A similar case can also happen if
one init.d script Provides: the name of another init.d script
(arguably this is at least questionable, but it might happen in
practice -- e. g. /etc/init.d/mariad might very well "Provides: mysql"
as it's kind of a drop-in replacement).

I wrote some more tests which reproduce these failures, and a proposed
patch. It's not exactly nice due to the TOCTOU (which shouldn't cause
any practical problem though, it's just a bit unclean), but I can't
think of a better solution which covers all corner cases.

Details are in the git commit message. Note that this currently adds
two log_debug() statements, so that you can better see what's going on
(and wrong) in the output for test failures. If you don't want to keep
them, I'm fine with dropping them again of course.

Opinions? (Especially from Thomas?)

Thanks,

Martin

[1] http://cgit.freedesktop.org/systemd/systemd/commit/?id=b7e7184634d5
[2] https://bugs.debian.org/775404
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
Jóhann B. Guðmundsson
2015-01-21 09:59:50 UTC
Permalink
Post by Martin Pitt
while working on the sysv generator, some more cases came up where the
recently introduced "Provides:" symlink handling [1] causes trouble
[2]. As soon as you have backup files like /etc/init.d/foo.bak, you'll
get a "foo.service -> foo.bak.service" link which prevents the
creation of a real foo.service unit
Seems like a corner case as administrator should fix himself by not
backing up files in the /etc/init.d directory so arguably this broken
behaviour is expect.

That said at one point or another we need to drop legacy sysv initscript
support and have downstream the generator themselves if they intend on
supporting legacy sysv initscripts.

JBG
Martin Pitt
2015-01-21 10:08:44 UTC
Permalink
Hey Jóhann,
Seems like a corner case as administrator should fix himself by not backing
up files in the /etc/init.d directory so arguably this broken behaviour is
expect.
With SysV init this isn't "broken" at all. As long as you don't
actually enable the backup files in rcN.d/, this is perfectly valid.
The effect is that systems with such backup files work fine under SysV
init and even under systemd up to 218, but will fail to boot under
systemd 219 onwards (i. e. with current master). I call this a
regression.
That said at one point or another we need to drop legacy sysv
initscript support and have downstream the generator themselves if
they intend on supporting legacy sysv initscripts.
If upstream wants to drop it at some point that's their prerogative of
course. I'd advise against it though, as LSB compliant systems need to
support SysV init scripts, it's still the lowest common denoniator,
and tons of third-party software still ship with init.d scripts. I. e.
it's not enough to port the distro packages.

So I expect if it gets dropped upstream, a lot of distros (and all the
major ones) will have to bring that back; it's IMHO better to just
maintain it upstream by the distro maintainers.

Thanks,

Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
Zbigniew Jędrzejewski-Szmek
2015-01-21 15:43:18 UTC
Permalink
Post by Martin Pitt
So I expect if it gets dropped upstream, a lot of distros (and all the
major ones) will have to bring that back; it's IMHO better to just
maintain it upstream by the distro maintainers.
Exactly. Dropping it would be just busy work for everyone. General
purpose distributions need to carry it for the forseeable future.

In the same vein, altough I don't think we should implement
significant new features, we should update the sysv script
compatibility as needed to keep it useful.

Zbyszek
Jóhann B. Guðmundsson
2015-01-21 19:56:46 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Martin Pitt
So I expect if it gets dropped upstream, a lot of distros (and all the
major ones) will have to bring that back; it's IMHO better to just
maintain it upstream by the distro maintainers.
Exactly. Dropping it would be just busy work for everyone. General
purpose distributions need to carry it for the forseeable future.
That argument does not hold water since there are systemd and
core/baseOS consumers that want the other side of that coin and have to
patch out all the legacy stuff.

Arguably upstream should be leading everybody into the future not
dwelling on the past and having to maintain it in the process.

JBG
Michael Biebl
2015-01-21 20:00:37 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Martin Pitt
So I expect if it gets dropped upstream, a lot of distros (and all the
major ones) will have to bring that back; it's IMHO better to just
maintain it upstream by the distro maintainers.
Exactly. Dropping it would be just busy work for everyone. General
purpose distributions need to carry it for the forseeable future.
That argument does not hold water since there are systemd and core/baseOS
consumers that want the other side of that coin and have to patch out all
the legacy stuff.
That's non-sense. The sysv-generator is a separate component which can
be disabled via a configure switch.
No need to patch anything out.
--
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?
Jóhann B. Guðmundsson
2015-01-21 20:11:27 UTC
Permalink
Post by Michael Biebl
Post by Zbigniew Jędrzejewski-Szmek
Post by Martin Pitt
So I expect if it gets dropped upstream, a lot of distros (and all the
major ones) will have to bring that back; it's IMHO better to just
maintain it upstream by the distro maintainers.
Exactly. Dropping it would be just busy work for everyone. General
purpose distributions need to carry it for the forseeable future.
That argument does not hold water since there are systemd and core/baseOS
consumers that want the other side of that coin and have to patch out all
the legacy stuff.
That's non-sense. The sysv-generator is a separate component which can
be disabled via a configure switch.
No need to patch anything out.
Let me re-phrase that "configure it" out...

JBG
Lennart Poettering
2015-01-28 00:47:51 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
Post by Martin Pitt
So I expect if it gets dropped upstream, a lot of distros (and all the
major ones) will have to bring that back; it's IMHO better to just
maintain it upstream by the distro maintainers.
Exactly. Dropping it would be just busy work for everyone. General
purpose distributions need to carry it for the forseeable future.
That argument does not hold water since there are systemd and core/baseOS
consumers that want the other side of that coin and have to patch out all
the legacy stuff.
Arguably upstream should be leading everybody into the future not dwelling
on the past and having to maintain it in the process.
If you compile systemd and pass empty strings for the sysv rc and
scripts paths, then this turns off all sysv support in systemd, and
all that old cruft goes away. Today.

Lennart
--
Lennart Poettering, Red Hat
Lennart Poettering
2015-01-28 00:45:59 UTC
Permalink
Post by Martin Pitt
Hey Jóhann,
Seems like a corner case as administrator should fix himself by not backing
up files in the /etc/init.d directory so arguably this broken behaviour is
expect.
With SysV init this isn't "broken" at all. As long as you don't
actually enable the backup files in rcN.d/, this is perfectly valid.
The effect is that systems with such backup files work fine under SysV
init and even under systemd up to 218, but will fail to boot under
systemd 219 onwards (i. e. with current master). I call this a
regression.
That said at one point or another we need to drop legacy sysv
initscript support and have downstream the generator themselves if
they intend on supporting legacy sysv initscripts.
If upstream wants to drop it at some point that's their prerogative of
course. I'd advise against it though, as LSB compliant systems need to
support SysV init scripts, it's still the lowest common denoniator,
and tons of third-party software still ship with init.d scripts. I. e.
it's not enough to port the distro packages.
Just to clarify: I have zero intention to drop LSB script support
any time soon.

Lennart
--
Lennart Poettering, Red Hat
Zbigniew Jędrzejewski-Szmek
2015-01-21 15:36:26 UTC
Permalink
Keeping track of which alias symlinks we actually want is error prone, and
restricting the creation of services for enabled init.d scripts would reduce
the utility of the generator (for manual starting disabled init.d scripts) as
well as not cover the second case. So if we encounter an existing symlink, just
remove it before writing a real unit.
Looks fine. Although the code is clearer than the description :)
Note that two init.d scripts "foo" and "bar" which both provide the same name
"common" already work. The first processed init script wins and creates the
"common.service" symlink, and the second just fails to create the symlink
again. Thus create an additional test case for this to ensure that it keeps
working sensibly.
https://bugs.debian.org/775404
---
src/sysv-generator/sysv-generator.c | 12 ++++++++++++
test/sysv-generator-test.py | 38 +++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+)
diff --git a/src/sysv-generator/sysv-generator.c b/src/sysv-generator/sysv-generator.c
index a47b072..fd3ee20 100644
--- a/src/sysv-generator/sysv-generator.c
+++ b/src/sysv-generator/sysv-generator.c
@@ -147,6 +147,7 @@ static int generate_unit_file(SysvStub *s) {
_cleanup_free_ char *wants = NULL;
_cleanup_free_ char *conflicts = NULL;
int r;
+ struct stat st;
before = strv_join(s->before, " ");
if (!before)
@@ -168,6 +169,14 @@ static int generate_unit_file(SysvStub *s) {
if (!unit)
return log_oom();
+ /* We might already have a symlink with the same name from a Provides:,
+ * or from backup files like /etc/init.d/foo.bak. Real scripts always win,
+ * so remove an existing link */
+ if (lstat(unit, &st) == 0 && S_ISLNK(st.st_mode)) {
+ log_warning("Overwriting existing symlink %s with real service", unit);
+ unlink(unit);
+ }
+
f = fopen(unit, "wxe");
if (!f)
return log_error_errno(errno, "Failed to create unit file %s: %m", unit);
@@ -343,6 +352,8 @@ static int load_sysv(SysvStub *s) {
if (!f)
return errno == ENOENT ? 0 : -errno;
+ log_debug("loading SysV script %s", s->path);
Capital "L"?
+
while (!feof(f)) {
char l[LINE_MAX], *t;
@@ -492,6 +503,7 @@ static int load_sysv(SysvStub *s) {
continue;
if (unit_name_to_type(m) == UNIT_SERVICE) {
+ log_debug("Adding Provides: alias '%s' for '%s'", m, s->name);
r = add_alias(s->name, m);
} else {
/* NB: SysV targets
diff --git a/test/sysv-generator-test.py b/test/sysv-generator-test.py
index a3daa9f..63a10ec 100644
--- a/test/sysv-generator-test.py
+++ b/test/sysv-generator-test.py
self.assertEqual(os.readlink(os.path.join(self.out_dir, f)),
'foo.service')
+ '''multiple init.d scripts provide the same name'''
+
+ self.add_sysv('foo', {'Provides': 'foo common'}, enable=True, prio=1)
+ self.add_sysv('bar', {'Provides': 'bar common'}, enable=True, prio=2)
+ err, results = self.run_generator()
+ self.assertEqual(sorted(results), ['bar.service', 'foo.service'])
+ # should create symlink for the alternative name for either unit
+ self.assertIn(os.readlink(os.path.join(self.out_dir, 'common.service')),
+ ['foo.service', 'bar.service'])
+
+ '''init.d scripts provides the name of another init.d script'''
+
+ self.add_sysv('foo', {'Provides': 'foo bar'}, enable=True)
+ self.add_sysv('bar', {'Provides': 'bar'}, enable=True)
+ err, results = self.run_generator()
+ self.assertEqual(sorted(results), ['bar.service', 'foo.service'])
+
'''ignores non-executable init.d script'''
self.assertEqual(os.readlink(os.path.join(self.out_dir, 'bar.service')),
'foo.service')
+ '''init.d script with backup file'''
+
+ script = self.add_sysv('foo', {}, enable=True)
+ # backup files (not enabled in rcN.d/)
+ shutil.copy(script, script + '.bak')
+ shutil.copy(script, script + '.old')
+
+ err, results = self.run_generator()
+ print(err)
+ self.assertEqual(sorted(results),
+ ['foo.bak.service', 'foo.old.service', 'foo.service'])
+
+ # ensure we don't try to create a symlink to itself
+ self.assertNotIn(err, 'itself')
+
+ self.assert_enabled('foo.service', [2, 3, 4, 5])
+ self.assert_enabled('foo.bak.service', [])
+ self.assert_enabled('foo.old.service', [])
Looks fine from my POV.

Zbyszek
Martin Pitt
2015-01-21 16:04:29 UTC
Permalink
Hey Zbigniew,
Post by Zbigniew Jędrzejewski-Szmek
Keeping track of which alias symlinks we actually want is error prone, and
restricting the creation of services for enabled init.d scripts would reduce
the utility of the generator (for manual starting disabled init.d scripts) as
well as not cover the second case. So if we encounter an existing symlink, just
remove it before writing a real unit.
Looks fine. Although the code is clearer than the description :)
Heh. I removed the above rationale why it isn't done in a different
way, as it might be indeed overcomplicating the commit log.
Post by Zbigniew Jędrzejewski-Szmek
+ log_debug("loading SysV script %s", s->path);
Capital "L"?
Fixed.
Post by Zbigniew Jędrzejewski-Szmek
Looks fine from my POV.
Thanks for the review! Pushed.

Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
Lennart Poettering
2015-01-28 00:44:11 UTC
Permalink
Post by Martin Pitt
A similar case can also happen if
one init.d script Provides: the name of another init.d script
(arguably this is at least questionable, but it might happen in
practice -- e. g. /etc/init.d/mariad might very well "Provides: mysql"
as it's kind of a drop-in replacement).
I wrote some more tests which reproduce these failures, and a proposed
patch. It's not exactly nice due to the TOCTOU (which shouldn't cause
any practical problem though, it's just a bit unclean), but I can't
think of a better solution which covers all corner cases.
I am not a fan of this stuff either. I really don't like the TOCTOU
behaviour I must say...

If this is really just about ".bak", then we can add it to the list of
suffixes in hidden_files()...

I think a much better fix for all of this would be to first read in
all sysv scripts, and only then start creating aliases. So far we read
everything in, and while doing so already create symlinks, while
defering creation of the unit files to the end. If we moevd the
symlink creation part to the end too we could easily check in the sysv
script hashtable if we have a real script for a name before writing
out an alias symlink for this.

Lennart
--
Lennart Poettering, Red Hat
Michael Biebl
2015-01-28 13:56:17 UTC
Permalink
Post by Lennart Poettering
I am not a fan of this stuff either. I really don't like the TOCTOU
behaviour I must say...
If this is really just about ".bak", then we can add it to the list of
suffixes in hidden_files()...
Martin already committed an update [1] to at least ignore all the
temporary files created by dpkg and dpkg-related helper tools.

While adding .bak would probably not be wrong, I don't think it would
solve this particular issue in a robust way.
People are very inventive when it comes to such names. In the Debian
bug report which triggered this patch, the user had a file named
networking.save [2]. Most likely, this had been created by himself and
later on he forgot to remove it.
Post by Lennart Poettering
I think a much better fix for all of this would be to first read in
all sysv scripts, and only then start creating aliases.
I guess with "read in", you actually mean read and parse all init
scripts *and* generate all unit files. After that create the symlinks.

So far we read
Post by Lennart Poettering
everything in, and while doing so already create symlinks, while
defering creation of the unit files to the end. If we moevd the
symlink creation part to the end too we could easily check in the sysv
script hashtable if we have a real script for a name before writing
out an alias symlink for this.
I agree here and incidentally mentioned that in [3].
Martin and I briefly discussed that in #debian-systemd and IIRC, the
reason why Martin did it this way was simply because it was less work.

Michael

[1] c7088e4999f2e5dd33259948c806f4e2706e77ce
[2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775404#15
[3] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775404#54
--
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?
Zbigniew Jędrzejewski-Szmek
2015-01-28 14:06:35 UTC
Permalink
Post by Michael Biebl
Post by Lennart Poettering
I am not a fan of this stuff either. I really don't like the TOCTOU
behaviour I must say...
If this is really just about ".bak", then we can add it to the list of
suffixes in hidden_files()...
Martin already committed an update [1] to at least ignore all the
temporary files created by dpkg and dpkg-related helper tools.
While adding .bak would probably not be wrong, I don't think it would
solve this particular issue in a robust way.
People are very inventive when it comes to such names. In the Debian
bug report which triggered this patch, the user had a file named
networking.save [2]. Most likely, this had been created by himself and
later on he forgot to remove it.
Post by Lennart Poettering
I think a much better fix for all of this would be to first read in
all sysv scripts, and only then start creating aliases.
I guess with "read in", you actually mean read and parse all init
scripts *and* generate all unit files. After that create the symlinks.
Actually this will not help for the TOCTOU race, IIUC.
systemd-sysv-generator does not race with itself, but with other
generators. But it seems fairly unlikely that a different generator
would create a symlink in generators.early, and if they do, that's
pretty much undefined behaviour who wins.

Zbyszek

Continue reading on narkive:
Loading...