Bug 22429 - Infinite loop in patron card printing
Summary: Infinite loop in patron card printing
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 22462 22497
  Show dependency treegraph
 
Reported: 2019-02-28 07:01 UTC by David Cook
Modified: 2019-03-26 06:27 UTC (History)
8 users (show)

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


Attachments
Bug 22429: Infinite loop in patron card printing (1.89 KB, patch)
2019-03-04 06:53 UTC, David Cook
Details | Diff | Splinter Review
Bug 22429: Infinite loop in patron card printing (1.96 KB, patch)
2019-03-12 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2019-02-28 07:01:27 UTC
I ran into an infinite loop in patron card printing in the following scenario:

if (($string_width + $text_attribs->{'llx'}) > $self->{'width'})

Then there's an odd "trim" or "wrap" which isn't working, which means that the following condition is not met and leads to an infinite loop:

if (($string_width + $text_attribs->{'llx'}) < $self->{'width'})
Comment 1 Katrin Fischer 2019-02-28 07:02:50 UTC
We have seen conditions where exporting a batch of patron cards brings down the instance. Do you know how the patron cards need to be set up to create this one?
Comment 2 David Cook 2019-02-28 07:03:11 UTC
I'm going to follow this up more tomorrow I hope. 

I need to gather more data regarding these lines:

                $line =~ m/^.*(\s.*\s*|\s&|\<.*\>)$/;
                warn sprintf('Line wrap failed. DEBUG INFO: Data: \'%s\'\n Method: C4::Patroncards->draw_text Additional Information: Line wrap regexp failed. (Please file in this information in a bug report
 at http://bugs.koha-community.org', $line) and last WRAP_LINES if !$1;
                $trim = $1 . $trim;
                $line =~ s/$1//;
                $string_width = C4::Creators::PDF->StrWidth($line, $text_attribs->{'font'}, $text_attribs->{'font_size'});
Comment 3 David Cook 2019-02-28 07:05:44 UTC
(In reply to Katrin Fischer from comment #1)
> We have seen conditions where exporting a batch of patron cards brings down
> the instance. Do you know how the patron cards need to be set up to create
> this one?

I have a bit of data but I need to gather more. Unfortunately it's the end of my work day.

However, it's a "<firstname> <surname>" scenario and the name is quite long it seems. And when the Lower Left X for the layout is quite large, the two combine to require line wrapping.

But at a glance I don't see how the line wrapping code could possibly work. 

In a CGI environment, it was causing huge problems as infinitely looping processes were running in the background and chewing up more and more resources as new processes were launched (and timed out at the web server end).
Comment 4 David Cook 2019-03-04 06:05:33 UTC
(In reply to Katrin Fischer from comment #1)
> We have seen conditions where exporting a batch of patron cards brings down
> the instance. Do you know how the patron cards need to be set up to create
> this one?

I don't have master conditions yet but in a 17.05.x I've reproduced with the following:

--
Layout:

Units: PostScript Points

Field 1
Text: <firstname> <surname>
Font: Times-Roman
Font size: 20pt
Text alignment: Center
Lower left X coordinate: 40pt
Lower left Y coordinate: 20pt

Barcode
Print card number as barcode: checked
Lower left X coordinate: 0
Lower left Y coordinate: 50
Print card number as text under barcode: checked
--

--
Template:

Units: SI Millimeters
Page height: 32mm
Page width: 95mm
Card width: 95mm
Card height: 30mm

Top page margin: 5mm
Left page margin: 2mm
Number of columns: 1
Number of rows: 1
--

--
User:

Cardnumber: 1
Surname: Test
Firnatme: 12345678 123456 (Test)
--

This can generate an infinitely looping process of /home/dcook/git/patroncards/create-pdf.pl
Comment 5 David Cook 2019-03-04 06:08:17 UTC
Forgot to mention the problem is in C4/Patroncards/Patroncard.pm
Comment 6 David Cook 2019-03-04 06:35:31 UTC
Aha... it's the parentheses which are the problem. They're being interpreted as regex metacharacters and not literal values! 


First line: '12345678 123456 (Test) Test'
First string width: 299.775390625
First $1: ' Test'
First $trim: ''

Next line: '12345678 123456 (Test)'
Next string width: 249.94140625
Next $1: ' (Test)'
Next $trim: ' (Test) Test'

Next line: '12345678 123456 (Test)'
Next string width: 249.94140625
Next $1: ' (Test)'
Next $trim: ' (Test) (Test) Test'

And the $trim just keeps prepending (Test) over and over again. 

--

Fortunately, this is a very easy solve.

I change the following
$line =~ s/$1//;

to

$line =~ s/\Q$1\E//;

And voila. It works. 

I bet this sort of "regular expression injection" could bite us in other parts of Koha...
Comment 7 David Cook 2019-03-04 06:53:23 UTC
Created attachment 85972 [details] [review]
Bug 22429: Infinite loop in patron card printing

Text fields in Patron Card Text Layouts can contain regular
expression metacharacters, which - instead of being treated as
literal values - are interpreted and prevent line wrapping. This
causes the process to get stuck in an infinite loop, which keeps
running even after the web server has timed out (at least when
using CGI).

This patch escapes the relevant input from the text field so the
regular expression substitution treats characters as literals
instead of as metacharacters.
Comment 8 David Cook 2019-03-04 06:57:16 UTC
Test plan:

1. Create data in your test Koha as follows:

Layout:

Units: PostScript Points

Field 1
Text: <firstname> <surname>
Font: Times-Roman
Font size: 20pt
Text alignment: Center
Lower left X coordinate: 40pt
Lower left Y coordinate: 20pt

Barcode
Print card number as barcode: checked
Lower left X coordinate: 0
Lower left Y coordinate: 50
Print card number as text under barcode: checked
--

--
Template:

Units: SI Millimeters
Page height: 32mm
Page width: 95mm
Card width: 95mm
Card height: 30mm

Top page margin: 5mm
Left page margin: 2mm
Number of columns: 1
Number of rows: 1
--

--
User:

Cardnumber: 1
Surname: Test
Firstname: 12345678 123456 (Test)

2. Before applying the patch, try to export a batch that contains the user with cardnumber 1
3. Note that Apache will timeout and that create-pdf.pl infinitely loops in the background (I haven't tested with Plack, but Katrin has mentioned in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22429#c1 that it will bring down the instance.)
  3a. ps -efww | grep "create-pdf"
4. Apply the patch
5. Note that the patron card is generated and downloaded via your browser
Comment 9 Christian Stelzenmüller 2019-03-04 13:24:58 UTC
> "Aha... it's the parentheses which are the problem. They're being interpreted as 
> regex metacharacters and not literal values!" 

We noticed that there seems to be an infinite loop if the template has some or all entries with "0" in it (like in default when creating a new template). Could this be the same cause?
Comment 10 David Cook 2019-03-05 00:34:01 UTC
(In reply to Christian Stelzenmüller from comment #9)
> > "Aha... it's the parentheses which are the problem. They're being interpreted as 
> > regex metacharacters and not literal values!" 
> 
> We noticed that there seems to be an infinite loop if the template has some
> or all entries with "0" in it (like in default when creating a new
> template). Could this be the same cause?

I don't think it would be the exact same cause, but probably related. My guess would be that the other regular expression used for "trimming" the long line might have problems with the 0s. I could probably look at that later today. 

Infinite loops are very easy to encounter with this code because it only breaks the loops for some very specific positive conditions. There probably should be some kind of sanity check to prevent infinite loops, or a more mathematically precise loop...

I have an urgent matter at hand but I'll keep this in mind to look at later today...
Comment 11 David Cook 2019-03-05 00:57:44 UTC
(In reply to Christian Stelzenmüller from comment #9)
> > "Aha... it's the parentheses which are the problem. They're being interpreted as 
> > regex metacharacters and not literal values!" 
> 
> We noticed that there seems to be an infinite loop if the template has some
> or all entries with "0" in it (like in default when creating a new
> template). Could this be the same cause?

Sorry, I just realized that you were referring to the template rather than the layout. 

Yes, it would be a related problem, but a separate issue. 

If 'label_width' is 0, then $self->{'width'} will be 0. 

That means that this condition will be triggered:

if (($string_width + $text_attribs->{'llx'}) > $self->{'width'})

And because $self->{'width'} is 0, then the following condition would never feasibly be triggered:

if (($string_width + $text_attribs->{'llx'}) < $self->{'width'}) 

And this condition is the only thing that can stop the infinite loop.

Really the line wrapping code looks like it would benefit from a re-write. 

I'd suggest opening a new bug report.
Comment 12 Martin Renvoize 2019-03-12 12:28:45 UTC
Created attachment 86520 [details] [review]
Bug 22429: Infinite loop in patron card printing

Text fields in Patron Card Text Layouts can contain regular
expression metacharacters, which - instead of being treated as
literal values - are interpreted and prevent line wrapping. This
causes the process to get stuck in an infinite loop, which keeps
running even after the web server has timed out (at least when
using CGI).

This patch escapes the relevant input from the text field so the
regular expression substitution treats characters as literals
instead of as metacharacters.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-03-12 12:29:30 UTC
I agree that the other issues should be opened as a distinct bug and as such having testing this code I'm signing off on it.
Comment 14 Martin Renvoize 2019-03-12 12:32:16 UTC
Trivial fix.. I've decide to go straight for QA
Comment 15 Nick Clemens 2019-03-15 12:49:09 UTC
Awesome work all!

Pushed to master for 19.05
Comment 16 Martin Renvoize 2019-03-19 09:34:11 UTC
Pushed to 18.11.x for 18.11.04
Comment 17 Lucas Gass 2019-03-19 23:08:14 UTC
backported to 18.05.x for 18.05.11
Comment 18 Fridolin SOMERS 2019-03-26 06:27:01 UTC
Pushed to 17.11.x for 17.11.17