Bug 23723 - Using exit inside eval to stop sending output to the browser doesn't work under Plack
Summary: Using exit inside eval to stop sending output to the browser doesn't work und...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Dobrica Pavlinusic
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-10-02 19:31 UTC by Dobrica Pavlinusic
Modified: 2019-11-08 14:24 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.05


Attachments
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack (1.38 KB, patch)
2019-10-03 07:40 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack (1.44 KB, patch)
2019-10-03 09:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack (1.06 KB, patch)
2019-10-03 10:08 UTC, Theodoros Theodoropoulos
Details | Diff | Splinter Review
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack (1.96 KB, patch)
2019-10-03 10:50 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack (2.02 KB, patch)
2019-10-03 11:18 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack (2.07 KB, patch)
2019-10-03 12:04 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Dobrica Pavlinusic 2019-10-02 19:31:27 UTC
When fixing Bug 23589 Theodoros Theodoropoulos noticed that we are sending headers and html after pdf output to browser. It seems that using just exit isn't enough to stop plack from generating headers and html page. This was noticed back in 2011 with Bug 5630 which added safe_exit in C4::Auth. It seems that we need to use something similar throughout all code which gets invoked with plack to fix this.

I'm opening this bug as placeholder until I do more testing and figure out which parts need change and we have a lot of exit; in code:

git grep 'exit;' | wc -l
352
Comment 1 Dobrica Pavlinusic 2019-10-02 20:55:11 UTC
Problem occurs only if we use exit inside eval block like:

eval {
	warn "in eval";
	exit;
};
warn "after eval";

Under CGI, this would print just "in eval". Bacause of interaction of CGI::Compile which wraps system exit under plack we get both lines.
Comment 2 Dobrica Pavlinusic 2019-10-03 07:40:19 UTC
Created attachment 93527 [details] [review]
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack

When fixing Bug 23589 Theodoros Theodoropoulos noticed that we are sending
headers and html after pdf output to browser.

Using exit inside eval block doesn't stop plack from generating
headers and html page after exit since CGI::Compile will catch
exit but doesn't stop emiting output. Example is:

eval {
	warn "in eval";
	exit;
};
warn "after eval";

Under CGI, this would print just "in eval", but under plack we get both lines
and thus generate additional header and html after we already sent pdf data.
Comment 3 Dobrica Pavlinusic 2019-10-03 07:41:51 UTC
I did check all places in code where we use eval and it seems that only place where we have also exit is this one, so situation is not as bad as I though initially.
Comment 4 Brendan Gallagher 2019-10-03 09:37:02 UTC
Created attachment 93541 [details] [review]
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack

When fixing Bug 23589 Theodoros Theodoropoulos noticed that we are sending
headers and html after pdf output to browser.

Using exit inside eval block doesn't stop plack from generating
headers and html page after exit since CGI::Compile will catch
exit but doesn't stop emiting output. Example is:

eval {
	warn "in eval";
	exit;
};
warn "after eval";

Under CGI, this would print just "in eval", but under plack we get both lines
and thus generate additional header and html after we already sent pdf data.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 5 Theodoros Theodoropoulos 2019-10-03 09:47:03 UTC
Dobrica, in the patch you submitted, you tackle the issue only in opac-discharge.pl
The same code exists also in discharge.pl (where it was originally discovered). Please include the fix for that as well in your patch in order to sign it off.

Thanks in advance!
Comment 6 Theodoros Theodoropoulos 2019-10-03 10:08:27 UTC
Created attachment 93551 [details] [review]
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack

Also fixes ./src/members/discharge.pl
Comment 7 Theodoros Theodoropoulos 2019-10-03 10:15:27 UTC
Extending the patch was trivial, so I gave it a go myself. Please check if it works for you too (it worked for me).

Brendan, I'm changing status to "Needs Sign Off" (the original patch indeed tackled the problem, but not in all places).

