azadkuh / qhttp

a light-weight and asynchronous HTTP library (both server & client) in Qt5 and c++14
MIT License
484 stars 158 forks source link

Crash when fooling around after response->end() #17

Open hoensr opened 8 years ago

hoensr commented 8 years ago

Hi, here's a crash I have found: When in a request handler I do stuff after calling end() on the response, e.g. have an event loop run for a second, qhttp crashes on me. I have tried both the master and dev branch.

I think it is because end() calls the done() signal, which closes the socket. Afterwards, it still receives a readyRead(), and then it crashes in QSocket::bytesAvailable(), because itcpSocket is a dangling pointer containing garbage.

I attach a minimal example. Can you reproduce the issue? TestQhttp.zip

Here is the stack trace: > TestQhttp.exe!qhttp::details::QSocket::bytesAvailable() Line 100 C++ TestQhttp.exe!qhttp::server::QHttpConnectionPrivate::onReadyRead() Line 81 C++ TestQhttp.exe!qhttp::server::QHttpConnectionPrivate::initTcpSocket::__l3::<lambda>() Line 118 C++ TestQhttp.exe!QtPrivate::FunctorCall<QtPrivate::IndexesList<>,QtPrivate::List<>,void,void <lambda>(void) >::call(qhttp::server::QHttpConnectionPrivate::initTcpSocket::__l3::void <lambda>(void) f, void * * arg) Line 495 C++ TestQhttp.exe!QtPrivate::Functor<void <lambda>(void),0>::call<QtPrivate::List<>,void>(qhttp::server::QHttpConnectionPrivate::initTcpSocket::__l3::void <lambda>(void) & f, void * __formal, void * * arg) Line 553 C++ TestQhttp.exe!QtPrivate::QFunctorSlotObject<void <lambda>(void),0,QtPrivate::List<>,void>::impl(int which, QtPrivate::QSlotObjectBase * this_, QObject * r, void * * a, bool * ret) Line 193 C++ Qt5Cored.dll!QtPrivate::QSlotObjectBase::call(QObject * r, void * * a) Line 124 C++ Qt5Cored.dll!QMetaObject::activate(QObject * sender, int signalOffset, int local_signal_index, void * * argv) Line 3720 C++ Qt5Cored.dll!QMetaObject::activate(QObject * sender, const QMetaObject * m, int local_signal_index, void * * argv) Line 3596 C++ Qt5Cored.dll!QIODevice::readyRead() Line 160 C++ Qt5Networkd.dll!QAbstractSocketPrivate::canReadNotification() Line 737 C++ Qt5Networkd.dll!QAbstractSocketPrivate::readNotification() Line 69 C++ Qt5Networkd.dll!QAbstractSocketEngine::readNotification() Line 153 C++ Qt5Networkd.dll!QReadNotifier::event(QEvent * e) Line 1202 C++ Qt5Cored.dll!QCoreApplicationPrivate::notify_helper(QObject * receiver, QEvent * event) Line 1150 C++ Qt5Cored.dll!doNotify(QObject * receiver, QEvent * event) Line 1090 C++ Qt5Cored.dll!QCoreApplication::notify(QObject * receiver, QEvent * event) Line 1077 C++ Qt5Cored.dll!QCoreApplication::notifyInternal2(QObject * receiver, QEvent * event) Line 1015 C++ Qt5Cored.dll!QCoreApplication::sendEvent(QObject * receiver, QEvent * event) Line 227 C++ Qt5Cored.dll!qt_internal_proc(HWND__ * hwnd, unsigned int message, unsigned __int64 wp, __int64 lp) Line 399 C++ user32.dll!0000000077289c11() Unknown user32.dll!000000007728992a() Unknown Qt5Cored.dll!QEventDispatcherWin32::processEvents(QFlags<enum QEventLoop::ProcessEventsFlag> flags) Line 829 C++ Qt5Cored.dll!QEventLoop::processEvents(QFlags<enum QEventLoop::ProcessEventsFlag> flags) Line 129 C++ Qt5Cored.dll!QEventLoop::exec(QFlags<enum QEventLoop::ProcessEventsFlag> flags) Line 204 C++ Qt5Cored.dll!QCoreApplication::exec() Line 1285 C++ TestQhttp.exe!main(int argc, char * * argv) Line 11 C++ TestQhttp.exe!__tmainCRTStartup() Line 626 C TestQhttp.exe!mainCRTStartup() Line 466 C kernel32.dll!00000000773859bd() Unknown ntdll.dll!00000000774ba2e1() Unknown

azadkuh commented 8 years ago

yes, qhttp is designed to behave this way. for such scenario you need to check qhttp::server::QHttpConnection::disconnected() signal and stop any further processing on current instances of request, response and connection.

please have a a look at issue#15

hoensr commented 8 years ago

OK, so I have to do connect(request->connection(), &QHttpConnection::disconnected, this, &MyListener::FoolAround); and do my fooling around there. This works fine, thank you for clarifying this! And of course, thank you for the fast answer and for the great lib!

Oh, by the way, in order to compile the dev branch under Windows (VS 2013), I had to change some things. In httpparser.hxx line 98, I have replaced static auto me(http_parser* p) { by static TImpl* me(http_parser* p) {

And in httpwriter.hxx, I have replaced

        qhttp::for_each(TBase::iheaders.constBegin(), TBase::iheaders.constEnd(),
                [this](const auto& cit) {
            const QByteArray& field = cit.key();
            const QByteArray& value = cit.value();

            this->writeHeader(field, value);
        });

by

        QHashIterator<QByteArray, QByteArray> cit(TBase::iheaders);
        while (cit.hasNext()) {
            cit.next();
            const QByteArray& field = cit.key();
            const QByteArray& value = cit.value();

            this->writeHeader(field, value);
        }

You might want to consider this to make it more platform-independent.

azadkuh commented 8 years ago

humm, I do not have access to a Windows machine, i rarely have a chance to compile qhttp under Windows. I guess vs2013 support of c++14 (minimum requirement) is not complete, and you need mingw or vs2015 (community edition just works)

RafalNiewinski commented 8 years ago

yea, vs2015 and mingw works fine

hoensr commented 8 years ago

Sure, no problem. You're right, Microsoft is kind of slow in implementing the new features, e.g. the auto keyword.

I just wanted to propose that if you change these two things, it can be compiled without problem under VS2013 too, and I don't think there are any disadvantages. On the contrary, if you use QHashIterator, you can get rid of your qhttp::for_each construct.

But if you'd prefer not to, it's fine -- it's not a big deal for me to change it for myself.

azadkuh commented 8 years ago

generally it's good idea to support both c++14 compatible compilers and c++11 ones. needs some #ifdefs and conditional compiling.

qhttp::for_each is analogue to std::for_each and in my taste it's more c++ rather than QHashIterator (very java style!)

it's all about taste and conventions. Hopefully QHash, QMap, ... will be supported by range for in near future and we will get rid of qhttp::for_each() and QHashIterator.