Bug 25491

Created attachment 104851 [details] [review]
Bug 25491: Fix for "Use of uninitialized value" in InstallAuth.pm

This warning was thrown:
    Use of uninitialized value $info{"invalid_username_or_password"}
    in numeric eq (==) at /home/vagrant/kohaclone/C4/InstallAuth.pm
    line 387.

There is the case when hash key can be undefined in numeric comparison.

Fixed by adding additional precheck for
$info{"invalid_username_or_password"} being Perl's "true".

To test:
    1) Go to the first page of the web-installer where it asks to login.
    2) Observe the warning in the log file.
    3) Apply patch.
    4) Repeat step 1.
    7) Check that previous warning suppressed.

I would use "exists" to prevent the autovivication.

If we talk about:

    if ( $info{'invalid_username_or_password'} &&
         $info{'invalid_username_or_password'} == 1) { ...

so this is ok,

there is first-level key check, not a sub-key, so no autovivification
and at the same time, this combines both "existence of the key" check + "Perl false" check (i.e. undef too) on the left "&&" part. 

Autovivification happens for parent keys or array elements in complex data structures if we will go for a subkey from a key, because Perl first needs to create all chain or parent structure/refs (autovivification here) to reach the deepest requested one, but that deepest-requested one does not created.

So for the top-level key it is ok to check existence + Perl non-false in single "$hash{key}" form.


( docs references:

  [https://perldoc.perl.org/perlref.html] "Before this statement, $array[$x]
  may have been undefined. If so, it's automatically defined with a hash
  reference so that we can look up {"foo"} in it. Likewise $array[$x]->{"foo"}
  will automatically get defined with an array reference so that we can look
  up [0] in it. This process is called autovivification." 


  brian d foy's: 
  https://www.effectiveperlprogramming.com/2011/04/understand-autovivification/ 


  my own example (Peter & Salava, pls pay attention too):

  # this is okay, no autovivification and also this prevents "undef" warning:
  if ($info{invalid_username_or_password} &&
      $info{invalid_username_or_password} == 1) { ...

  # this below one is useless, because still no undef-proteciton,
  # so warning etmitted in case key has undef:
  if (exists $info{invalid_username_or_password} &&
             $info{invalid_username_or_password} == 1) { ...

  # this is overreaction yet will work correctly:
  if (exists  $info{invalid_username_or_password} &&
      defined $info{invalid_username_or_password} &&
              $info{invalid_username_or_password} == 1) { ...

  # this is bad, upper level key (config_set) autovivified:
  if ($info{config_set}{invalid_username_or_password} &&
      $info{config_set}{invalid_username_or_password} == 1) { ...

  # this is bad, upper level key (config_set) autovivified, even you have 
  # 'exists', because before 'exists' Perl solves sub-hash deref:
  if (exists $info{config_set}{invalid_username_or_password} &&
             $info{config_set}{invalid_username_or_password} == 1) { ...

)

P.S. sorry for such a long example, I wanted all related people to read & train this and maybe we can refer to this comment one day for my other students :P.

Proof of concept for the local Perl to run & see:

====== vivify.pl ======

#!/usr/bin/env perl
use Modern::Perl; use Data::Dumper qw(Dumper); $|=1; $, = ", ";
$Data::Dumper::Terse = 1; $Data::Dumper::Indent = 0;

my %H = (
    user => undef
); 
say __LINE__, Dumper \%H;
# 8, {'user' => undef}


# this is okay, no autovivification and also this prevents "undef" warning:
say __LINE__,
    $H{user} && $H{user} == 1
        ? 'User exists, defined, and == 1' : 'User absent';
# 13, User absent
say __LINE__, Dumper \%H;
# 17, {'user' => undef}

# this below one is useless, because still no undef-proteciton,
# so warning etmitted in case key has undef:
say __LINE__,
    exists $H{user} && $H{user} == 1
# Use of uninitialized value $H{"user"} in numeric eq (==) at vivify.pl line 23.
        ? 'User exists and == 1' : 'User absent';
# 22, User absent
say __LINE__, Dumper \%H;
# 27, {'user' => undef}


# this is overreaction yet will work correctly:
say __LINE__,
    exists $H{user} && defined $H{user} && $H{user} == 1
        ? 'User name exists, defined, and == 1' : 'User name absent';
# 32, User name absent
say __LINE__, Dumper \%H;
# 36, {'user' => undef}


%H = ();
say __LINE__, Dumper \%H;
# 41, {}

# this is wrong, upper level key (user) unexpectedly autovivified:
say __LINE__,
    $H{user}{name} && $H{user}{name} == 1
        ? 'User name exists and == 1' : 'User name absent';
# 45, User name absent
say __LINE__, Dumper \%H;
# 49, {'user' => {}}


%H = ();
say __LINE__, Dumper \%H;
# 54, {}

# this is wrong, upper level key (user) unexpectedly autovivified:
say __LINE__,
    exists $H{user}{name} && defined $H{user}{name} && $H{user}{name} == 1
        ? 'User exists and == 1' : 'User absent';
# 58, User absent
say __LINE__, Dumper \%H;
# 62, {'user' => {}}

Created attachment 106306 [details] [review]
Bug 25491: Fix for "Use of uninitialized value" in InstallAuth.pm

This warning was thrown:
    Use of uninitialized value $info{"invalid_username_or_password"}
    in numeric eq (==) at /home/vagrant/kohaclone/C4/InstallAuth.pm
    line 387.

There is the case when hash key can be undefined in numeric comparison.

Fixed by adding additional precheck for
$info{"invalid_username_or_password"} being Perl's "true".

To test:
    1) Go to the first page of the web-installer where it asks to login.
    2) Observe the warning in the log file.
    3) Apply patch.
    4) Repeat step 1.
    7) Check that previous warning suppressed.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Well reasoned in the autovivication case.. I agree with your conclusion.

Works as expected, passes qa scripts.

I'm going straight for QA here...

Final note.. was there perhaps some Mentoring going on with this patch.. does it warrant a 'Mentored-by' git trailer ;)

Created attachment 106308 [details] [review]
Bug 25491: Fix for "Use of uninitialized value" in InstallAuth.pm

This warning was thrown:
    Use of uninitialized value $info{"invalid_username_or_password"}
    in numeric eq (==) at /home/vagrant/kohaclone/C4/InstallAuth.pm
    line 387.

There is the case when hash key can be undefined in numeric comparison.

Fixed by adding additional precheck for
$info{"invalid_username_or_password"} being Perl's "true".

To test:
    1) Go to the first page of the web-installer where it asks to login.
    2) Observe the warning in the log file.
    3) Apply patch.
    4) Repeat step 1.
    7) Check that previous warning suppressed.

Mentored-by: Andrew Nugged <nugged@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Pushed to master for 20.11, thanks to everybody involved!

backported to 20.05.x for 20.05.02

backported to 19.11.x for 19.11.08

Backported to 19.05.x branch for 19.05.14