[I'm not a senior koha dev (like most of you), so please forgive me if I did something wrong. Full credits for identifying the real cause and fixing the problem go to Dobrica!]
Comment 8 Dobrica Pavlinusic 2019-10-03 10:50:52 UTC
Created attachment 93552 [details] [review]
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack

When fixing Bug 23589 Theodoros Theodoropoulos noticed that we are sending
headers and html after pdf output to browser.

Using exit inside eval block doesn't stop plack from generating
headers and html page after exit since CGI::Compile will catch
exit but doesn't stop emiting output. Example is:

eval {
	warn "in eval";
	exit;
};
warn "after eval";

Under CGI, this would print just "in eval", but under plack we get both lines
and thus generate additional header and html after we already sent pdf data.


Current status: Needs Signoff
Comment 9 Dobrica Pavlinusic 2019-10-03 10:52:10 UTC
Patch should include also commit message, so I replaced it with full version which you can now sign-off :-)
Comment 10 ByWater Sandboxes 2019-10-03 11:18:12 UTC
Created attachment 93556 [details] [review]
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack

When fixing Bug 23589 Theodoros Theodoropoulos noticed that we are sending
headers and html after pdf output to browser.

Using exit inside eval block doesn't stop plack from generating
headers and html page after exit since CGI::Compile will catch
exit but doesn't stop emiting output. Example is:

eval {
	warn "in eval";
	exit;
};
warn "after eval";

Under CGI, this would print just "in eval", but under plack we get both lines
and thus generate additional header and html after we already sent pdf data.

Signed-off-by: Theodoros Theodoropoulos <theod@lib.auth.gr>
Comment 11 Theodoros Theodoropoulos 2019-10-03 11:35:38 UTC
I used ByWater Solutions' sandboxes because they seem to use Plack (required for the bug to appear).

Seems to work OK, but QA may complain that there is no test plan for the patch.

Because the bug relates only to code found in discharges, I would propose something like:

1/ make sure Plack is used in the system
2/ go to System Preferences and set useDischarge: Allow
3/ create a patron (or select a patron) and go to discharges -> generate discharge
4/ the produced file is not a proper PDF as is contains headers and html code in the end

5/ Apply patch (restart starman/apache ?)
6/ go to discharges-> generate discharge
7/ the bug is fixed and produced file is proper PDF

(Something similar could be described for opac-discharge)

ByWater Solutions' sandbox sign-off procedure did not change the status to Signed off... Doing it manually.
Comment 12 Katrin Fischer 2019-10-03 11:38:48 UTC
Thx Dobrica (missing you here in Marseille) and Theodoros!
Comment 13 Katrin Fischer 2019-10-03 11:54:52 UTC
I must be missing something - because my PDF look great without the patch :(
Comment 14 Nick Clemens 2019-10-03 12:04:44 UTC
Created attachment 93580 [details] [review]
Bug 23723: using exit inside eval to stop sending output to browser doesn't work under plack

When fixing Bug 23589 Theodoros Theodoropoulos noticed that we are sending
headers and html after pdf output to browser.

Using exit inside eval block doesn't stop plack from generating
headers and html page after exit since CGI::Compile will catch
exit but doesn't stop emiting output. Example is:

eval {
	warn "in eval";
	exit;
};
warn "after eval";

Under CGI, this would print just "in eval", but under plack we get both lines
and thus generate additional header and html after we already sent pdf data.

Signed-off-by: Theodoros Theodoropoulos <theod@lib.auth.gr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Theodoros Theodoropoulos 2019-10-03 12:06:39 UTC
Katrin, try to open it in Notepad++ or vi (or another editor).
You will see the headers and html at the end of the file.
With the patch, the resulting file has no such garbage and it has 1/3 of the size

In 18.11 (and earlier versions) the resulting file was not even readable by a PDF reader. For some reason in master the added garbage do not invalidate the produced PDF. Maybe has to do with some perl module update?
Comment 16 Martin Renvoize 2019-10-07 11:58:38 UTC
Nice work!

Pushed to master for 19.11.00
Comment 17 Fridolin SOMERS 2019-11-08 14:24:39 UTC
Pushed to 19.05.x for 19.05.05