Bug 11188 - Make gather_print_notices.pl die on failed open()
Summary: Make gather_print_notices.pl die on failed open()
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Magnus Enger
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-02 18:16 UTC by Magnus Enger
Modified: 2015-06-04 23:24 UTC (History)
3 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 11188 - Make gather_print_notices.pl die on failed open() (3.30 KB, patch)
2013-11-26 22:31 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 11188 - Make gather_print_notices.pl die on failed open() (3.35 KB, patch)
2013-12-29 06:01 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 11188 - Make gather_print_notices.pl die on failed open() (3.47 KB, patch)
2013-12-29 08:15 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 Magnus Enger 2013-11-02 18:16:10 UTC
Currently, gather_print_notices.pl tries to open a file to write notices to, but it does not check that the open() succeeded. This can result in the following error error, if you point at a location you do not have write access to:

print() on closed filehandle $OUTPUT at /usr/share/koha/bin/cronjobs/gather_print_notices.pl line 129.

But the script proceeds on its merry way, and marks the "pending" messages it found as "sent", which means you can't re-run the script with another location and gather up the notices you meant to gather up. 

I'll propose to replace the current "open(...)" with an "open(...) or die()". 

(I'll probably do a patch for this during training on Monday, so please don't fix it before that :-)
Comment 1 Magnus Enger 2013-11-26 22:31:17 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2013-12-29 06:01:52 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2013-12-29 08:15:22 UTC
Created attachment 23855 [details] [review]
[PASSED QA] Bug 11188 - Make gather_print_notices.pl die on failed open()

Problem:
If you tell gather_print_notices.pl to write output to a location
you do not have write access to, it will silently fail to write the
data, but still mark unsent messages as sent.

Solution:
This patch adds two lines of defense:
1. Check that the location given for the output is writable
2. use "open() or die" instead of just "open()" when writing the
   output
The first measure should catch most of the potential errors, but
I guess a directory can be writable, but the open() still can fail
because the disk is full or something similar.

To test:
- Make sure you have some unsent messages in the message_queue table,
  that do not have an email adress
- Apply the patch
- Run the script, pointing at a location you do not have access to
  write to. Check that the script exits with an appropriate error
  message, and that the unsent messages are still unsent. Do this
  both with and without the -s option.
- To fake passing the first line of defence, comment out line 62
  and put this in instead:
  if ( !$output_directory || !-d $output_directory ) {
- Run the script again as above, check you get an appropriate
  error and that the message queue is not touched
- Reset line 62 to how it was
- Run the script against a directory you do have access to write to
  and check that output is produced as expected and that messages
  are marked as sent
- Sign off

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
Works as described.
Comment 4 Galen Charlton 2013-12-30 15:14:23 UTC
Pushed to master.  Thanks, Magnus!
Comment 5 Fridolin Somers 2014-01-27 13:31:11 UTC
Patch pushed to 3.14.x, will be in 3.14.3.
Comment 6 Tomás Cohen Arazi 2014-02-07 17:11:19 UTC
This patch has been pushed to 3.12.x, will be in 3.12.11.

Thanks Magnus!