(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 <n...@npbroadcast.com> 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.
> See:
>
> https://gist.github.com/briandfoy/4525877
> https://www.securecoding.cert.org/confluence/display/perl/
> SEI+CERT+Perl+Coding+Standard
> https://matrix.to/#/!qyoLumPqusaXqFJNyK:matrix.org/
> $14917685211662024izFqM:matrix.org
> https://matrix.to/#/!qyoLumPqusaXqFJNyK:matrix.org/
> $1491849623220407yWMci:matrix.org
>
> 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' =>
> 172,
> 'Perl::Critic::Policy::Modules::RequireVersionVar' => 170,
> 'Perl::Critic::Policy::ErrorHandling::RequireCarping' => 122,
> 'Perl::Critic::Policy::Subroutines::ProhibitUnusedPrivateSubroutines' =>
> 34,
> 'Perl::Critic::Policy::InputOutput::RequireCheckedSyscalls' => 31,
> 'Perl::Critic::Policy::Variables::RequireInitializationForLocalVars' =>
> 26,
> 'Perl::Critic::Policy::Subroutines::ProhibitBuiltinHomonyms' => 24
> 'Perl::Critic::Policy::ValuesAndExpressions::
> ProhibitMixedBooleanOperators'
> => 23,
> 'Perl::Critic::Policy::Subroutines::ProhibitExplicitReturnUndef' => 21,
> 'Perl::Critic::Policy::RegularExpressions::ProhibitCaptureWithoutTest'
> => 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' =>
> 10,
> 'Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles' => 10,
> 'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMismatchedOperators'
> => 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,
> 'Perl::Critic::Policy::ValuesAndExpressions::
> ProhibitCommaSeparatedStatements'
> => 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:
> https://ledgersmb.org/community-guide/community-guide/development/coding-
> guidelines/perl-coding-guidelines
>
> 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
>
> --
> 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
> Ledger-smb-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel
>
--
Bye,
Erik.
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
Ledger-smb-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel
