Stackoverflow if WSDL file includes recursively

[DRFrederik]

Example:
when using onvif media:
http://www.onvif.org/onvif/ver10/media/wsdl/media.wsdl
Function GetStreamUri requires StreamSetup which requires Transport which requires Tunnel ... and Tunnel requires Tunnel ...
This causes the recursive creation of Tunnel objects on the stack when making a Tunnel instance resulting in runtime errors.
Using a pointer to Tunnel instead of an object instance would solve this issue.

[DRFrederik]

Gererated stub code:

wsdl_media.h

namespace media {
    class TT__Transport
    {
    public:
        void setProtocol( const media::TT__TransportProtocol& _protocol );
        media::TT__TransportProtocol protocol() const;
        void setTunnel( const media::TT__Transport& _tunnel );
        media::TT__Transport tunnel() const;
        KDSoapValue serialize( const QString& valueName ) const;
        void deserialize( const KDSoapValue& mainValue );
        TT__Transport();
        ~TT__Transport();

    public:
        TT__Transport( const TT__Transport& );
        TT__Transport &operator=( const TT__Transport& );

    private:
        class PrivateDPtr;
        QSharedDataPointer<PrivateDPtr> d_ptr;
};
}

wsdl_media.cpp

media::TT__Transport::TT__Transport()
    : d_ptr(new PrivateDPtr)
{
}

wsdl_media.cpp

class media::TT__Transport::PrivateDPtr : public QSharedData
{
public:
    PrivateDPtr();

public:
    media::TT__TransportProtocol mProtocol;
    media::TT__Transport mTunnel;
    bool mTunnel_nil;
};

[DRFrederik]

Step 1: TT_Transport has a QSharedDataPointer of type PrivateDPtr, new PrivateDPtr is made in ctor.
Step 2: PrivateDPtr has a TT_Transport named mTunnel.
Step 3: continue from step 1.
...
Eventually your runtime will crash.
I hacked the stub one year ago not to create mTunnel as we don't need it.
The problem I've described is a bug in KDSoap, no "new PrivateDPtr" should be made.
The pointer should be a nullptr up to the point initialization of the object is required by one of the get or set functions.
Some one must edit Kdwsdl2cpp to get this behavior.

I wonder how many more years until KDAB is going to respond.
Must I conclude KDSoap is abandonware?

[dfaure-kdab]

Nope, I'm always happy to review patches, so it's not abandonware :-)

[DRFrederik]

You mean I'm the one who's going to fix it. ;-)

[dfaure-kdab]

That's how opensource works ;)

I'm taking a look at how to add media.wsdl to the onvif.org unittest but this raises another issue, being able to parse two WSDL files at the same time to avoid duplication of xsd-generated classes.

I'm actually surprised that mTunnel isn't an optional member, there were commits to better support that...

[dfaure-kdab]

For me wsdl_media.cpp says the following:

class TT__Transport::PrivateDPtr : public QSharedData
{
public:
    TT__TransportProtocol mProtocol;
    QSharedPointer<TT__Transport> mTunnel;
    KDSoapValue mTunnel_as_kdsoap_value;
};

Are you sure you retested with a recent version of KDSoap?

[DRFrederik]

No, I'll upgrade to 1.6 next week and I'll retest it then.

[winterz]

DRFrederik,

I am the release manager for KDSoap. When I release a new version (1.6.0 was released in early May) I send an announcement email to the kdsoap-interest mailing list. I suggest you join that list. It is very, very low volume

https://mail.kdab.com/mailman/listinfo/kdsoap-interest

[DRFrederik]

My colleague had just retested with 1.6 and he has the same generated code as I've posted before.
Can you tell us what triggers the use of a QSharedPointer<TT__Transport> mTunnel instead of a class object?

[dfaure-kdab]

Did he completely clean out the build directory so that the wsdl-generated files get regenerated with the new KDSoap?

[DRFrederik]

He did setup a new test project using the latest code. Can you tell us which version of kdwsl2cpp you've used to generate your code? We could check if it's equal to yours.

[dfaure-kdab]

kdwsdl2cpp isn't versioned separately, what matters is the KDSoap release that it's from.

Using the binary from a 1.6.0 build, I do
kdwsdl2cpp media.wsdl -impl wsdl_media.h -o wsdl_media.cpp and then the output of grep mTunnel wsdl_media.cpp starts with QSharedPointer<TT__Transport> mTunnel;

[DRFrederik]

We've tested the command on Jessie (using Qt 5.3.2) and we have seen both versions (yours and mine) coming out of kdwsdl2cpp ...
My colleague is looking into it, we'll update when we find out more.

[dfaure-kdab]

You made me wonder if there was some uninitialized variable in there, but no, valgrind says kdwsdl2cpp is clean. My only hypothesis is that you're sometimes picking up the wrong version of kdwsdl2cpp.

[DRFrederik]

My colleague got it working when he doesn't use a namespace.
As you can see in my second post I've used the namespace "media", when not using any namespace mTunnel will be of type QSharedPointer<TT__Transport> instead of media::TT__Transport.

[RMesotten]

Hi, I am said colleague.

I just created a pull request with my fix for the issue.
It seems that the usePointerForElement function is broken and somewhat hastily implemented with parts copied from isElementPolymorphic.
I just changed the part that fixes our issue, but the references to isElementPolymorphic seem strange to me, although they could be intentional. Maybe you could have a look yourself, as I'm not too familiar with the code.

Kind Regards

[RMesotten]

Could you please take a look at my fix ?

[dfaure-kdab]

Sorry I was on vacations the last 2.5 weeks. I'm back now. The fix looks OK and doesn't break any existing unittests, but it's missing a unittest for the actual fix. I'm setting that up now.

[dfaure-kdab]

What do you find strange in the references to isElementPolymorphic?

We want to use a pointer if 1) inheritance is used or 2) the element is optional and used "inside itself".

isElementPolymorphic is about case 1, your fix is about case 2.

[RMesotten]

I might have drawn to that conclusion too fast. But the debug print should probably say "Making element optional", which threw me off a bit.

[dfaure-kdab]

It's already optional (it would use a "bool isnil" otherwise), but I see your point, I'll adjust the debug to "Using pointer for optional element used inside itself" instead.

If you don't mind I'll create a single commit with your fix and my unittest.

[RMesotten]

No problem, though i'd loose my first contribution to an open source project. Just a question, why at all use 2 different ways of implementing optional arguments ? Why not just default to a pointer ?

[dfaure-kdab]

OK I merged your commit and put my changes on top, so your contribution isn't lost ;-)

The reason for not using a pointer everywhere is historical. At the time I realized the missing support for optional types, existing applications were using values already, and changing it all to pointers would have broken them (since this is about the generated API, not just the internal implementation).