Why does QSharedPointer<T>::create call destructor of incomplete object?

Question

I have following code example:

#include <QCoreApplication>
#include <QSharedPointer>
#include <QDebug>

#include <memory>

class A
{
public:
    A()
    {
        throw 1;
    }
    ~A() { qDebug() << "A destr"; }
};

int main(int argc, char* argv[])
{
    QCoreApplication a(argc, argv);

    try
    {
        //auto m1 = std::make_shared<A>();
        auto m2 = QSharedPointer<A>::create();
    }
    catch (...)
    {
        qDebug() << "catch!";
    }

    return a.exec();
}

The output for the above code is:

A destr
catch!

But if I uncomment the line with std::make_shared the output is following:

catch!

So why does QSharedPointer::create call destructor of incomplete object? Is that a bug or I'm missing something?

I tried it with MSVC2013 + Qt 5.5.1 and MSVC2015 + Qt 5.6 (built from sources). The result is the same.

Correlation

For reference the source code for your particular version of QSharedPointer::create() is here: code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/… - I assume you're building without QT_SHAREDPOINTER_TRACK_POINTERS defined? – Prebo

And here is a possibly-related Qt bug filed five years ago (with no activity since): bugreports.qt.io/browse/QTBUG-14637 – Prebo

@JohnZwinck Yes, it's not defined. As I understand this flag is used to debug multiple smart pointers owning the same object. Or should I give it a try anyway? – Correlation

No need to define it--I was just trying to understand which code path you're actually using. It's clear now that there is a bug in Qt, which I have described in my answer below. – Prebo

@JohnZwinck Thanks, didn't update the page before comment. – Correlation

Answer 1

It appears you have found a bug in Qt. I suggest you file a bug report and reference this somewhat related bug: https://bugreports.qt.io/browse/QTBUG-14637

The problem seems to be in http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qsharedpointer_impl.h?h=v5.5.1#n420 - whose simplified code looks like this:

static inline QSharedPointer create()
{
    typedef QtSharedPointer::ExternalRefCountWithContiguousData<T> Private;
    typename Private::DestroyerFn destroy = &Private::deleter;

    QSharedPointer result(Qt::Uninitialized);
    result.d = Private::create(&result.value, destroy);

    new (result.data()) T();
    result.d->setQObjectShared(result.value, true);
    result.enableSharedFromThis(result.data());
    return result;
}

It's a little complicated with references to other functions (mostly in the same file), but it appears that deleter is stored in result before the constructor is called by placement new. When your constructor throws, your object is never completely constructed, but the QSharedPointer result is constructed already, and contains the deleter. From there it's a short hop to the deleter function:

static void deleter(ExternalRefCountData *self)
{
    ExternalRefCountWithContiguousData *that =
            static_cast<ExternalRefCountWithContiguousData *>(self);
    that->data.~T();
}

And now your destructor is called, despite your constructor never having completed. That's undefined behavior. If you're unlucky, this will corrupt your application state (because it goes against the rule that a destructor is only called if a constructor runs to completion--a rule some class types may rely on).

A possible fix (which I haven't tested, but you can) is:

static void noOpDeleter(ExternalRefCountData *self)
{
    Q_UNUSED(self);
}

static inline QSharedPointer create()
{
    typedef QtSharedPointer::ExternalRefCountWithContiguousData<T> Private;
    typename Private::DestroyerFn noDestroy = &noOpDeleter;
    typename Private::DestroyerFn destroy = &Private::deleter;

    QSharedPointer result(Qt::Uninitialized);
    result.d = Private::create(&result.value, noDestroy);

    new (result.data()) T();
    result.d->destroyer = destroy;
    result.d->setQObjectShared(result.value, true);
    result.enableSharedFromThis(result.data());
    return result;
}

If you can validate the above, you should feel free to weave it into a patch and submit it to the Qt bug tracker. Hopefully with a working patch attached they'll accept it promptly.

Prebo

It doesn't take ill luck: calling the destructor of an object that isn't alive is undefined behavior. The lifetime of an object begins after its constructor completes successfully. In many cases, that UB may be innocuous, but it is UB regardless. – Locklear

@Yakk: Thanks for that, I've updated my answer to reflect your point. – Prebo

Answer 2

Finally, we will have it fixed! I gues it would be Qt 5.8.2 or Qt 5.9.

Thanks @JohnZwinck, your idea works just fine.

Mctyre