New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace explicit undef returns with bare returns #8

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@paultcochrane
Contributor

paultcochrane commented Nov 3, 2017

Explicitly returning undef can cause a subtle bug, where the return
value evaluates as true instead of false as intended. It turns out that
using the default return behaviour is actually safer, and hence why this
is marked as a problem at the default perlcritic level.

This could be considered a controversial change, nevertheless, I thought I'd submit it just in case you want perlcritic compatability. If you don't want the change, just close unmerged, I won't be offended :-)

Replace explicit undef returns with bare returns
Explicitly returning undef can cause a subtle bug, where the return
value evaluates as true instead of false as intended.  It turns out that
using the default return behaviour is actually safer, and hence why this
is marked as a problem at the default perlcritic level.
@preaction

This comment has been minimized.

Show comment
Hide comment
@preaction

preaction Nov 3, 2017

Owner

For the external API, this 100% must be done how Minion::Backend::Pg (the official backend) does it. So, if it's got bare returns, we can do bare returns. If it has explicit undef, we've got to do explicit undef. The surrounding Minion classes expect the sub to return either a list or a scalar, and we've got to give it what it expects.

The problem with defaulting to list context is that it can also create undesired behavior: Bare return can cause problems when a single, explicit value was expected:

my %vars = (
    worker_info => $minion->worker(1)->info,
);

With return undef, that does what we expect: Worker 1 doesn't exist. With return, it turns into an odd-sized list (which Perl will warn us about). This was the cause of the Bugzilla security vulnerability a couple years back, and a few years worth of under-informed security-related bashing of Perl by Netanel Rubin.

I recall sri being proactive when that Bugzilla issue hit, so he probably went through and made sure every method had a single expected return context.

For the internal API (sub _update), it doesn't largely matter, but I tend to make sure that subroutine names that seem singular return singular things. _update doesn't sound like it should return anything, but it's only working with one thing, so it makes sense that one thing should be returned (the thing after we've updated it).

So, if Minion::Backend::Pg also does bare return, then we must. Otherwise, we must keep it as-is to keep compatibility with the API.

Owner

preaction commented Nov 3, 2017

For the external API, this 100% must be done how Minion::Backend::Pg (the official backend) does it. So, if it's got bare returns, we can do bare returns. If it has explicit undef, we've got to do explicit undef. The surrounding Minion classes expect the sub to return either a list or a scalar, and we've got to give it what it expects.

The problem with defaulting to list context is that it can also create undesired behavior: Bare return can cause problems when a single, explicit value was expected:

my %vars = (
    worker_info => $minion->worker(1)->info,
);

With return undef, that does what we expect: Worker 1 doesn't exist. With return, it turns into an odd-sized list (which Perl will warn us about). This was the cause of the Bugzilla security vulnerability a couple years back, and a few years worth of under-informed security-related bashing of Perl by Netanel Rubin.

I recall sri being proactive when that Bugzilla issue hit, so he probably went through and made sure every method had a single expected return context.

For the internal API (sub _update), it doesn't largely matter, but I tend to make sure that subroutine names that seem singular return singular things. _update doesn't sound like it should return anything, but it's only working with one thing, so it makes sense that one thing should be returned (the thing after we've updated it).

So, if Minion::Backend::Pg also does bare return, then we must. Otherwise, we must keep it as-is to keep compatibility with the API.

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Nov 3, 2017

Contributor

Wow, what an awesome answer! Clear, precise, and with lots of background information, mate that's fantastic! I'll have a look at Minion::Backend::Pg to see what it does, and give feedback here as to what I find out.

Contributor

paultcochrane commented Nov 3, 2017

Wow, what an awesome answer! Clear, precise, and with lots of background information, mate that's fantastic! I'll have a look at Minion::Backend::Pg to see what it does, and give feedback here as to what I find out.

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Nov 4, 2017

Contributor

I've tried to grok the code in https://github.com/kraih/minion/blob/master/lib/Minion/Backend/Pg.pm and it looks like explicit undefs should be returned. In two locations (https://github.com/kraih/minion/blob/master/lib/Minion/Backend/Pg.pm#L23 and https://github.com/kraih/minion/blob/master/lib/Minion/Backend/Pg.pm#L264) it explicitly returns undef. Many of the other methods return the value of a query directly, which (I'm guessing) are probably also be undef. Hence I think it's best in this case to leave the undefs where they are. Thanks for the pointers about bugs resulting from bare return. That's really interesting; I didn't know how potentially dangerous bare returns were. It's good to know that one has to be very careful with return values, possibly more than instinct would suggest.

Contributor

paultcochrane commented Nov 4, 2017

I've tried to grok the code in https://github.com/kraih/minion/blob/master/lib/Minion/Backend/Pg.pm and it looks like explicit undefs should be returned. In two locations (https://github.com/kraih/minion/blob/master/lib/Minion/Backend/Pg.pm#L23 and https://github.com/kraih/minion/blob/master/lib/Minion/Backend/Pg.pm#L264) it explicitly returns undef. Many of the other methods return the value of a query directly, which (I'm guessing) are probably also be undef. Hence I think it's best in this case to leave the undefs where they are. Thanks for the pointers about bugs resulting from bare return. That's really interesting; I didn't know how potentially dangerous bare returns were. It's good to know that one has to be very careful with return values, possibly more than instinct would suggest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment