[reSIProcate] NULL Pointer crash with resip 1.3.3 - [PATCH]forpart of issue
Byron Campen
bcampen at estacado.net
Fri Jul 18 14:39:51 CDT 2008
I still don't know about this. It looks _better_, but I still don't
like the whole idea of starting a brand new subscription if we get a
mid-dialog retry-after.
Daniel, Derek, you guys wrote this code. Any opinions on how it
should be fixed?
Best regards,
Byron Campen
> Never heard from anybody if this patch was correct…
>
> -Aron
>
> From: resiprocate-devel-bounces at resiprocate.org [mailto:resiprocate-devel-bounces at resiprocate.org
> ] On Behalf Of Aron Rosenberg
> Sent: Wednesday, July 16, 2008 12:14 PM
> To: Byron Campen
> Cc: resiprocate-devel
> Subject: Re: [reSIProcate] NULL Pointer crash with resip 1.3.3 -
> [PATCH]forpart of issue
>
> This is an updated patch which removes the use mRemoteTarget
> completely for the new subscription and drops the appDialogSet reuse
>
> Its untested right now, but wanted to see if it was conceptually
> correct.
>
> Index: dum/ClientSubscription.cxx
> ===================================================================
> --- dum/ClientSubscription.cxx (revision 8133)
> +++ dum/ClientSubscription.cxx (working copy)
> @@ -142,9 +142,9 @@
> msg.exists(h_Expires) && msg.header(h_Expires).value()
> > 0)
> {
> InfoLog (<< "Received 481 to SUBSCRIBE, reSUBSCRIBEing
> (presence server probably restarted) "
> - << mDialog.mRemoteTarget);
> + << mLastRequest->header(h_To));
>
> - SharedPtr<SipMessage> sub =
> mDum.makeSubscription(mDialog.mRemoteTarget, getEventType(),
> getAppDialogSet()->reuse());
> + SharedPtr<SipMessage> sub =
> mDum.makeSubscription(mLastRequest->header(h_To), getEventType());
> mDum.send(sub);
>
> delete this;
> @@ -188,17 +188,10 @@
> DebugLog(<< "Application requested immediate retry on
> Retry-After");
> //!dcm! -- why isn't this just a refresh--retry after
> might be
> //middle element and not indicate dialog destruction
> - if (mDialog.mRemoteTarget.uri().host().empty())
> - {
> - SharedPtr<SipMessage> sub =
> mDum.makeSubscription(mLastRequest->header(h_To), getEventType());
> - mDum.send(sub);
> - }
> - else
> - {
> - SharedPtr<SipMessage> sub =
> mDum.makeSubscription(mDialog.mRemoteTarget, getEventType(),
> getAppDialogSet()->reuse());
> - mDum.send(sub);
> - }
> - //!dcm! -- new sub created above, when does this usage get
> destroyed?
> + SharedPtr<SipMessage> sub =
> mDum.makeSubscription(mLastRequest->header(h_To), getEventType());
> + mDum.send(sub);
> +
> + //!dcm! -- new sub created above, when
> does this usage get destroyed?
> //return;
> }
> else
> @@ -386,17 +379,8 @@
> else
> {
> InfoLog(<< "ClientSubscription: application retry new
> request");
> -
> - if (mDialog.mRemoteTarget.uri().host().empty())
> - {
> - SharedPtr<SipMessage> sub =
> mDum.makeSubscription(mLastRequest->header(h_To), getEventType());
> - mDum.send(sub);
> - }
> - else
> - {
> - SharedPtr<SipMessage> sub =
> mDum.makeSubscription(mDialog.mRemoteTarget, getEventType(),
> getAppDialogSet()->reuse());
> - mDum.send(sub);
> - }
> + SharedPtr<SipMessage> sub =
> mDum.makeSubscription(mLastRequest->header(h_To), getEventType());
> + mDum.send(sub);
>
> delete this;
> }
>
> From: Byron Campen [mailto:bcampen at estacado.net]
> Sent: Tuesday, July 15, 2008 10:12 AM
> To: Aron Rosenberg
> Cc: Scott Godin; resiprocate-devel
> Subject: Re: [reSIProcate] NULL Pointer crash with resip 1.3.3 -
> [PATCH] forpart of issue
>
> That is _completely_ broken. It's not even close to
> being correct. This needs to be fixed.
>
> Best regards,
> Byron Campen
>
> In ClientSubscription.cxx mRemoteTarget is used as the first
> parameter to makeSubscription, which is populated from the Contact
> header whenever onRequestRetry returns 0.
>
> -Aron
>
> From: Byron Campen [mailto:bcampen at estacado.net]
> Sent: Tuesday, July 15, 2008 9:13 AM
> To: Aron Rosenberg
> Cc: Scott Godin; resiprocate-devel
> Subject: Re: [reSIProcate] NULL Pointer crash with resip 1.3.3 -
> [PATCH] forpart of issue
>
> DUM is using the value of the Contact header in the To
> header? What?
>
> Best regards,
> Byron Campen
>
>
> I get an empty To/From since the presence server (OpenSER) puts only
> the ip:port or domain name as the Contact header in the dialog.
>
> Contact: <sip:sip.sightspeed.com> or <sip:123.123.123.123:5060>
>
> If the dialog/transaction then times-out (408) the onRequestRetry
> callback is called. Our code returns 0 (please retry transaction).
> In this case the mRemoteTarget contained the original contact header
> (just domain name or ip:port) and this value is then used as the new
> “To” header when the ClientSubscription.cxx code calls
> makeSubscription for us. We then have a To/From header which is
> missing the “user” field of the original request.
>
> As for the “end”. We call end off the subscription handle at some
> point in the future (i.e. when the user is logging out)
>
> -Aron
>
>
> From: Scott Godin [mailto:slgodin at icescape.com]
> Sent: Tuesday, July 15, 2008 6:50 AM
> To: Aron Rosenberg; resiprocate-devel
> Subject: RE: [reSIProcate] NULL Pointer crash with resip 1.3.3 -
> [PATCH] forpart of issue
>
> The NULL mAppDialogSet pointer issues aside – I’m a little confused
> how you are getting the empty From/To header. From the code it
> seems that if mRemoteTarget is set (ie. Host is non-empty) then it
> will use that, otherwise it will use mLastRequest->header(h_To).
> How does it end up being empty?
>
> Also – from your step 2 below, you say:
> Ø Client ends the subscription by invoking end() on the handle
> What handle are you referring to? I don’t think you should even
> have a client subscription handle until the first notify arrives and
> you get your first callback (onNewSubscription).
>
> Scott
>
> From: resiprocate-devel-bounces at resiprocate.org [mailto:resiprocate-devel-bounces at resiprocate.org
> ] On Behalf Of Aron Rosenberg
> Sent: Monday, July 14, 2008 8:17 PM
> To: resiprocate-devel
> Subject: Re: [reSIProcate] NULL Pointer crash with resip 1.3.3 -
> [PATCH] forpart of issue
>
> Here is a simple patch which I believe addresses part of the issue
> we are seeing.
>
> Index: resip/dum/ClientSubscription.cxx
> ===================================================================
> --- resip/dum/ClientSubscription.cxx (revision 8133)
> +++ resip/dum/ClientSubscription.cxx (working copy)
> @@ -78,10 +78,7 @@
> if (!mOnNewSubscriptionCalled && !getAppDialogSet()-
> >isReUsed())
> {
> InfoLog (<< "[ClientSubscription] " << mLastRequest-
> >header(h_To));
> - if (msg.exists(h_Contacts))
> - {
> - mDialog.mRemoteTarget = msg.header(h_Contacts).front();
> - }
> + mDialog.mRemoteTarget = msg.header(h_To);
>
> handler->onNewSubscription(getHandle(), msg);
> mOnNewSubscriptionCalled = true;
> --
>
> The original symptom of an empty From/To header was caused by the
> mRemoteTarget being set with the contact address which is almost
> always (IP:Port) or just (DNS name:port). The mRemoteTarget was then
> used to build the resubscribe if you requested it which resulted in
> the empty to/from username.
>
> I believe that all the if…else cases which tested mRemoteTarget for
> Uri / Host values aren’t needed if the above is fixed properly.
>
> There is still the issue that the getAppDialogSet() crashes since
> the pointer is NULL
>
> -Aron
>
>
>
> From: resiprocate-devel-bounces at resiprocate.org [mailto:resiprocate-devel-bounces at resiprocate.org
> ] On Behalf Of Aron Rosenberg
> Sent: Monday, July 14, 2008 4:20 PM
> To: resiprocate-devel
> Subject: Re: [reSIProcate] NULL Pointer crash with resip 1.3.3
>
> I was finally able to get a working pcap, resip log and debug crash
> at the same time. Here is what is going on
>
> 1. Client makes subscription
> 2. Client ends the subscription by invoking end() on the handle
> 3. This end results in a local 408 error, which calls
> onRequestRetry
> 4. Our code returns 0 to
> onRequestRetry(ClientSubscriptionHandle) to retry the request since
> we want the server to know we ended the sub
> 5. "Application requested immediate retry on Retry-After" is
> printed to log
> 6. Crash happens in the else statement in
> ClientSubscription.cxx:198 when trying to call getAppDialogSet()-
> >reuse().
>
> I have a full log (over 100MB of resip data which I can send to a
> developer who wants to look at it along with the matching pcap error
> file
>
> -Aron
>
>
> From: resiprocate-devel-bounces at resiprocate.org [mailto:resiprocate-devel-bounces at resiprocate.org
> ] On Behalf Of Aron Rosenberg
> Sent: Monday, July 14, 2008 2:17 PM
> To: resiprocate-devel
> Subject: Re: [reSIProcate] NULL Pointer crash with resip 1.3.3
>
> Here is a little bit more information gleaned from a pcap trace.
>
> The stack seems to be crashing when dealing with a 400 error where
> the “From:” header looks like this
>
> “From: <sip:>;tag=5b461e50”
>
> I was able to find the outbound SUBSCRIBE request and it also has an
> empty From address so something strange is going on in the stack.
> Still working on getting the resip logs.
>
> -Aron
>
> From: resiprocate-devel-bounces at resiprocate.org [mailto:resiprocate-devel-bounces at resiprocate.org
> ] On Behalf Of Aron Rosenberg
> Sent: Monday, July 14, 2008 11:50 AM
> To: resiprocate-devel
> Subject: [reSIProcate] NULL Pointer crash with resip 1.3.3
>
> Resip ver: SVN rev 8128 on 1.3 branch
>
> Call Stack:
> resip::AppDialogSet::getHandle() Line 22 + 0x3 bytes C++
> resip::DialogUsage::getAppDialogSet() Line 38 + 0x18 bytes C++
> resip::ClientSubscription::processResponse(const resip::SipMessage &
> msg={...}) Line 198 + 0x12 bytes C++
> resip::ClientSubscription::dispatch(const resip::SipMessage &
> msg={...}) Line 117 C++
> resip::Dialog::dispatch(const resip::SipMessage & msg={...}) Line
> 651 + 0x1a bytes C++
> resip::DialogSet::dispatchToAllDialogs(const resip::SipMessage &
> msg={...}) Line 1028 C++
> resip::DialogSet::dispatch(const resip::SipMessage & msg={...}) Line
> 608 C++
> resip::DialogUsageManager::processResponse(const resip::SipMessage &
> response={...}) Line 1810 C++
> resip
> ::DialogUsageManager::incomingProcess(std::auto_ptr<resip::Message>
> msg=auto_ptr {tu=??? }) Line 1363 C++
> resip
> ::DialogUsageManager::internalProcess(std::auto_ptr<resip::Message>
> msg=auto_ptr {tu=??? }) Line 1190 C++
> resip::DialogUsageManager::process(resip::RWMutex *
> mutex=0x00000000) Line 1390 + 0x49 bytes C++
> SipEP::run() Line 3408 + 0xa bytes C++
>
> The crash is because the appDialogSet returned in
> DialogUsage::getAppDialogSet() is NULL.
>
> It came from our production client and is reasonable repeatable, so
> I am working on getting the resip logs that would go with it.
>
> -Aron
>
>
> ---------------------------------------------
> Aron Rosenberg
> Founder and CTO
> SightSpeed - http://www.sightspeed.com/
>
> 918 Parker St, Suite A14
> Berkeley, CA 94710
>
> Email: arosenberg at sightspeed.com
> Phone: 510-665-2920
> Cell: 510-847-7389
> Fax: 510-649-9569
> SightSpeed Video Link: http://aron.sightspeed.com
>
>
>
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel at resiprocate.org
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>
>
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel at resiprocate.org
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20080718/fa1e92d7/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2423 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20080718/fa1e92d7/attachment.bin>
More information about the resiprocate-devel
mailing list