From b46da4fe57615473a80f450d4fc36708688a4403 Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Fri, 17 Jun 2016 13:42:53 +0200
Subject: [PATCH] Bug 15006: [COUNTERPATCH] Timeout logic in sip_protocol_loop
Content-Type: text/plain; charset=utf-8

Instead of adding yet another client timeout, I propose to use the
policy timeout that we also communicate in send_acs_status.
Although I agree that we perhaps better move it from institution to
account/login level, this c/should be done on another report. We
may have issues with existing configurations.

In order to find a compromise between bug 13432 and the originally
proposed 15006 patch, this patch allows you to put a zero in the
policy timeout entry in the SIPconfig.

The timeout logic is moved to a Sip.pm subroutine. Will add a test :)

Test plan:
[1] Check timeout after login with policy/timeout > 0.
[2] Check timeout after login with policy/timeout = 0. (No timeout!)
[3] Check timeout after login with no policy timeout. Fallback to service.
[4] Check timeout after login with no policy and service timeout.
    Should fallback to 30 seconds for both login process and after login.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
---
 C4/SIP/SIPServer.pm |   19 ++++++++-----------
 C4/SIP/Sip.pm       |   27 ++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/C4/SIP/SIPServer.pm b/C4/SIP/SIPServer.pm
index 50473ed..19549f8 100755
--- a/C4/SIP/SIPServer.pm
+++ b/C4/SIP/SIPServer.pm
@@ -15,7 +15,7 @@ use C4::SIP::Sip::Constants qw(:all);
 use C4::SIP::Sip::Configuration;
 use C4::SIP::Sip::Checksum qw(checksum verify_cksum);
 use C4::SIP::Sip::MsgType qw( handle login_core );
-use C4::SIP::Sip qw( read_SIP_packet );
+use C4::SIP::Sip qw( read_SIP_packet get_timeout );
 
 use base qw(Net::Server::PreFork);
 
@@ -128,7 +128,7 @@ sub raw_transport {
     my $strikes = 3;
 
     local $SIG{ALRM} = sub { die "raw_transport Timed Out!\n"; };
-    my $timeout = $service->{timeout} || 30;
+    my $timeout = get_timeout( $self, { transport => 1 } );
     syslog("LOG_DEBUG", "raw_transport: timeout is %d", $timeout);
 
     while( $strikes && ( !$self->{account} || !$self->{account}->{id} )) {
@@ -193,7 +193,7 @@ sub telnet_transport {
     my $account = undef;
     my $input;
     my $config  = $self->{config};
-    my $timeout = $self->{service}->{timeout} || $config->{timeout} || 30;
+    my $timeout = get_timeout( $self, { transport => 1 } );
     syslog("LOG_DEBUG", "telnet_transport: timeout is %s", $timeout);
 
     eval {
@@ -255,7 +255,8 @@ sub sip_protocol_loop {
     my $self = shift;
     my $service = $self->{service};
     my $config  = $self->{config};
-    my $timeout = $self->{service}->{client_timeout} || $config->{client_timeout};
+
+    my $timeout = get_timeout( $self, { client => 1 } );
     my $input;
 
     # The spec says the first message will be:
@@ -272,12 +273,10 @@ sub sip_protocol_loop {
     #
     # In short, we'll take any valid message here.
     #my $expect = SC_STATUS;
-    local $SIG{ALRM} = sub { die "SIP Timed Out!\n"; } if $timeout;
+    local $SIG{ALRM} = sub { die "SIP Timed Out!\n"; };
     my $expect = '';
     while (1) {
-        if ($timeout) {
-            alarm $timeout;
-        }
+        alarm $timeout;
         $input = read_SIP_packet(*STDIN);
         unless ($input) {
             return;        # EOF
@@ -303,9 +302,7 @@ sub sip_protocol_loop {
         }
         # We successfully received and processed what we were expecting
         $expect = '';
-        if ($timeout ) {
-            alarm 0;
-        }
+        alarm 0;
     }
 }
 
diff --git a/C4/SIP/Sip.pm b/C4/SIP/Sip.pm
index a6cf97f..89ad9d1 100644
--- a/C4/SIP/Sip.pm
+++ b/C4/SIP/Sip.pm
@@ -23,13 +23,14 @@ BEGIN {
 
 	@EXPORT_OK = qw(y_or_n timestamp add_field maybe_add add_count
 		    denied sipbool boolspace write_msg read_SIP_packet
+            get_timeout
 		    $error_detection $protocol_version $field_delimiter
 		    $last_response);
 
 	%EXPORT_TAGS = (
 		    all => [qw(y_or_n timestamp add_field maybe_add
 			       add_count denied sipbool boolspace write_msg
-			       read_SIP_packet
+                   read_SIP_packet get_timeout
 			       $error_detection $protocol_version
 			       $field_delimiter $last_response)]);
 }
@@ -251,4 +252,28 @@ sub write_msg {
     $last_response = $msg;
 }
 
+# get_timeout( $server, { transport|client => 1, fallback => some_val } );
+#
+# Centralizes all timeout logic from SIPServer into one routine
+
+sub get_timeout {
+    my ( $server, $params ) = @_;
+
+    my $fallback = $params->{fallback} || 30;
+    if( $params->{transport} ) {
+        # We do not allow zero values here
+        return $server->{service}->{timeout} || $fallback;
+    } elsif( $params->{client} ) {
+        if( exists $server->{policy}->{timeout} ) {
+        # We do allow zero values here to indicate no timeout
+            my $val = $server->{policy}->{timeout};
+            $val = 0 if ! $val > 0; # just precautious
+            return $val;
+        }
+        return $server->{service}->{timeout} || $fallback;
+    } else {
+        return $fallback;
+    }
+}
+
 1;
-- 
1.7.10.4