[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Perl::Critic Tests
- Subject: Re: Perl::Critic Tests
- From: Jason Lewis <..hidden..>
- Date: Mon, 17 Apr 2017 21:00:18 +1000
Hi Guys,
This question tread lead me to ask about using Perl::Critic on Dancer2
code on #dancer on IRC. mst had some things to say about it and I
thought you might find it relevant.
Later David G joined the discussion:
2017-04-15
11:09:13 k-man do we run perl critic on the d2 code base?
11:09:28 k-man also, would it be relevant to use this on d2?
https://gist.github.com/briandfoy/4525877
11:09:37 @mst k-man: do you have a set of perl critic settings
that make sense?
11:09:47 @mst because the standard ones are useless for
production code
11:10:16 k-man mst, none at all. I'm not an expert in this
11:10:45 @mst a bunch of the poliicies recommended in that gist
are actively unsafe
11:11:22 k-man there is a xt/perlcritic.rc file i see
11:13:46 k-man mst, could you point me to one of the policies that
is unsafe in there and maybe point me to why its unsafe?
11:14:00 @mst
https://gist.github.com/briandfoy/4525877#file-cert-perl-critic-policies-txt-L25
11:14:00 {yaURLs}~> http://tinyurl.com/mynmr79 (gist.github.com)
11:14:05 @mst ProhibitExplicitReturnUndef
11:14:30 @mst will mean people use just 'return;' which is
context sensitive which will result in the same sort of security bugs
seen from CGI.pm
11:14:53 k-man ah i see
11:15:27 k-man have you looked into that specific list before?
11:16:13 @mst nope
11:16:26 @mst I'm pretty good at spotting problems with perl
critic policies
11:16:30 @mst due to them all being terrible ;)
11:16:48 k-man mst, I was wondering all this because i was reading
the LedgerSMB mailing list, and they were discussing using that list for
perlcritic checking their code
11:17:07 @mst that's a shame
11:17:16 @mst the ledgersmb project used to come to me for perl
advuce quite regularly
11:17:27 k-man oh
11:17:34 k-man they don't any more?
11:17:49 @mst not for a while, I think the person who mostly did
that may have moved on
11:17:57 k-man i see
11:18:14 k-man do you care to comment on their plan if I point you
to the post in question?
11:25:28 fuzzix PBP and critic policies are great if you want to
get roasted for your Perl in ##c
11:38:04 k-man so, perlcritic on lib/Dancer2.pm suggest our
$runner is a bad idea
11:39:37 @mst based on which policy?
11:40:17 k-man Variables::ProhibitPackageVars
11:40:38 @mst well, that's bullshit
11:40:49 k-man ok! glad I raised it then
11:40:51 @mst it's a global variable. that's the correct way to
implement it.
11:41:14 k-man it goes on to say it suggests using all caps for
global vars if they are needed
11:41:27 k-man and then it won't complain about it
11:42:20 @mst that's actually wrong
11:42:25 @mst perlstyle says all caps are for constants
11:42:29 @mst globals should be Foo_Bar
11:42:40 @mst so, yeah, theoretically, it should be 'our $Runner'
11:42:49 @mst but this is the sort of stupid shit that makes me
not want to use perlcritic
11:42:53 k-man heh
11:44:40 k-man the percritic advice comes from Conway, is that a
style difference between Conway and perlstyle? (what is perlstyle)?
11:44:59 k-man ok, ignore that last question, found it
12:17:05 k-man mst, so is that list even worth looking at and
culling the bad rules?
12:17:17 @mst maybe
12:17:17 k-man mst, or is there a recommended list to use as a
starting point?
12:17:56 k-man mst, they asked me if you'd review it ot suggest a
better list
12:18:14 @mst the freenode policies are a start
12:29:57 --> dcg (~..hidden..)
has joined #dancer
12:38:49 dcg mst: hi
12:38:55 @mst dcg: ay up
12:39:34 dcg I haven't seen the backlog here (Client wasn't
connected) but have a couple of questions about Perl::Critic policies
12:39:55 @mst short version
12:40:04 dcg k-man: mentioned that you (in particular) don't
like ProhibitExplicitReturnUndef
12:40:06 @mst 1) most of the builtin ones are crap and don't
reflect how perl is written in the real world
12:40:16 @mst 2) the Freenode preset projibits actual mistakes we
see regularl;y
12:41:23 @mst well, yes, that one introduces bugs and security holes
12:41:49 @mst which suggests that whoever created the list with
it on isn't any good at perl
12:42:19 dcg Ok, I'm interested in how, I thought all subs have
an implicit return that is equivalent to "return;"
12:43:11 k-man now that I have sufficiently stired the pot, I have
to go now
12:43:17 k-man :)
12:43:18 @mst subroutines without an explicit return will
explicitly return whatever their last expression is
12:43:18 k-man ttyl
12:43:22 @mst but
12:43:24 @mst return;
12:43:26 dcg (note: I code in many languages, and freely admit
my indepth perl knowledge is sadly lacking, and sometimes clouded by
other languages)
12:43:28 @mst is 'return wantarray ? () : undef;'
12:43:40 @mst generally, wantarray is a code smell
12:44:04 dcg k-man: thanks for that <eyeroll>
12:44:16 @mst for example
12:44:37 k-man dcg, i'll be back later and read the backlog, I
promise!
12:45:14 @mst sub lookup_row { my ($id) = @_; if (my @row =
$dbh->selectrow_array(...) { my %row; @row{qw(id name foo bar)} = @row;
return \@row } return undef }
12:45:34 @mst dcg: yes, I know that's not quite what you'd really
write but can we accept it as an example of a pattern?
12:46:00 @mst now, consider
12:46:24 @mst my %info = (row => lookup_row($id), id => $id);
12:46:45 @mst if you replace 'return undef' with 'return', now in
the case where the row doesn't exist, you get
12:46:52 @mst my $info = (row => 'id', $id => undef);
12:47:03 @mst i.e. you just handed control of a hash key to
whatever code you got $id from, maybe a user
12:47:10 @mst so now I sent your code ?id=is_admin
12:47:19 @mst and SURPRISE bug/security hole
12:47:47 @mst basically ProhibitExplicitUndef will make you add
'The Perl Jam' CGI.pm-style security holes *back in to your code*
12:47:56 @mst this does not, to me, seem like a good idea.
12:48:56 @mst (brb)
12:49:17 @mst dcg: ^^
12:55:51 dcg mst: Ok, I sortof see the problem.
12:55:51 dcg I wasn't aware that perl returned the kitchen sink
like that.
12:55:51 dcg Would a reasonable requirement be that ALL subs
have an explicit return, with an explicit value returned?
12:55:52 dcg ie: 'return ($retval);'
12:55:52 dcg Even if $retval is UNDEF if that's what it really
should be?
12:56:19 @mst 'returned the kitchen sink' ?
12:57:12 dcg Well, it seems from your example, that "too much"
is returned if using a simple "return;" hence the kitchen sink comment
12:57:17 @mst no
12:57:29 @mst *nothing* is returned
12:57:51 @mst again: return; is return wantarray ? () : undef;
12:58:19 dcg Ah, sorry, Need to re-read your example.
12:59:52 dcg Yep, on re-reading I caught what I missed the first
scan around :-(
13:00:22 @mst right
13:00:38 @mst so this is what bugs me about policies - people
blindly 'fix' what it tells them to and can very easily introduce bugs
in the process
13:04:23 dcg mst:
https://perlmaven.com/how-to-return-undef-from-a-function suggests that
both "return;" and "return undef;" have their own issues/side-affects
and that there really is no easy way out.
13:04:23 dcg Do you have any thoughts on that article?
13:04:23 {yaURLs}~> http://tinyurl.com/lao5e9s (perlmaven.com)
13:06:00 @mst oh gods perlmaven
13:06:11 dcg Yeah, Yeah, I know......
13:06:46 @mst yeah, no, because the example where 'return undef'
is wrong is ... list context
13:06:54 dcg Don't worry, I take EVERYTHING on the web with a
tablespoon of salt, hence the questions I'm asking now ;-)
13:06:58 @mst the whole point is 'return undef;' is correct for a
*scalar* function
13:07:26 @mst for a list context function, you use 'return;' or
'return ();' which both do the same thing, but I tend towards 'return
();' to be explicit when I remember
13:07:35 @mst (I don't claim my code 100% follows these rules ;)
13:08:40 @mst all that article points out against 'return undef'
is that if you use a scalar return in a lst-returning function you will
confuse everybody
13:08:43 @mst and, well, yes, you will
13:09:10 @mst but then my original complaint is that 'return;'
turns a scalar returning fuynction into a
maybe-scalar-maybe-list-returning function
13:09:14 @mst which EW
13:09:39 dcg mst: Ok, that makes sense. And I totally agree.
13:11:06 @mst baaasically, the world is a better place if you
pretend that all perl routines are *either* scalar returning *or*
list-returning, only ever use a given subroutine one way, and define
your subroutines to be used one of the two ways
13:11:35 @mst some of my greatest sadnesses over my career have
involved maintaing code where I decided 'wantarray' was a good idea
13:11:46 dcg My first language was TurboPascal 1.x and most
other languages I use are strongly typed (or in the cast of assembly
everything is binary anyway)
13:11:46 dcg This means I can tend to overlook and oversimplify
the vagaries of typing in perl etc
13:11:50 @mst and then for scalar-returning functions, 'return;'
introduces an implicit wantarray which argh
13:12:29 @mst perl is weird. it is, once you reach a certain
point, actually mostly the same sort of weird uniformly
13:12:44 @mst (but you need to have a level of fluency to see
*how* it's mostly the same weird0
13:12:48 @mst but, still, weird :)
13:13:31 dcg Ok, Thanks very much for your input there, It helps
me (and hopefully #ledgersmb) significantly.
13:15:07 @mst in fact, many years ago somebody tested DBIC-using
code for basically exactly the lookup_row bug while trying to exploit it
13:15:19 @mst and DBIx::Class containing an explicit 'return
undef' was why we passed the security audit
13:15:32 @mst (I just went and found said undef just to check I
remembered right ;)
13:16:08 dcg Before I go spend some easter time with SWMBO were
there other policies in this link https://gist.github.com/briandfoy/4525877
13:16:08 {yaURLs}~> http://tinyurl.com/moaajj7 (gist.github.com)
13:16:47 dcg Ah, cool, that's a good datapoint, the fact that
"return undef;" helped you pass an audit
13:17:29 @mst I think about half of those make some vague sort of
sense
13:17:34 @mst and the other half are either pointless or harmful
13:18:07 dcg mst: Sorry, incomplete request there.
13:18:07 dcg Other Policies at that link you consider to be "Bad"
13:19:03 @mst yes, probably
13:19:14 @mst going through each one in detail is quite a bit of
effort though
13:20:45 dcg Fair enough, If any jump out at you we'd be
interested to know, but I'm not asking you to spend any significant time
(unless your at a loss for something to do :-o)
13:20:58 @mst well ProhibitBooleanGrep is a pessimisation
13:21:14 @mst ProhibitStringySplit is silly and unidiomatic
13:21:33 @mst ProhibitExplicitISA is probably fine because it's
rare you actually want to do that
13:22:00 @mst RequireCarping is at best pointless and at worst
screws up your error handling
13:24:02 @mst so ... three I'd avoid out of the first ten
13:24:14 @mst which is why I'm not convinced of the list in general
13:24:41 @mst though 4,5,7,8,10 all seem pretty reasonable to me
for most code
13:25:24 @mst ProhibitInteractiveTest misses the point
13:25:35 @mst RequireCheckedClose # no. really. just no.
13:26:14 @mst meh, this is just depressing
13:28:30 dcg mst: Once again thanks for your knowledge, It's
invaluable IMHO
13:28:52 dcg for RequireCheckedClose I'm struggling to
understand that one as well.
13:30:31 dcg for a webapp in particular does it really matter in
most cases (reading a file) if you could close it without an error?
13:30:31 dcg What conditions would cause an error on close
anyway? The file no-longer being accessible?
13:31:14 * mst cries
13:31:25 @mst you're making me want to try and bikeshed a decent
set of policies
13:31:27 @mst and oh gods
13:31:50 @mst dcg: btw, this is the sort of stuff metatrontech
used to poke me for and I was always happy to
13:31:54 @mst however
13:32:09 dcg Oh, dear, I'm sooooo sorry. I didn't mean to ruin
easter for you ;-)
13:34:35 @mst hmm
13:34:46 @mst actually, the rest of the list is holding up
better, roughly
13:34:54 @mst poke me tomorrow and I'll try and go through the
rest of it
13:35:12 @mst I'm starting to think that about 2/3 of this list
would genuinely be a good start
13:35:46 dcg That would be awesome, if you have the time.
13:37:20 dcg Feel free to drop in at #ledgersmb or hit me up in
a PM on freenode (nick is dcg_mx)
13:37:29 @mst aight, shall do
13:37:42 dcg once again, thanks for your expertise.
--
Jason Lewis
http://emacstragic.net
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Ledger-smb-devel mailing list
..hidden..
https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel