Hi David (and Nick),
On Mon, Apr 17, 2017 at 11:02 AM, Erik Huelsmann <ehu...@gmail.com> 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.
------------------------------------------------------------------------------
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
