Re: Perl::Critic Tests

Hi David (and Nick),

On Mon, Apr 17, 2017 at 11:02 AM, Erik Huelsmann <..hidden..> wrote:

(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

I think it's good and important (as mst is quoted by you below) it's important to review the set of policies we want to adopt. As such I think it's really important to get mst's view on the policy set we're working on. It's also good to consider other available sets of policies out there. In my view, it's good to concentrate on the CERT set of policies (concentrating on security) right now. Then in a next iteration we could (or even should) decide to adopt the Freenode set of policies. However, the Freenode set seems to be concentrating on coding style as well. I'm thinking that there are a number of policies in the default set which we can configure to our coding style as well.

(Remember that in the default set, a lot of policies have been excluded so far because I wanted/needed to get a minimal set of checks up. There's a lot left for investigation even in the default set.)

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

Very important feedback that I hadn't thougt about too much so far. 'wantarray' did feel wrong in many cases although it seems to be very "easy to have" in many others. But I agree with mst that there are cases where the context isn't what it looks like it is to the inexperienced and/or quick eye.

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

While that may be true, there are a number there which are actually quite useful in how we wrote our code so far. At least the default set helped me clean out some problematic code (and enforcing a particular style). The policies we have today *did* find some real bugs.

    2) the Freenode preset prohibits 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

Again, we should introduce the policies we need to be enforced, preferably with some discussion as to the merit of the policy. Additionally, if there are cases where we should be aware of potential problems, we might want to work on a "reviewers guide" to help identify potential pit-falls best judged by humans.

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.

The CERT list (like any other list) isn't the holy grail, IMO. We could go over the list now to see which ones we decide to explicitly not accept as ours and exclude from our testing?

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.

Ok. Lets see what mst has to say and make our choices in part on that. As for "RequireCheckedClose", I can't imagine what we would want to do with that, other than logging. RequireCarping? Same comment as his.

Bye,

Erik.

http://efficito.com -- Hosted accounting and ERP.

Robust and Flexible. No vendor lock-in.