[reSIProcate] WSS with AfterSocketCreationFunction causes core

Tue Jun 18 12:19:36 CDT 2013

I went through that same thought process.  However I believe that calling
transport() from the classes where the transport() override is defined (ie:
TlsTransport and TcpTransport) is safe.

Scott

On Tue, Jun 18, 2013 at 1:14 PM, Daniel Pocock <daniel at pocock.com.au> wrote:

>
>
> On 18/06/13 15:13, Scott Godin wrote:
> > In InternalTransport::bind - we could access the transport type from
> mTuple
> > instead - it is set in the constructor of each derived class before bind
> is
> > called (from init()).
> >
> >    if (mSocketFunc)
> >    {
> >        mSocketFunc(mFd, mTuple.getType(), __FILE__, __LINE__);
> >    }
>
>
> Oddly enough, TlsTransport sets mTuple with this superclass constructor
> invocation (from TlsTransport.cxx):
>
>    TlsBaseTransport(fifo, portNum, version, interfaceObj, security,
> sipDomain, sslType, transport(), socketFunc, compression,
> transportFlags, cvm, useEmailAsSIP)
>
>
> Notice it is calling the virtual function transport() to get the value?
>
> and in TcpTransport.cxx, it is slightly different, the constructor
> contains this call:
>
>     mTuple.setType(transport());
>
> but that is still naughty too.
>
> What I'm now tempted to do is
>
> a) remove the virtual keyword from transport()'s prototype, and replace
> it with { return mTuple.getType(); }
>
> b) change all other superclass constructor invocations to have hardcoded
> transport IDs to that (a) doesn't lead to unpleasant recursion
>
>
> >
> > Scott
> >
> > On Mon, Jun 17, 2013 at 4:48 PM, Daniel Pocock <daniel at pocock.com.au>
> wrote:
> >
> >>
> >>
> >> On 15/06/13 22:36, Vladimir Pavluk wrote:
> >>> Hi Daniel,
> >>>
> >>>> I've had a quick look at this
> >>>>
> >>>> I notice that in repro we call SipStack::addTransport( .. ) and we
> don't
> >>>> set mSocketFunc
> >>>>
> >>>> In your test case, you use DialogUsageManager::addTransport( .. )
> which
> >>>> just proxies the call to mStack.addTransport
> >>>>
> >>>> Do you definitely need to use the dum variation?  I think it may be
> >>>> useful to have this test case under resip/stack and avoid the dum
> >>>> dependency if possible.
> >>> No matter whichever we use (and I think we actually use
> >>> SipStack::addTransport( .. ) in our app), it always cores.
> >>>
> >>>> Would you know if it cores for TLS, or only for WSS?
> >>> It does core for both (you can build the test program and make sure).
> >>>
> >>>> Could you possibly share a stack trace?  You can exclude those parts
> of
> >>>> the stack that are in your own code.
> >>> It aborts with "pure virtual function call" right on the line where it
> >>> calls AfterSocketCreationFunction (we'll get back to you with more
> >>> precise stack trace).
> >>>
> >>
> >> I've adapted the test case and confirmed there is an issue (see below)
> >>
> >> The root cause is that it calls a pure virtual function,
> >> Transport::transport() during construction of an object of class
> >> TlsTransport
> >>
> >> I can confirm that this is definitely bad behavior.  I hacked around in
> >> this code a little bit to enable support for WSS, but not at this low
> >> level where the virtual function exists, so I don't believe it is a
> >> regression.
> >>
> >> Oddly, creating a TcpTransport invokes the same code path, it doesn't
> >> trigger this problem, but it is not safe for it to rely on the virtual
> >> function call during construction either.
> >>
> >> One option that comes to mind is that we can pass the transport type
> >> down from the constructor to make sure it is initialized.  The other
> >> option is to defer initialisation until after construction.
> >>
> >>
> >>
> >> $ libtool --mode=execute gdb resip/stack/test/testSocketFunc
> >> GNU gdb (GDB) 7.4.1-debian
> >> Copyright (C) 2012 Free Software Foundation, Inc.
> >> License GPLv3+: GNU GPL version 3 or later
> >> <http://gnu.org/licenses/gpl.html>
> >> This is free software: you are free to change and redistribute it.
> >> There is NO WARRANTY, to the extent permitted by law.  Type "show
> copying"
> >> and "show warranty" for details.
> >> This GDB was configured as "x86_64-linux-gnu".
> >> For bug reporting instructions, please see:
> >> <http://www.gnu.org/software/gdb/bugs/>...
> >> Reading symbols from
> >>
> >>
> /home/daniel/ws/resiprocate/resiprocate-trunk.git/resip/stack/test/.libs/lt-testSocketFunc...done.
> >> (gdb) run
> >> Starting program:
> >>
> >>
> /home/daniel/ws/resiprocate/resiprocate-trunk.git/resip/stack/test/.libs/lt-testSocketFunc
> >>
> >> [Thread debugging using libthread_db enabled]
> >> Using host libthread_db library
> "/lib/x86_64-linux-gnu/libthread_db.so.1".
> >> INFO | 20130617-221954.755 |  | RESIP:DNS | 140737353914144 |
> >> dns/AresDns.cxx:394 | DNS initialization: using c-ares v1.9.1
> >> INFO | 20130617-221954.755 |  | RESIP:DNS | 140737353914144 |
> >> dns/AresDns.cxx:403 | DNS initialization: found 1 name servers
> >> INFO | 20130617-221954.755 |  | RESIP:DNS | 140737353914144 |
> >> dns/AresDns.cxx:409 |  name server: 192.168.1.2
> >> INFO | 20130617-221954.760 |  | RESIP:TRANSPORT | 140737353914144 |
> >> Connection.cxx:38 | Connection::Connection: new connection created to
> >> who: [ V4 0.0.0.0:0 UNKNOWN_TRANSPORT target domain=unspecified
> >> mFlowKey=0 ]
> >> pure virtual method called
> >> terminate called without an active exception
> >>
> >> Program received signal SIGABRT, Aborted.
> >> 0x00007ffff6185475 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> >> (gdb) bt
> >> #0  0x00007ffff6185475 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> >> #1  0x00007ffff61886f0 in abort () from /lib/x86_64-linux-gnu/libc.so.6
> >> #2  0x00007ffff69da89d in __gnu_cxx::__verbose_terminate_handler() ()
> >> from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
> >> #3  0x00007ffff69d8996 in ?? () from
> >> /usr/lib/x86_64-linux-gnu/libstdc++.so.6
> >> #4  0x00007ffff69d89c3 in std::terminate() () from
> >> /usr/lib/x86_64-linux-gnu/libstdc++.so.6
> >> #5  0x00007ffff69d94df in __cxa_pure_virtual () from
> >> /usr/lib/x86_64-linux-gnu/libstdc++.so.6
> >> #6  0x00007ffff7ab7d3e in resip::InternalTransport::bind (this=0x696e80)
> >> at InternalTransport.cxx:147
> >> #7  0x00007ffff7afde98 in resip::TcpBaseTransport::init (this=0x696e80)
> >> at TcpBaseTransport.cxx:82
> >> #8  0x00007ffff7b53559 in resip::TlsBaseTransport::TlsBaseTransport
> >> (this=0x696e80, fifo=..., portNum=<optimized out>, version=<optimized
> >> out>, interfaceObj=..., security=...,
> >>     sipDomain=..., sslType=resip::SecurityTypes::TLSv1,
> >> transportType=resip::TLS, socketFunc=0x401230
> >> <afterSocketCreationFunction(int, int, char const*, int)>,
> compression=...,
> >>     transportFlags=0, cvm=resip::SecurityTypes::None,
> >> useEmailAsSIP=false) at ssl/TlsBaseTransport.cxx:46
> >> #9  0x00007ffff7b564a3 in resip::TlsTransport::TlsTransport
> >> (this=0x696e80, fifo=..., portNum=<optimized out>, version=<optimized
> >> out>, interfaceObj=..., security=..., sipDomain=...,
> >>     sslType=resip::SecurityTypes::TLSv1, socketFunc=0x401230
> >> <afterSocketCreationFunction(int, int, char const*, int)>,
> >> compression=..., transportFlags=0, cvm=resip::SecurityTypes::None,
> >>     useEmailAsSIP=false) at ssl/TlsTransport.cxx:35
> >> #10 0x00007ffff7af75f2 in resip::SipStack::addTransport
> >> (this=0x7ffffffb6660, protocol=resip::TLS, port=5061, version=resip::V4,
> >> stun=resip::StunDisabled, ipInterface=...,
> >>     sipDomainname=..., privateKeyPassPhrase=...,
> >> sslType=resip::SecurityTypes::TLSv1, transportFlags=0,
> >> cvm=resip::SecurityTypes::None, useEmailAsSIP=false) at SipStack.cxx:359
> >> #11 0x000000000040105f in main () at testSocketFunc.cxx:41
> >> _______________________________________________
> >> 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/20130618/2ddf00ed/attachment.htm>