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 (~thunde...@ppp174-218.static.internode.on.net) 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 Ledger-smb-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel
