[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Perl::Critic Tests

(Posting a mail on behalf of David Godfrey who seems to be having trouble posting to the list)

Hi Nick, k-man (irc) mentioned that the dancer team think some of these policies are bad. mst (not a dancer dev per-se, but acrive in #dancer especially in respect to security and quality from what I've noted) suggested another list of recommended policies https://metacpan.org/pod/Perl::Critic::Freenode mst also had this to say about Explicit returns. ProhibitExplicitReturnUndef will mean people use just 'return;' which is context sensitive which will result in the same sort of security bugs seen from CGI.pm So while I personally think the policy is a good one, it probably falls short in it's critique, and we probably need to ensure that any returns we have are correctly scoped to do what we want/expect In short, mst has this to say about Perl::Critic Policies.... 1) most of the builtin ones are crap and don't reflect how perl is written in the real world 2) the Freenode preset projibits actual mistakes we see regularly Further to ProhibitExplicitReturnUndef subroutines without an explicit return will explicitly return whatever their last _expression_ is but return; is 'return wantarray ? () : undef;' and generally, wantarray is a code smell for example sub lookup_row { my ($id) = @_; if (my @row = $dbh->selectrow_array(...) { my %row; @row{qw(id name foo bar)} = @row; return \@row } return undef } yes, I know that's not quite what you'd really write but can we accept it as an example of a pattern? now, consider my %info = (row => lookup_row($id), id => $id); if you replace 'return undef' with 'return', now in the case where the row doesn't exist, you get my $info = (row => 'id', $id => undef); i.e. you just handed control of a hash key to whatever code you got $id from, maybe a user so now I sent your code ?id=is_admin and SURPRISE bug/security hole basically ProhibitExplicitUndef will make you add 'The Perl Jam' CGI.pm-style security holes *back in to your code* this does not, to me, seem like a good idea. 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 Reading https://perlmaven.com/how-to-return-undef-from-a-function, it would appear it's a complicated issue that has no easy solutions. :-(( Once again mst has this to say about parts of that article.... the example where 'return undef' is wrong is ... list context the whole point is 'return undef;' is correct for a *scalar* function 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 (I don't claim my code 100% follows these rules ;) 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 and, well, yes, you will but then my original complaint is that 'return;' turns a scalar returning fuynction into a maybe-scalar-maybe-list-returning function which EW 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 some of my greatest sadnesses over my career have involved maintaing code where I decided 'wantarray' was a good idea and then for scalar-returning functions, 'return;' introduces an implicit wantarray which argh in fact, many years ago somebody tested DBIC-using code for basically exactly the lookup_row bug while trying to exploit it and DBIx::Class containing an explicit 'return undef' was why we passed the security audit (I just went and found said undef just to check I remembered right ;) Some other notes from mst (not exhaustive by any means) .... * ProhibitBooleanGrep is a pessimisation * ProhibitStringySplit is silly and unidiomatic * ProhibitExplicitISA is probably fine because it's rare you actually want to do that * RequireCarping is at best pointless and at worst screws up your error handling * ProhibitInteractiveTest misses the point * RequireCheckedClose # no. really. just no. So 3 out of the first 10 on the list he'd avoid, which suggests to me we should be very careful about what we choose to include in our list. mst is going to have a closer look at that policy list, and will get back to me over the next few days. I'll report back in this thread. Regards David G

On Fri, Apr 14, 2017 at 8:02 PM, Nick Prater <..hidden..> wrote:
As discussed in chat, I've been following up on ehuelsmann's suggestion
to test our code against the Perl::Critic policies recommended by CERT.


Enabling the CERT recommended policies on our 'new' code results in 1050
policy violations:

'Perl::Critic::Policy::Subroutines::RequireFinalReturn' => 293,
'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMagicNumbers' =>
'Perl::Critic::Policy::Modules::RequireVersionVar' => 170,
'Perl::Critic::Policy::ErrorHandling::RequireCarping' => 122,
'Perl::Critic::Policy::Subroutines::ProhibitUnusedPrivateSubroutines' =>
'Perl::Critic::Policy::InputOutput::RequireCheckedSyscalls' => 31,
'Perl::Critic::Policy::Variables::RequireInitializationForLocalVars' =>
'Perl::Critic::Policy::Subroutines::ProhibitBuiltinHomonyms' => 24
=> 23,
'Perl::Critic::Policy::Subroutines::ProhibitExplicitReturnUndef' => 21,
=> 14,
'Perl::Critic::Policy::InputOutput::RequireCheckedOpen' => 11,
'Perl::Critic::Policy::Objects::ProhibitIndirectSyntax' => 11,
'Perl::Critic::Policy::Variables::ProhibitUnusedVariables' => 10,
'Perl::Critic::Policy::TestingAndDebugging::ProhibitNoWarnings' => 10,
'Perl::Critic::Policy::Variables::RequireLocalizedPunctuationVars' =>
'Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles' => 10,
=> 9,
'Perl::Critic::Policy::Subroutines::ProtectPrivateSubs' => 8,
'Perl::Critic::Policy::InputOutput::RequireCheckedClose' => 8,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitBooleanGrep' => 5,
'Perl::Critic::Policy::Variables::RequireLexicalLoopIterators' => 5,
=> 4,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitStringySplit' => 4,
'Perl::Critic::Policy::ControlStructures::ProhibitUnreachableCode' => 3,
'Perl::Critic::Policy::Variables::ProhibitConditionalDeclarations' => 3,
'Perl::Critic::Policy::TestingAndDebugging::ProhibitNoStrict' => 3,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitUniversalIsa' => 3,
'Perl::Critic::Policy::InputOutput::RequireEncodingWithUTF8Layer' => 2,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitStringyEval' => 1,

I've not tested 'old' code, as I view 'new' code as the first priority.

Our current coding guidelines are set out here:

Moving forward, Do we want to make these Perl::Critic policies part of
our coding standard?

If yes, I'm happy to start preparing patches to fix these violations and
add policies to the tests in xt/01.1-critic.t.

Happy Easter!


Nick Prater - NP Broadcast Limited
100 Pitfold Road  Lee  London  SE12 9HY
T: 020 3627 3815  M: 07887 916 458

NP Broadcast Limited is registered in England and Wales number 0794374
VAT Registration Number: GB 129 0388 11

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



http://efficito.com -- Hosted accounting and ERP.
Robust and Flexible. No vendor lock-in.
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