| < Previous by Date | Date Index | Next by Date > |
| < Previous in Thread | Thread Index | Next in Thread > |
Fix seems fine to me. You've retained the transport() method in Transport.hxx so API users should not notice anything missing. I would remove the signatures from the other header files though to cleanup the code, instead of leaving them commented out.I like adding dummy certs.ScottOn Tue, Jun 18, 2013 at 1:46 PM, Daniel Pocock <daniel@xxxxxxxxxxxxx> wrote:
I've just committed the variation of the fix removing virtual, but I'm
happy to revise this later if there are problems with it.
I've also commented out the TLS and WSS parts of the test case because
they require certs - do you think we should include dummy certs in the
source tree or just script the openssl command to make them on the fly?
I've put the openssl command in a comment in the test case:
https://svn.resiprocate.org/viewsvn/resiprocate/main/resip/stack/test/testSocketFunc.cxx?view=markup#l37
On 18/06/13 19:14, Daniel Pocock 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>>> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>>>
>>
_______________________________________________
resiprocate-devel mailing list
resiprocate-devel@xxxxxxxxxxxxxxx
https://list.resiprocate.org/mailman/listinfo/resiprocate-devel