Bug#704197: Please review: systemd checks
Hi Niels,
Niels Thykier writes:
> I thought this was safe, but it does have an issue as well. Consider
> symlink chaining:
>
> safe-symlink -> unsafe-symlink
> unsafe-symlink -> ../../../../etc/passwd
>
> $path->link_resolved will approve "safe-symlink" because it can be
> resolved safely. However, it does not check that the target is also a
> safe symlink - so a loop/recursion is needed. That said, using the new
> "is_ancestor_of" (from L::Util) is probably a lot easier to use
> correctly. Basically:
Thanks for the explanation and the example. I have updated my code and
the tests still work. Find the updated patches attached (rebased against
current master).
--
Best regards,
Michael
>From ceb4afecf02c6c1a1277ad69bb2d3430baed6fa9 Mon Sep 17 00:00:00 2001
From: Michael Stapelberg
Date: Sat, 13 Apr 2013 23:14:31 +0200
Subject: [PATCH 1/3] add systemd checks
---
checks/systemd | 252 +++
checks/systemd.desc | 76
2 files changed, 328 insertions(+)
create mode 100644 checks/systemd
create mode 100644 checks/systemd.desc
diff --git a/checks/systemd b/checks/systemd
new file mode 100644
index 000..866110c
--- /dev/null
+++ b/checks/systemd
@@ -0,0 +1,252 @@
+# systemd -- lintian check script -*- perl -*-
+#
+# Copyright © 2013 Michael Stapelberg
+#
+# based on the apache2 checks file by:
+# Copyright © 2012 Arno Töll
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, you can find it on the World Wide
+# Web at http://www.gnu.org/copyleft/gpl.html, or write to the Free
+# Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
+# MA 02110-1301, USA.
+
+package Lintian::systemd;
+
+use strict;
+use warnings;
+
+use File::Basename;
+use Text::ParseWords qw(shellwords);
+
+use Lintian::Tags qw(tag);
+use Lintian::Util qw(fail);
+
+sub run {
+my ($pkg, $type, $info) = @_;
+
+# Figure out whether the maintainer of this package did any effort to
+# make the package work with systemd. If not, we will not warn in case
+# of an init script that has no systemd equivalent, for example.
+my $ships_systemd_file = (scalar ( grep { m,/systemd/, } $info->sorted_index ) > 0);
+
+# An array of names which are provided by the service files.
+# This includes Alias= directives, so after parsing
+# NetworkManager.service, it will contain NetworkManager and
+# network-manager.
+my @systemd_targets;
+
+for my $file ($info->sorted_index) {
+if ($file =~ m,^etc/tmpfiles\.d/.*\.conf$,) {
+tag 'systemd-tmpfiles.d-outside-usr-lib', $file;
+}
+if ($file =~ m,^etc/systemd/system/.*\.service$,) {
+tag 'systemd-service-file-outside-lib', $file;
+}
+if ($file =~ m,/systemd/system/.*\.service$,) {
+check_systemd_service_file ($pkg, $info, $file);
+for my $name (extract_service_file_names ($pkg, $info, $file)) {
+push @systemd_targets, $name;
+}
+}
+}
+
+my @init_scripts = grep { m,^etc/init\.d/.+, } $info->sorted_index;
+
+# Verify that each init script includes /lib/lsb/init-functions,
+# because that is where the systemd diversion happens.
+for my $init_script (@init_scripts) {
+check_init_script ($pkg, $info, $init_script);
+}
+
+@init_scripts = map { basename($_) } @init_scripts;
+
+if ($ships_systemd_file) {
+for my $init_script (@init_scripts) {
+tag 'systemd-no-service-for-init-script', $init_script
+unless grep /\Q$init_script\E\.service/, @systemd_targets;
+}
+}
+
+check_maintainer_scripts ($info);
+}
+
+sub check_init_script {
+my ($pkg, $info, $file) = @_;
+
+my $lsb_source_seen;
+my $path = $info->index ($file);
+unless ($path->is_regular_file ||
+($path->is_symlink && defined($path->link_resolved))) {
+tag 'init-script-is-not-a-file', $file;
+return;
+}
+open(my $fh, '<', $info->unpacked ($file))
+or fail "cannot open $file: $!";
+while (<$fh>) {
+s/^\s+//;
+next if /^#/;
+if (m,^(?:\.|source)\s+/lib/lsb/init-functions,) {
+$lsb_source_seen = 1;
+last;
+}
+}
+close($fh);
+
+if (!$lsb_source_seen) {
+tag 'init.d-script-does-not-source-init-functions', $file;
+}
+}
+
+sub check_system
Bug#704197: Please review: systemd checks
On 2013-04-06 18:33, Michael Stapelberg wrote:
> Hi Niels,
>
> [...]
> Okay, so how about this?
>
> sub check_init_script {
> my ($pkg, $info, $file) = @_;
>
> my $lsb_source_seen;
> my $path = $info->index ($file);
> unless ($path->is_regular_file ||
> ($path->is_symlink && defined($path->link_resolved))) {
> tag 'init-script-is-not-a-file', $file;
> }
> open(my $fh, '<', $info->unpacked($file))
> or fail "cannot open $file: $!";
> # …
> }
>
> [...]
I thought this was safe, but it does have an issue as well. Consider
symlink chaining:
safe-symlink -> unsafe-symlink
unsafe-symlink -> ../../../../etc/passwd
$path->link_resolved will approve "safe-symlink" because it can be
resolved safely. However, it does not check that the target is also a
safe symlink - so a loop/recursion is needed. That said, using the new
"is_ancestor_of" (from L::Util) is probably a lot easier to use
correctly. Basically:
use Lintian::Util qw(is_ancestor_of);
[...]
my $unpacked_file = $info->unpacked($file);
if ( -f $unpacked_file &&
is_ancestor_of($info->unpacked, $unpacked_file)) {
# exists, is a file and within the package root.
open(my $fd, '<', $unpacked_file) or fail "..."
[...]
} else {
# unsafe, is not a file or does not exist
[...]
}
~Niels
--
To UNSUBSCRIBE, email to debian-lint-maint-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/516d82b4.6080...@thykier.net
Bug#704197: Please review: systemd checks
Hi Niels,
Thanks for your prompt reply.
Niels Thykier writes:
> Tests! Once there are tests for all the new tags (and none of the
> existing tests breaks) we are usually ready to accept the checks.
Cool.
Find attached two git format-patch files. The first adds the latest
version of my systemd checks, the second one adds tests.
Could you please have a look at the tests? Please note that I did not
run the whole testsuite because it fails on my machine (see my IRC
query). It’d be great if you could run it for me.
--
Best regards,
Michael
>From eb5c8b33019e1b838f675bd455052b3d1347fe75 Mon Sep 17 00:00:00 2001
From: Michael Stapelberg
Date: Sat, 13 Apr 2013 23:14:31 +0200
Subject: [PATCH 1/2] add systemd checks
---
checks/systemd | 252 +++
checks/systemd.desc | 76
2 files changed, 328 insertions(+)
create mode 100644 checks/systemd
create mode 100644 checks/systemd.desc
diff --git a/checks/systemd b/checks/systemd
new file mode 100644
index 000..866110c
--- /dev/null
+++ b/checks/systemd
@@ -0,0 +1,252 @@
+# systemd -- lintian check script -*- perl -*-
+#
+# Copyright © 2013 Michael Stapelberg
+#
+# based on the apache2 checks file by:
+# Copyright © 2012 Arno Töll
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, you can find it on the World Wide
+# Web at http://www.gnu.org/copyleft/gpl.html, or write to the Free
+# Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
+# MA 02110-1301, USA.
+
+package Lintian::systemd;
+
+use strict;
+use warnings;
+
+use File::Basename;
+use Text::ParseWords qw(shellwords);
+
+use Lintian::Tags qw(tag);
+use Lintian::Util qw(fail);
+
+sub run {
+my ($pkg, $type, $info) = @_;
+
+# Figure out whether the maintainer of this package did any effort to
+# make the package work with systemd. If not, we will not warn in case
+# of an init script that has no systemd equivalent, for example.
+my $ships_systemd_file = (scalar ( grep { m,/systemd/, } $info->sorted_index ) > 0);
+
+# An array of names which are provided by the service files.
+# This includes Alias= directives, so after parsing
+# NetworkManager.service, it will contain NetworkManager and
+# network-manager.
+my @systemd_targets;
+
+for my $file ($info->sorted_index) {
+if ($file =~ m,^etc/tmpfiles\.d/.*\.conf$,) {
+tag 'systemd-tmpfiles.d-outside-usr-lib', $file;
+}
+if ($file =~ m,^etc/systemd/system/.*\.service$,) {
+tag 'systemd-service-file-outside-lib', $file;
+}
+if ($file =~ m,/systemd/system/.*\.service$,) {
+check_systemd_service_file ($pkg, $info, $file);
+for my $name (extract_service_file_names ($pkg, $info, $file)) {
+push @systemd_targets, $name;
+}
+}
+}
+
+my @init_scripts = grep { m,^etc/init\.d/.+, } $info->sorted_index;
+
+# Verify that each init script includes /lib/lsb/init-functions,
+# because that is where the systemd diversion happens.
+for my $init_script (@init_scripts) {
+check_init_script ($pkg, $info, $init_script);
+}
+
+@init_scripts = map { basename($_) } @init_scripts;
+
+if ($ships_systemd_file) {
+for my $init_script (@init_scripts) {
+tag 'systemd-no-service-for-init-script', $init_script
+unless grep /\Q$init_script\E\.service/, @systemd_targets;
+}
+}
+
+check_maintainer_scripts ($info);
+}
+
+sub check_init_script {
+my ($pkg, $info, $file) = @_;
+
+my $lsb_source_seen;
+my $path = $info->index ($file);
+unless ($path->is_regular_file ||
+($path->is_symlink && defined($path->link_resolved))) {
+tag 'init-script-is-not-a-file', $file;
+return;
+}
+open(my $fh, '<', $info->unpacked ($file))
+or fail "cannot open $file: $!";
+while (<$fh>) {
+s/^\s+//;
+next if /^#/;
+if (m,^(?:\.|source)\s+/lib/lsb/init-functions,) {
+$lsb_source_seen = 1;
+last;
+}
+}
+close($fh);
+
+if (!$lsb_source_seen) {
+tag 'init.d-script-does-not-source-init-functions', $file;
+}
+}
+
+sub check_systemd_service_file {
+my ($pkg, $info, $file) = @_;
+
+my @values = extract_service_file_values ($pkg, $info, $file, 'Unit', 'After');
+my @obsolete
Bug#704197: Please review: systemd checks
On 2013-04-06 20:44, Michael Stapelberg wrote:
> Hi Niels,
>
> Niels Thykier writes:
>> I think you are missing a return here?
> Indeed, thanks.
>
> New files are attached, here is the list of things that I know need to
> be fixed:
>
> 1) We don’t have any documentation references in the .desc file yet.
> 2) I need to switch to lab_data_path in check_maintainer_scripts().
>
> Could you please also say a few words on how the usual inclusion process
> works? I.e., what are the next steps after there are no more things left
> to fix?
Tests! Once there are tests for all the new tags (and none of the
existing tests breaks) we are usually ready to accept the checks.
On test writing: We don't have a tutorial for writing tests atm. There
is a bit of help (although a bit outdated) in t/tests/README.
To give you a rough idea of what you are working with, a test case in
t/tests basically follows this pattern:
mkdir -p rundir/test/
rsync t/templates/tests/skel/ rundir/test/
# note t/tests//debian/ is the root of the source package
# thus, t/tests//debian/debian/ is the "debian" dir.
rsync t/tests//debian/ rundir/test/
for file in changelog control ; do
subst rundir/test/debian/${file}.in > rundir/test/debian/${file}
done
cd rundir/test && dpkg-buildpackage -us -uc
lintian -IE rundir/*.changes | sort > actual
diff -u t/tests//tags actual
The test should be named after the check (i.e. systemd- in this
case) and have a sequence of 6000 (in t/tests//desc). A
template for the desc:
Testname:
Sequence: 6000
Description: <1-line description>
Test-For:
tag1
tag2
Note: Testname must match the (base)name of the directory[1]. The
description will always be used as synopsis for the package (so it
should follow the same rules). There are more fields you can use (see
t/tests/README).
If you want to test for false-positives, replace Test-For with
Test-Against[2].
The full test suite takes over 30 minutes (user time). Have a look at
Lintian::Tutorial::TestSuite to run only the tests you need while you
work on the tests/check[3]. When you are done, please do a full run of
all tests. Some of the other tests do some funny checks that may
trigger bugs in your check[4].
If the full test suite is too heavy for your machine (my laptop
doesn't like it either), just let me know and I will do the run for you.
> Also, do I need to mark the checks as experimental because they are new?
>
Depends on your level of confidence in your tags and how "well-defined"
the "problem" is[5]. Most tags never had the experimental marker on them.
~Niels
[1] Not checked, but can cause "hard to debug test failures"...
especially if it matches the name of another test (copy-waste ftl).
[2] Test-For requires the test to emit the tag for it to pass.
Test-Against causes it to fail unconditionally if the tag was emitted.
Both fields may be used at the same time.
[3] To test your check for syntax errors (etc.), you may want to run:
debian/rules onlyrun=check-load
After that:
debian/rules onlyrun=
debian/rules onlyrun=
debian/rules onlyrun=suite:scripts
When they pass, you might be ready for the full test suite. If you got
cores to spare, throw a DEB_BUILD_OPTIONS=parallel=n.
[4] e.g. suite:debs builds some really interesting debs. The legacy test
"filenames" also got some fun filenames in it.
[5] If there are a lot of corner cases (etc.), the check may need
several iterations to mature.
--
To UNSUBSCRIBE, email to debian-lint-maint-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/51607e34.3030...@thykier.net
Bug#704197: Please review: systemd checks
Hi Niels, Niels Thykier writes: > I think you are missing a return here? Indeed, thanks. New files are attached, here is the list of things that I know need to be fixed: 1) We don’t have any documentation references in the .desc file yet. 2) I need to switch to lab_data_path in check_maintainer_scripts(). Could you please also say a few words on how the usual inclusion process works? I.e., what are the next steps after there are no more things left to fix? Also, do I need to mark the checks as experimental because they are new? -- Best regards, Michael systemd Description: Binary data systemd.desc Description: Binary data
Bug#704197: Please review: systemd checks
On 2013-04-06 18:33, Michael Stapelberg wrote:
> Hi Niels,
>
> Niels Thykier writes:
>> [...]
>> Almost; it definitely plugs the issues I mentioned. That said, I
>> believe we prefer to emit tags instead of erroring out when we see an
>> unexpected file type (e.g. see control-file-is-not-a-file).
>> Secondly, there is a bug in that link_resolved is only applicable to
>> links. So if it is not a regular file and not a link, the code will
>> croak in $path->link_resolved[2].
> Okay, so how about this?
>
> sub check_init_script {
> my ($pkg, $info, $file) = @_;
>
> my $lsb_source_seen;
> my $path = $info->index ($file);
> unless ($path->is_regular_file ||
> ($path->is_symlink && defined($path->link_resolved))) {
> tag 'init-script-is-not-a-file', $file;
I think you are missing a return here?
> }
> open(my $fh, '<', $info->unpacked($file))
> or fail "cannot open $file: $!";
> # …
> }
>
>> It really looks like a implementation of Text::ParseWords's
>> shellwords[3]. If so, we can get that entire sub as a oneliner (we
>> already use Text::ParseWords elsewhere).
> I switched to shellwords. We can always rever to the code we’ve had
> before, but in my tests, shellwords works fine.
>
Appreciated.
> Find the new files attached.
>
~Niels
--
To UNSUBSCRIBE, email to debian-lint-maint-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/516064aa.6050...@thykier.net
Bug#704197: Please review: systemd checks
Hi Niels,
Niels Thykier writes:
>> sub check_init_script {
>> my ($pkg, $info, $file) = @_;
>>
>> my $lsb_source_seen;
>> my $path = $info->index ($file);
>> fail "$file is neither a regular file nor a resolvable symlink"
>> unless ($path->is_regular_file || defined($path->link_resolved));
>> open(my $fh, '<', $info->unpacked($file))
>> or fail "cannot open $file: $!";
>>
>> # …
>> }
>>
>> Does that seem alright to you?
>>
>
> Almost; it definitely plugs the issues I mentioned. That said, I
> believe we prefer to emit tags instead of erroring out when we see an
> unexpected file type (e.g. see control-file-is-not-a-file).
> Secondly, there is a bug in that link_resolved is only applicable to
> links. So if it is not a regular file and not a link, the code will
> croak in $path->link_resolved[2].
Okay, so how about this?
sub check_init_script {
my ($pkg, $info, $file) = @_;
my $lsb_source_seen;
my $path = $info->index ($file);
unless ($path->is_regular_file ||
($path->is_symlink && defined($path->link_resolved))) {
tag 'init-script-is-not-a-file', $file;
}
open(my $fh, '<', $info->unpacked($file))
or fail "cannot open $file: $!";
# …
}
> It really looks like a implementation of Text::ParseWords's
> shellwords[3]. If so, we can get that entire sub as a oneliner (we
> already use Text::ParseWords elsewhere).
I switched to shellwords. We can always rever to the code we’ve had
before, but in my tests, shellwords works fine.
Find the new files attached.
--
Best regards,
Michael
systemd
Description: Binary data
systemd.desc
Description: Binary data
Bug#704197: Please review: systemd checks
On 2013-03-29 15:24, Michael Stapelberg wrote:
> Hi Niels,
>
> Thanks for the super-fast review. New version is attached, I have fixed
> everything you mentioned, and for the other things I commented inline:
>
You are welcome :) I have not reviewed your revised version yet, but I
will try to look at it soon.
> Niels Thykier writes:
>> guidelines. I know Lintian's code style is a mess in general, so it
>> describes the style I hope we will eventually reach[1]. :)
> Have you tried using perltidy for Lintian? I loathe manual source code
> formatting after working with gofmt and subsequently perltidy.
>
We did have an "off-by-default" perlcritic test, but not a perltidy one.
I have spent some time with the perlcritic test and it is now on by
default (though with fewer policies than originally).
>> I noticed that there appear to be no use of references (Ref: URL, #bug,
>> policy X.Y ...). I would recommend finding such so people can quickly
>> find more information. Links to systemd documentation, specification or
>> even just a Debian wiki page.
> Will do once we’ve put up some wiki pages on that.
>
Good. :)
>> If you do not need $pkg or $type, then you can replace them with undef. E.g.
>> my (undef, undef, $info) = @_;
> I prefer to have the variables around, just in case the code needs to be
> changed to use those.
>
I don't feel very strongly about unused arguments[1]. If the perlcritic
policy on unused variables does not trigger on it, they can stay.
Though, I may out of habbit kill the unused variable if I happen to
touch that code.
[1] In many other languages you cannot avoid declaring them anyway :)
>> That has the advantage that we know that argument is unused.
> I don’t understand what the advantage of knowing that is :-).
>
I like it because it sends a clear signal by not having a "name" on the
argument (also, not sure how smart Perl's compile time analysis is so it
may even save a minor amount of memory).
>> Secondly, there is no check for file type. If someone (deliberately)
>> creates $file as a fifo-pipe or a symlink it will DoS or (possibly) read
>> "host system" files (respectively). Usually, a
>>
>> $info->index ($file)->is_regular_file
>>
>> should do (if symlinks/hardlinks can be ignored). Alternatively, (for
>> symlinks) please check that the symlink can be safely resolved before
>> opening the file (e.g. via the link_resolved method). For more
>> information, please see the Lintian::Path module's API.
> I came up with this:
>
> sub check_init_script {
> my ($pkg, $info, $file) = @_;
>
> my $lsb_source_seen;
> my $path = $info->index ($file);
> fail "$file is neither a regular file nor a resolvable symlink"
> unless ($path->is_regular_file || defined($path->link_resolved));
> open(my $fh, '<', $info->unpacked($file))
> or fail "cannot open $file: $!";
>
> # …
> }
>
> Does that seem alright to you?
>
Almost; it definitely plugs the issues I mentioned. That said, I
believe we prefer to emit tags instead of erroring out when we see an
unexpected file type (e.g. see control-file-is-not-a-file).
Secondly, there is a bug in that link_resolved is only applicable to
links. So if it is not a regular file and not a link, the code will
croak in $path->link_resolved[2].
[2] Admittedly a non-issue with the current code where it would
eventually have called "fail" instead. But if the fail part is replaced
with a tag, it becomes an issue.
>>> sub split_quoted {
>>> [...]
>>> }
>>>
>>
>> Is this something that could be done by Text::ParseWords?
> I’m not entirely sure about it. The code I’m using is a 1:1 port of the
> corresponding systemd C code. This obviously has the benefit that there
> are no subtle differences between what we do and what systemd does.
>
It really looks like a implementation of Text::ParseWords's
shellwords[3]. If so, we can get that entire sub as a oneliner (we
already use Text::ParseWords elsewhere).
~Niels
[3] http://perldoc.perl.org/Text/ParseWords.html
--
To UNSUBSCRIBE, email to debian-lint-maint-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/515a05f5.5010...@thykier.net
Bug#704197: Please review: systemd checks
Michael Stapelberg writes:
> Niels Thykier writes:
>> guidelines. I know Lintian's code style is a mess in general, so it
>> describes the style I hope we will eventually reach[1]. :)
> Have you tried using perltidy for Lintian? I loathe manual source code
> formatting after working with gofmt and subsequently perltidy.
I desperately need to update the page that Niels is looking at to
incorporate the results of a very comprehensive style review we did at
Stanford with reference to Perl Best Practices, as it's changed a lot. We
have a new house style that's similar but incorporates a ton of advice
from Perl Best Practices, and I have both perlcritic and perltidy
configurations that correspond to it (as closely as one can, at least --
perltidy is sadly not very flexible).
Some of those changes are, alas, annoyingly intrusive compared to all of
my previous Perl code (which is a chunk of the code in Lintian); for
example, I was convinced to drop the space before the parens on function
calls because it seems to really confuse people coming from just about any
other language except GNU-style C.
I definitely recommend adopting perlcritic and perltidy for this sort of
thing, though, and now use them as part of an automated test suite for my
Perl code.
For a copy of the standard tests that I'm now using, as well as the
perlcriticrc and perltidyrc files (until I can get everything else
updated), clone:
git://git.eyrie.org/devel/rra-c-util.git
and look at tests/data/perl{critic,tidy}rc and the tests in perl/t.
tests/tap/perl/Test/RRA.pm and tests/tap/perl/test/Config.pm may also be
of interest (particularly the former; the latter is used by my tests to
load configuration so that I can reuse the same test scripts across
multiple packages with different requirements).
I'll send the raw Markdown of our new internal style to the Lintian list.
--
Russ Allbery (r...@debian.org)
Bug#704197: Please review: systemd checks
Hi Niels,
Thanks for the super-fast review. New version is attached, I have fixed
everything you mentioned, and for the other things I commented inline:
Niels Thykier writes:
> guidelines. I know Lintian's code style is a mess in general, so it
> describes the style I hope we will eventually reach[1]. :)
Have you tried using perltidy for Lintian? I loathe manual source code
formatting after working with gofmt and subsequently perltidy.
> I noticed that there appear to be no use of references (Ref: URL, #bug,
> policy X.Y ...). I would recommend finding such so people can quickly
> find more information. Links to systemd documentation, specification or
> even just a Debian wiki page.
Will do once we’ve put up some wiki pages on that.
> If you do not need $pkg or $type, then you can replace them with undef. E.g.
> my (undef, undef, $info) = @_;
I prefer to have the variables around, just in case the code needs to be
changed to use those.
> That has the advantage that we know that argument is unused.
I don’t understand what the advantage of knowing that is :-).
> Secondly, there is no check for file type. If someone (deliberately)
> creates $file as a fifo-pipe or a symlink it will DoS or (possibly) read
> "host system" files (respectively). Usually, a
>
> $info->index ($file)->is_regular_file
>
> should do (if symlinks/hardlinks can be ignored). Alternatively, (for
> symlinks) please check that the symlink can be safely resolved before
> opening the file (e.g. via the link_resolved method). For more
> information, please see the Lintian::Path module's API.
I came up with this:
sub check_init_script {
my ($pkg, $info, $file) = @_;
my $lsb_source_seen;
my $path = $info->index ($file);
fail "$file is neither a regular file nor a resolvable symlink"
unless ($path->is_regular_file || defined($path->link_resolved));
open(my $fh, '<', $info->unpacked($file))
or fail "cannot open $file: $!";
# …
}
Does that seem alright to you?
>> sub split_quoted {
>> [...]
>> }
>>
>
> Is this something that could be done by Text::ParseWords?
I’m not entirely sure about it. The code I’m using is a 1:1 port of the
corresponding systemd C code. This obviously has the benefit that there
are no subtle differences between what we do and what systemd does.
--
Best regards,
Michael
systemd
Description: Binary data
systemd.desc
Description: Binary data
Bug#704197: Please review: systemd checks
On 2013-03-29 11:11, Michael Stapelberg wrote:
> Package: lintian
> Version: 2.5.10.4
> Severity: wishlist
>
> Attached you can find my first stab at systemd-related checks for
> lintian. While some details in parsing the service files are not
> implemented (see the TODOs in the code), I’d like you to have a look at
> the checks in general. Is there anything that needs to be improved
> before these can be shipped with lintian?
>
> Thanks!
>
Hi,
Thanks for working on Lintian checks, it is much appreciated it. :)
I have annotated some comments with "[style]", which are minor stylistic
guidelines. I know Lintian's code style is a mess in general, so it
describes the style I hope we will eventually reach[1]. :)
Note that I will try to (remember to) not repeat style hints, even if
the same "mistake" is made multiple times.
~Niels
[1] When in doubt, I tend to use:
http://www.eyrie.org/~eagle/notes/style/perl.html
>
> systemd.desc
>
>
> Check-Script: systemd
> Author: Michael Stapelberg
> Abbrev: systemd
Abrrev is optional and is only useful if the name is shorter than the
name in Check-Script (it is an abbrevation of the name, like "manpages"
is aliased "man").
> Type: binary
> Info: Checks various systemd policy things
> Needs-Info: scripts, index, unpacked, file-info
>
> [... some tags ...]
I noticed that there appear to be no use of references (Ref: URL, #bug,
policy X.Y ...). I would recommend finding such so people can quickly
find more information. Links to systemd documentation, specification or
even just a Debian wiki page.
>
> systemd
>
> [...]
>
> use File::Basename;
[style] I like to group Lintian modules separately (and after) external
modules. That basically gives 3-4 groups; pragmas[, constants],
external modules and then Lintian modules.
> use Lintian::Collect::Binary ();
This import is not needed (in general, Perl do not require you to load
modules just because you use instances of classes from that module).
> use Lintian::Relation qw(:constants);
Does not appear to be used?
> use Data::Dumper;
Left-over from debugging?
>
> sub run {
> my ($pkg, $type, $info) = @_;
>
> if ($type eq 'binary') {
This condition will always be true - the "Type:"-field determines what
values $type can have. In this particular case, it is set to "binary".
If you do not need $pkg or $type, then you can replace them with undef. E.g.
my (undef, undef, $info) = @_;
That has the advantage that we know that argument is unused. (As far as
I can tell, $pkg is passed around to various subs but never actually used).
> [...]
>
> for my $file ($info->sorted_index) {
> if ($file =~ m,^etc/tmpfiles\.d/.*\.conf$,) {
> tag('systemd-tmpfiles.d-outside-usr-lib', $file);
[style] The tag sub is a special case in regards to style; it tends to
be treated like a "perl built-in" or "die-like sub" (i.e. no parentheses
unless needed for understanding or semantic reasons). So:
tag 'systemd-tmpfiles.d-outside-usr-lib', $file;
Note if you must use parentheses around tag, a built-in or a "die"-like
sub, please use the same style as for regular subs (see next comment).
> [...]
> if ($file =~ m,/systemd/system/.*\.service$,) {
> check_systemd_service_file($pkg, $info, $file);
[style] For "non-built" sub, we tend to have space between the sub name
and the argument parentheses. For subs that takes no arguments, the
parentheses are omitted where possible. So:
check_systemd_service_file ($pkg, $info, $file);
> [...]
> my @init_scripts = grep(m,^etc/init\.d/.+,, $info->sorted_index);
>
Erh, I think "," might have been a poor choice of regex delimiter here
(as it is also the argument delimiter). For this you could use ":"
(among others) or alternatively call grep with a block:
my @init_scripts = grep {m,^etc/init\.d/.+,} $info->sorted_index;
> [...]
> if ($ships_systemd_file) {
> for my $init_script (@init_scripts) {
> if (grep(/\Q$init_script\E\.service/, @systemd_targets) == 0)
> {
> tag('systemd-no-service-for-init-script', $init_script);
> }
We sometimes use the "reversed" form of if/unless to reduce scope level.
E.g. something like:
tag 'systemd-no-service-for-init-script', $init_script
unless grep /\Q$init_script\E\.service/, @systemd_targets;
There is no hard rule on went; just an FYI that you are free to use it.
> [...]
>
> sub check_init_script {
> my ($pkg, $info, $file) = @_;
$pkg argument does not appear to be used?
>
> my $lsb_source_seen;
> open(my $fh, '<', $info->unpacked($file));
No error handling if open fails! Something as simple as:
or fail "open $file: $!";
is fine.
Secondly, there is no check for file type. If someone (deliberately)
creates $file as a fifo-pipe or a symlink it will DoS or (possibly) read
"host system" files (respectively). Usually, a
Bug#704197: Please review: systemd checks
Package: lintian
Version: 2.5.10.4
Severity: wishlist
Attached you can find my first stab at systemd-related checks for
lintian. While some details in parsing the service files are not
implemented (see the TODOs in the code), I’d like you to have a look at
the checks in general. Is there anything that needs to be improved
before these can be shipped with lintian?
Thanks!
Check-Script: systemd
Author: Michael Stapelberg
Abbrev: systemd
Type: binary
Info: Checks various systemd policy things
Needs-Info: scripts, index, unpacked, file-info
Tag: systemd-service-file-outside-lib
Severity: serious
Certainty: certain
Info: The package ships a systemd service file outside
/lib/systemd/system/
.
System administrators should have the possibility to overwrite a service file
(or parts of it, in newer systemd versions) by placing a file in
/etc/systemd/system, so the canonical location we use for service
files is /lib/systemd/system/.
Tag: systemd-tmpfiles.d-outside-usr-lib
Severity: serious
Certainty: certain
Info: The package ships a systemd tmpfiles.d(5) conf file outside
/usr/lib/tmpfiles.d/
Tag: systemd-service-file-refers-to-obsolete-target
Severity: normal
Certainty: certain
Info: The systemd service file refers to an obsolete target.
.
Some targets are obsolete by now, e.g. syslog.target or dbus.target. For
example, declaring After=syslog.target is unnecessary by now because
syslog is socket-activated and will therefore be started when needed.
Tag: systemd-no-service-for-init-script
Severity: serious
Certainty: certain
Info: The listed init.d script has no systemd equivalent.
.
Systemd has a SysV init.d script compatibility mode. It provides access to
each SysV init.d script as long as there is no native service file with the
same name (e.g. /lib/systemd/system/rsyslog.service corresponds to
/etc/init.d/rsyslog).
.
Your package ships a service file, but for the listed init.d script, there is
no corresponding systemd service file.
Tag: init.d-script-does-not-source-init-functions
Severity: normal
Certainty: certain
Info: The /etc/init.d script does not source
/lib/lsb/init-functions. The systemd package provides
/lib/lsb/init-functions.d/40-systemd to redirect
/etc/init.d/$script calls to systemctl.
.
Please add a line like this to your /etc/init.d script:
.
. /lib/lsb/init-functions
Tag: maintainer-script-calls-systemctl
Severity: normal
Certainty: certain
Info: The maintainer script calls systemctl directly. Actions such as enabling
or starting a service have to be done via update-rc.d or
invoke-rc.d, respectively, which both do the right thing when systemd
is installed/running.
# systemd -- lintian check script -*- perl -*-
#
# Copyright © 2013 Michael Stapelberg
#
# based on the apache2 checks file by:
# Copyright © 2012 Arno Töll
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, you can find it on the World Wide
# Web at http://www.gnu.org/copyleft/gpl.html, or write to the Free
# Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
# MA 02110-1301, USA.
package Lintian::systemd;
use strict;
use warnings;
use File::Basename;
use Lintian::Collect::Binary ();
use Lintian::Tags qw(tag);
use Lintian::Relation qw(:constants);
use Lintian::Util qw(fail);
use Data::Dumper;
sub run {
my ($pkg, $type, $info) = @_;
if ($type eq 'binary') {
# Figure out whether the maintainer of this package did any effort to
# make the package work with systemd. If not, we will not warn in case
# of an init script that has no systemd equivalent, for example.
my $ships_systemd_file = (scalar ( grep { m,/systemd/, }
$info->sorted_index ) > 0);
# An array of names which are provided by the service files.
# This includes Alias= directives, so after parsing
# NetworkManager.service, it will contain NetworkManager and
# network-manager.
my @systemd_targets;
for my $file ($info->sorted_index) {
if ($file =~ m,^etc/tmpfiles\.d/.*\.conf$,) {
tag('systemd-tmpfiles.d-outside-usr-lib', $file);
}
if ($file =~ m,^etc/systemd/system/.*\.service$,) {
tag('systemd-service-file-outside-lib', $file);
}
if ($file =~ m,/systemd/system/.*\.service$,) {
check_systemd_service_file($pkg, $info, $file);
for my $name (extract_service_file_names($pkg, $info
