[sr-dev] git:master: tm: safety checks for possible escaped neg. ACKs

[Andrei Pelinescu-Onciul]

Module: sip-router
Branch: master
Commit: 8632598cac189adb557ad0c65f15744ef4256e45
URL:    http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=8632598cac189adb557ad0c65f15744ef4256e45

Author: Andrei Pelinescu-Onciul <andrei at iptel.org>
Committer: Andrei Pelinescu-Onciul <andrei at iptel.org>
Date:   Wed Jun 24 20:01:46 2009 +0200

tm: safety checks for possible escaped neg. ACKs

In normal operation looking up a transaction corresponding to an ACK to a
neg. reply or to a local transaction should end up in script
termination, so when t_relay_to() is called for a neg. ACK, the
transaction should not have been looked up previously. If this
assumption fails, the ACK will be processed normally (resulting at
worst in calling the TMCB_ACK_NEG_IN callback multiple times for
the same ACK) and a warning message will be logged.

---

 modules/tm/t_funcs.c |   28 +++++++++++++++++++++++-----
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/modules/tm/t_funcs.c b/modules/tm/t_funcs.c
index d60c99e..aa7d1f4 100644
--- a/modules/tm/t_funcs.c
+++ b/modules/tm/t_funcs.c
@@ -271,11 +271,10 @@ int t_relay_to( struct sip_msg  *p_msg , struct proxy_l *proxy, int proto,
 	/* parsing error, memory alloc, whatever ... if via is bad
 	   and we are forced to reply there, return with 0 (->break),
 	   pass error status otherwise
-
-       MMA: return value E_SCRIPT means that transaction was already started from the script
-	   so continue with that transaction
+	   MMA: return value E_SCRIPT means that transaction was already started
+	   from the script so continue with that transaction
 	*/
-	if (new_tran!=E_SCRIPT) {
+	if (likely(new_tran!=E_SCRIPT)) {
 		if (new_tran<0) {
 			ret = (ser_error==E_BAD_VIA && reply_to_via) ? 0 : new_tran;
 			goto done;
@@ -285,11 +284,30 @@ int t_relay_to( struct sip_msg  *p_msg , struct proxy_l *proxy, int proto,
 			ret = 1;
 			goto done;
 		}
+	}else if (unlikely(p_msg->REQ_METHOD==METHOD_ACK)) {
+			/* transaction previously found (E_SCRIPT) and msg==ACK
+			    => ack to neg. reply  or ack to local trans.
+			    => process it and exit */
+			/* FIXME: there's no way to distinguish here between acks to
+			   local trans. and neg. acks */
+			/* in normal operation we should never reach this point, if we
+			   do WARN(), it might hide some real bug (apart from possibly
+			   hiding a bug the most harm done is calling the TMCB_ACK_NEG
+			   callbacks twice) */
+			WARN("negative or local ACK caught, please report\n");
+			t=get_t();
+			if (unlikely(has_tran_tmcbs(t, TMCB_ACK_NEG_IN)))
+				run_trans_callbacks(TMCB_ACK_NEG_IN, t, p_msg, 0, 
+										p_msg->REQ_METHOD);
+			t_release_transaction(t);
+			ret=1;
+			goto done;
 	}
 
 	/* new transaction */
 
-	/* ACKs do not establish a transaction and are fwd-ed statelessly */
+	/* at this point if the msg is an ACK it is an e2e ACK and
+	   e2e ACKs do not establish a transaction and are fwd-ed statelessly */
 	if ( p_msg->REQ_METHOD==METHOD_ACK) {
 		DBG( "SER: forwarding ACK  statelessly \n");
 		if (proxy==0) {