Loop parsing POST contains request with two "readyRead"

[AlexObukhoff]

Tufao version 1.4.1

I discovered problem with some browsers. Create simple web-server handled POST request. If the client sends data to two packages I take loop parsing request in void HttpServerRequest::onReadyRead().

Example:

First buffer from priv->socket.readAll();

POST https://127.0.0.1:45678/sign HTTP/1.1
Referer: https://somesite.com/test/
Origin: https://somesite.com
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/538.1 (KHTML, like Gecko) QupZilla/1.8.9 Safari/538.1
Content-Type: application/xml
Accept: */*
Content-Length: 47
Accept-Language: ru-RU,ru;q=0.8
Connection: Keep-Alive
Accept-Encoding: gzip, deflate
Host: 127.0.0.1:45678

Second buffer must be

123456789 AUTH
DATE=2015-02-17 17:59
IP=1.2.3.4

But he had to be read because we are still spinning in the processing cycle of the first buffer is constantly falling on the condition:

        case http::token::code::error_insufficient_data:
                continue;

[cono]

There should be definitely "return" instead of "continue". As we are blocking our event loop totally. Easy to reproduce, run the program and telnet to the port and start slowly typing:

GET / HTTP/1.1
Host: example.com

Thats all, 100% CPU will be taken by the program. If you take one connection, with "continue", it will not take any additional connections. If you put "return" over there instead, it will accept other connections with no prob.

[cono]

A bit more code required, final change which works for me:

% git diff
diff --git a/src/httpserverrequest.cpp b/src/httpserverrequest.cpp
index 2f0c5f6..cd03954 100644
--- a/src/httpserverrequest.cpp
+++ b/src/httpserverrequest.cpp
@@ -128,6 +128,7 @@ void HttpServerRequest::onReadyRead()
         priv->timer.start(priv->timeout);

     priv->buffer += priv->socket.readAll();
+       priv->parser.reset();
     priv->parser.set_buffer(asio::buffer(priv->buffer.data(),
                                          priv->buffer.size()));

@@ -139,7 +140,7 @@ void HttpServerRequest::onReadyRead()
         priv->parser.next();
         switch(priv->parser.code()) {
         case http::token::code::error_insufficient_data:
-            continue;
+            return;
         case http::token::code::error_set_method:
             qFatal("unreachable");
             break;

[AlexObukhoff]

Thanks

[vinipsmaker]

+ priv->parser.reset();

The "fix" intended by this change... it's kinda... meh... We don't need to buffer old HTTP data. That's the reason why the parser is interruptable by design.

Anyway. Thank you very much for point this bug. I applied a slightly different fix (properly break out of the loop so the forever-loop bug if fixed and the parsed buffer data is properly removed and possibly emit some of the signals if they are ready[1]): https://github.com/vinipsmaker/tufao/commit/0b1ca859b60c6a9e77326a7478f2cf8364f2cfc1

On Boost.Http, I have a mock socket where I can setup a list of buffers to feed the "HttpServerRequest" and later run all unit tests ranging from buffer being feed from one byte at a time until the whole buffer size (where data is always "broken" at elements boundaries on the list). It'd be very useful to have this kind of mock socket in Qt. At least I'm using a similar usage pattern of the parser itself and the project is more used here.

[1] token::code::error_insufficient_data means insufficient data for current token only. It's possible that old tokens were enough to at least emit ready

[vinipsmaker]

Btw, I asked @skhaz if he'd be interested in maintaining this project and he refused. So you guys better keep this active approach and not wait/depend for me, as I dedicate a very small amount of time on this project nowadays.