Bug 25491 - Perl warning at the login page of installer
Summary: Perl warning at the login page of installer
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Slava Shishkin
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25790
  Show dependency treegraph
 
Reported: 2020-05-13 17:15 UTC by Peter Vashchuk
Modified: 2020-07-26 15:35 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.02, 19.11.08, 19.05.14


Attachments
Bug 25491: Fix for "Use of uninitialized value" in InstallAuth.pm (1.41 KB, patch)
2020-05-13 17:22 UTC, Slava Shishkin
Details | Diff | Splinter Review
Bug 25491: Fix for "Use of uninitialized value" in InstallAuth.pm (1.48 KB, patch)
2020-06-25 14:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25491: Fix for "Use of uninitialized value" in InstallAuth.pm (1.52 KB, patch)
2020-06-25 15:33 UTC, Slava Shishkin
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2020-05-13 17:15:51 UTC
I encountered this warning at the login page of installer
/cgi-bin/koha/installer/install.pl:

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

This requires a simple fix to check for undef before numeric comparison.
Comment 1 Slava Shishkin 2020-05-13 17:22:05 UTC
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.
Comment 2 Jonathan Druart 2020-06-17 12:44:25 UTC
I would use "exists" to prevent the autovivication.
Comment 3 Andrew Nugged 2020-06-17 18:54:17 UTC
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' => {}}
Comment 4 Martin Renvoize 2020-06-25 14:44:53 UTC
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>
Comment 5 Martin Renvoize 2020-06-25 14:45:56 UTC
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 ;)
Comment 6 Slava Shishkin 2020-06-25 15:33:54 UTC
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>
Comment 7 Jonathan Druart 2020-06-29 11:50:39 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 8 Lucas Gass 2020-07-13 16:01:37 UTC
backported to 20.05.x for 20.05.02
Comment 9 Aleisha Amohia 2020-07-16 04:28:09 UTC
backported to 19.11.x for 19.11.08
Comment 10 Victor Grousset/tuxayo 2020-07-26 15:35:01 UTC
Backported to 19.05.x branch for 19.05.14