Thanks for the extensive explanation.
I wonder why I did not do this vrfycmd as the others. Probably because I
want to know the results. Or maybe more likely that it is a patch from
someone else.
Would it be an issue if we use system("cmd args") instead of the array for
you?
/ Ola
PS I have no access to the source right now, hence the not so detailed
answer.
DS
Inguza Technology AB
Sent from a phone
Den 26 jul 2014 00:50 skrev "Ron" <r...@debian.org>:
> On Fri, Jul 25, 2014 at 11:50:02PM +0200, Ola Lundqvist wrote:
> > Hi
> >
> > Thanks for the suggestion. I like the solution. The question is whether
> we
> > should do this for more commands or not.
>
> All of the other *cmd's get passed through cmdaction(), which essentially
> turns them into system("$cmd") -- which means adding arguments to them
> does indeed work, it just possibly gets a little tricky if those arguments
> themselves would want quoting (or unsafe if they came from an untrusted
> source, but that's not an issue in this code).
>
> We could in theory retrofit those like this too, but it would probably be
> a considerably more involved patch to do it, since something like this
> would be ok:
>
> $foocmd = "ls -al";
> @foocmd = ($foocmd);
>
> system(@foocmd);
>
> since that still triggers the 'one argument' version of system(), but
> system(@foocmd, "bar") would end badly in this case.
>
>
> I might be missing something obvious, but in practice I think it would
> mean you'd need to rewrite cmdaction(), both to take its parameters
> differently and call system() differently, and then have to rewrite
> every use of cmdaction too, since some things like $copycmd will take
> multiple parameters of its own beyond what the user configures.
>
> Which probably isn't worth the effort (and risk of new bugs) unless
> someone shows a real use case that really needs it.
>
> Looking at the list of other commands, the only one that jumps out at
> me as this possibly being really useful for too might be $usermailcmd.
> But that would also need fairly extensive changes, and might be better
> done in the context of the other bug, with a patch to make mail really
> be optional.
>
>
> For $vrfycmd, we win and this is easy and safe, because we know the
> scalar version never had more than one argument in it (else it would
> have failed), and it's the only one that gets passed directly to
> system() without going through cmdaction() or something similar.
>
> So I'd probably suggest:
>
> 1) Do $vrfycmd now as a single clean and obvious patch.
> 2) Look at options for $usermailcmd et al. in the context of
> changes needed for the other bug report.
> 3) Worry about the other commands if anyone ever really hits a
> case where the existing usage is troublesome.
>
> Cheers,
> Ron
>
>
> > On Mon, Jul 21, 2014 at 12:27 AM, Ron <r...@debian.org> wrote:
> >
> > > Package: debarchiver
> > > Version: 0.10.1
> > > Severity: normal
> > >
> > > Hi Ola,
> > >
> > > I ran into another quirk today :) The way that $vrfycmd is used means
> > > it's not directly possible to set that to a command that requires some
> > > arguments other than the changes file.
> > >
> > > What I wanted to do was this:
> > >
> > > $vrfycmd = "dscverify --no-default-keyrings --keyring
> > > /var/lib/debarchiver/.gnupg/pubring.gpg";
> > >
> > > So that I could sign packages uploaded to this repo with a different
> key
> > > to the one that is in the DD keyring, to avoid there ever being any
> sort
> > > of accident with them actually getting pushed into a distro upload
> queue
> > > (either by me, or by someone else later).
> > >
> > > It looks like there's a few easy options to fix this though.
> > > My favourite so far is the patch below, which is backward compatible
> for
> > > any existing user config, but lets me instead do:
> > >
> > > @vrfycmd = ("dscverify", "--no-default-keyrings", "--keyring",
> > > "/var/lib/debarchiver/.gnupg/pubring.gpg");
> > >
> > > Which ensures there'll never be any quoting issues for weird arguments.
> > >
> > > Alternatively we could do system("$vrfycmd $cfile"), but that has a
> > > different set of pros and cons.
> > >
> > > It looks like the rest of the *cmd options go through cmdaction() so
> > > they don't have this problem.
> > >
> > > If you like this version, it probably also wants a oneliner in the
> > > sample debarchiver.conf too to note @vrfycmd can be an array now.
> > >
> > > Cheers,
> > > Ron
> > >
> > >
> > > --- /usr/bin/debarchiver 2014-07-21 06:05:08.059769856 +0930
> > > +++ debarchiver 2014-07-21 07:22:11.316828267 +0930
> > > @@ -96,6 +96,7 @@
> > > $rmcmd = "rm -f";
> > > $movecmd = "mv";
> > > $vrfycmd = "dscverify";
> > > +@vrfycmd = ($vrfycmd);
> > > $cachedir = "/var/cache/debarchiver";
> > > $inputdir = "/var/lib/debarchiver/incoming";
> > > $destdir = "/var/lib/debarchiver/dists";
> > > @@ -1742,9 +1743,9 @@
> > > }
> > > # Verify signatures.
> > > if ($verify) {
> > > - if (system($vrfycmd,$cfile)) { # non-zero == verification
> failure
> > > + if (system(@vrfycmd,$cfile)) { # non-zero == verification
> failure
> > > pdebug(4, "Signature verification failed for $cfile");
> > > - $CConf{ERROR} = "$CConf{ERROR}$vrfycmd was not able to
> verify
> > > $cfile.\n";
> > > + $CConf{ERROR} = "$CConf{ERROR}@vrfycmd was not able to
> verify
> > > $cfile.\n";
> > > return "reject";
> > > }
> > > }
> > >
> >
> >
> >
> > --
> > --- Inguza Technology AB --- MSc in Information Technology ----
> > / o...@inguza.com Annebergsslingan 37 \
> > | o...@debian.org 654 65 KARLSTAD |
> > | http://inguza.com/ Mobile: +46 (0)70-332 1551 |
> > \ gpg/f.p.: 7090 A92B 18FE 7994 0C36 4FE4 18A1 B1CF 0FE5 3DD9 /
> > ---------------------------------------------------------------
>
