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 <niels@thykier.net> 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
Attachment:
systemd
Description: Binary data
Attachment:
systemd.desc
Description: Binary data