Re: [reSIProcate] Resiprocate crash when rejecting a subscribe that have expires set to 0 with 404

[Ionut Slaveanu <islaveanu@xxxxxxxxx>]

Hi Scott,

I attached the patch to this email. Can you review it and commit if is ok.

Another solution would be to remove the code that handle the expire set to 0 in a subscription from ServerSubscription::dispatch function, I mean if (mExpires == 0){...}. and let the application to handle this situation in onNewSubscription callback.

Thank you!

On Fri, Mar 28, 2014 at 9:11 PM, Scott Godin <sgodin@xxxxxxxxxxxxxxx> wrote:

Sounds like we need to fixup this code.  If you have a fix for this, please feel free to contribute it.

Thanks,

Scott Godin

On Fri, Mar 28, 2014 at 5:47 AM, Ionut Slaveanu <islaveanu@xxxxxxxxx> wrote:

Hi guys,

The scenario is the following:

We implemented a resource list server with resiprocate and in ServerSubscriptionHandler::onNewSubscription we reject a subscribtion that have expires set to 0 to an unexistent resource with 404.
So in ServerSubscriptionHandler::onNewSubscription we call:
subscriptionHandle->send(subscriptionHandle->reject(404));

The problem is that ServerSubscription::send calls:
          if (shouldDestroyAfterSendingFailure(*msg))
129│          {
130├>            DialogUsage::send(msg);
131│             handler->onTerminated(getHandle());
132│             delete this;
133│             return;
134│          }

which deletes the subscriptionHandle. Then ServerSubscription::dispatch calls:
         makeNotifyExpires();
         handler->onExpiredByClient(getHandle(), msg, *mLastRequest);
        
         mDialog.makeResponse(*mLastResponse, mLastSubscribe, 200);
         mLastResponse->header(h_Expires).value() = mExpires;
         send(mLastResponse);

         send(mLastRequest);  // Send Notify Expires

Currently, resiprocate crashes in makeNotifyExpires, which tries to use members from the class which were already deleted.

Is there a way to reject a subscribe that have expires set to 0 with 404? Currently the resiprocate code also tries to send the notify when expires is set to 0.

_______________________________________________
resiprocate-devel mailing list
resiprocate-devel@xxxxxxxxxxxxxxx
https://list.resiprocate.org/mailman/listinfo/resiprocate-devel

Index: resip/dum/ServerSubscription.cxx
===================================================================
--- resip/dum/ServerSubscription.cxx	(revision 11127)
+++ resip/dum/ServerSubscription.cxx	(working copy)
@@ -25,7 +25,8 @@
    : BaseSubscription(dum, dialog, req),
      mSubscriber(req.header(h_From).uri().getAor()),
      mExpires(60),
-     mAbsoluteExpiry(0)
+     mAbsoluteExpiry(0),
+     mDeleteSubscription(true)
 {
    if (req.header(h_RequestLine).method() == REFER && req.header(h_To).exists(p_tag))
    {
@@ -89,7 +90,16 @@
    return mLastResponse;
 }
 
+void ServerSubscription::terminateSubscription(ServerSubscriptionHandler* handler)
+{
+  if (mDeleteSubscription)
+  {
+    handler->onTerminated(getHandle());
+    delete this;
+  }
+}
 
+
 void 
 ServerSubscription::send(SharedPtr<SipMessage> msg)
 {
@@ -119,8 +129,7 @@
       else if (code < 400)
       {
          DialogUsage::send(msg);
-         handler->onTerminated(getHandle());
-         delete this;
+         terminateSubscription(handler);
          return;
       }
       else
@@ -128,8 +137,7 @@
          if (shouldDestroyAfterSendingFailure(*msg))
          {
             DialogUsage::send(msg);
-            handler->onTerminated(getHandle());
-            delete this;
+            terminateSubscription(handler);
             return;
          }
          else
@@ -143,8 +151,7 @@
       DialogUsage::send(msg);
       if (mSubscriptionState == Terminated)
       {
-         handler->onTerminated(getHandle());
-         delete this;
+        terminateSubscription(handler);
       }
    }
 }
@@ -245,6 +252,8 @@
          if (mSubscriptionState == Invalid)
          {
             mSubscriptionState = Terminated;
+            mDeleteSubscription = false;
+
             if (mEventType != "refer" )
             {
                handler->onNewSubscription(getHandle(), msg);
@@ -253,11 +262,19 @@
             {
                handler->onNewSubscriptionFromRefer(getHandle(), msg);
             }
+
+            mDeleteSubscription = true;
+
+            if (mLastResponse->header(h_StatusLine).statusCode() >= 300)
+            {
+              send(mLastResponse);
+              return;
+            }
          }
 
          makeNotifyExpires();
          handler->onExpiredByClient(getHandle(), msg, *mLastRequest);
-         
+
          mDialog.makeResponse(*mLastResponse, mLastSubscribe, 200);
          mLastResponse->header(h_Expires).value() = mExpires;
          send(mLastResponse);
@@ -305,8 +322,7 @@
       {
          //in dialog NOTIFY got redirected? Bizarre...
          handler->onError(getHandle(), msg);
-         handler->onTerminated(getHandle());
-         delete this;         
+         terminateSubscription(handler);
       }
       else
       {
@@ -323,8 +339,7 @@
             case Helper::DialogTermination:
                DebugLog( << "ServerSubscription::UsageTermination: " << msg.brief());
                handler->onError(getHandle(), msg);
-               handler->onTerminated(getHandle());
-               delete this;
+               terminateSubscription(handler);
                break;
          }
       }
@@ -416,8 +431,7 @@
    ServerSubscriptionHandler* handler = mDum.getServerSubscriptionHandler(mEventType);
    assert(handler);   
    handler->onError(getHandle(), msg);
-   handler->onTerminated(getHandle());
-   delete this;
+   terminateSubscription(handler);
 }
 
 void 
Index: resip/dum/ServerSubscription.hxx
===================================================================
--- resip/dum/ServerSubscription.hxx	(revision 11127)
+++ resip/dum/ServerSubscription.hxx	(working copy)
@@ -7,6 +7,7 @@
 {
 
 class DialogUsageManager;
+class ServerSubscriptionHandler;
 
 //!dcm! -- no Subscription State expires parameter generation yet. 
 class ServerSubscription : public BaseSubscription 
@@ -63,6 +64,8 @@
       
       bool shouldDestroyAfterSendingFailure(const SipMessage& msg);      
 
+      void terminateSubscription(ServerSubscriptionHandler* handler);
+
       Data mSubscriber;
 
 //      const Contents* mCurrentEventDocument;
@@ -74,6 +77,8 @@
       ServerSubscription(const ServerSubscription&);
       ServerSubscription& operator=(const ServerSubscription&);
       UInt64 mAbsoluteExpiry;      
+
+      bool mDeleteSubscription;
 };
  
 }