Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm
Summary: Use of uninitialized values $max and $min in C4/Tags.pm
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 3.16
Hardware: All All
: P5 - low trivial (vote)
Assignee: Marc Véron
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-14 07:04 UTC by Marc Véron
Modified: 2015-12-03 22:11 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm (1004 bytes, patch)
2014-09-20 18:29 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm (1.08 KB, patch)
2014-10-22 16:47 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 12916 - Missing Test to demonstrate warnings. (1.36 KB, patch)
2014-10-22 19:52 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12916 - Missing Test to demonstrate warnings. (1.36 KB, patch)
2014-10-22 19:58 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12916 - Missing Test to demonstrate warnings. (1.36 KB, patch)
2014-10-22 22:50 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm (1.38 KB, patch)
2014-10-23 01:01 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12916 - Missing Test to demonstrate warnings. (1.41 KB, patch)
2014-10-25 03:22 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm (1.43 KB, patch)
2014-10-25 03:23 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 12916 - Missing Test to demonstrate warnings. (1.47 KB, patch)
2014-10-25 13:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm (1.53 KB, patch)
2014-10-25 13:16 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2014-09-14 07:04:45 UTC
If tag list is empty, sub stratify_tags issues a warning in opac-error.log because of uninitialized values $max and $min in C4/Tags.pm, line 600
Comment 1 Marc Véron 2014-09-20 18:29:46 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2014-10-20 13:17:19 UTC
Still applies on current master: 3.17.00.029
Comment 3 Owen Leonard 2014-10-22 16:47:57 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2014-10-22 19:06:24 UTC
Comment on attachment 32590 [details] [review]
[SIGNED-OFF] Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm

Review of attachment 32590 [details] [review]:
-----------------------------------------------------------------

::: C4/Tags.pm
@@ +589,4 @@
>  sub stratify_tags {
>      my ( $strata, $tags ) = @_;
>  
> +    my ( $min, $max ) = ( 0, 0 );

I'm not sure this is the correct solution.
Look at 595. There is a !defined($min) check.
The problem is there is nothing in the @$tags.
By initializing to 0, if there are tags, and the minimum is 5, the minimum is never set correctly.
Similarly, if there are tags, and the maximum is negative, the maximum is never set correctly.
Comment 5 Mark Tompsett 2014-10-22 19:52:35 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2014-10-22 19:53:35 UTC
After further code reading and test, the proposed solution is adequate. Here are the missing tests to make testing trivial. :)
Comment 7 Mark Tompsett 2014-10-22 19:58:08 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2014-10-22 22:50:44 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2014-10-22 22:58:41 UTC
Sorry, prove -v t/db_dependent/Tags.t fails with signed off patch.
Comment 10 Mark Tompsett 2014-10-23 01:01:53 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2014-10-25 03:22:22 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2014-10-25 03:23:35 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2014-10-25 13:16:07 UTC
Created attachment 32732 [details] [review]
[PASSED QA] Bug 12916 - Missing Test to demonstrate warnings.

Modifying C4/Tags.pm should have tests.

TEST PLAN
---------
1) Apply this test patch only.
2) prove -v t/db_dependent/Tags.t
   -- should see two warnings.
3) Apply the first patch.
4) prove -v t/db_dependent/Tags.t
   -- no warnings.

Signed-off-by: Chris <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2014-10-25 13:16:20 UTC
Created attachment 32733 [details] [review]
[PASSED QA] Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm

When the stratify_tags function is called without any tags, the
$min and $max variables are left undefined, which triggers a
warning message about uninitialized values in the subtraction.

TEST PLAN
---------
1) prove -v t/db_dependent/Tags.t
   -- should succeed, but there is no test for empty tags.
2) Make sure that you have no tags
3) In Opac, go to 'Tag cloud'
4) Check the opac error log file
   (e.g. ~/koha-dev/var/log/koha-opac-error_log) for entries
   regarding uninitialized values $max and $min in C4/Tags.pm
5) apply this patch
6) prove -v t/db_dependent/Tags.t
   -- should still succeed, though no tests are added.
7) Reload page 'Tag cloud'
   -- Result: No additional warnings in opac error log file.

Signed-off-by: Chris <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
New and old tests pass. Works as described.
Comment 15 Tomás Cohen Arazi 2014-10-30 03:34:59 UTC
Patches pushed to master.

Thanks Marc and Mark!
Comment 16 Mark Tompsett 2015-02-09 05:48:59 UTC
Could this be included in 3.16.x? It applies cleanly.