http_parser_pause causes feed to return wrong read count

[pezcode]

This is kinda weird and I don't know if it's a bug in http-parser or in the usage of http_parser_pause in httpxx.

I call rev(MSG_PEEK) which reads, say, 300 bytes. Request::feed returns 299 bytes read, although the header is really 300 bytes long (I doublechecked). The complete flags are both set (it has no body).
As a result, I recv 299 bytes (to remove the header from the TCP buffer), but that's one byte to few. In my next loop (thinking this is a pipelined request), I call recv which blocks and at some point returns with no data read if the connection was closed.

PS: I temporarily used the newest commit of http-parser but the bug is the same.

(Only noticed this now after adding tons of error checking. What looked like a slow connection was a timed out select because of the missing byte. I know I'm a terrible bug reporter lol)

[AndreLouisCaron]

Can you put up an example request that reproduces the behavior? I step through the code and see if I can diagnose the problem. It's possible that forcing the parser to pause prevents it from consuming the last byte. I remember seeing some special cases in the http-parser code where the parser doesn't consume the last byte for obscure reasons.

[pezcode]

The problem code in http-parser is at case s_headers_almost_done:

    if (HTTP_PARSER_ERRNO(parser) != HPE_OK) {
      return p - data;
    }

    goto reexecute_byte;

After returning from the callback, errno is HPE_PAUSED, thus it returns p - data (which is one too few).
Normally, at reexecute_byte, it would execute case s_headers_done (right below), break and return len (the original size input)

[pezcode]

I just picked the first header the browser threw at me:

GET http://filehippo.com/ HTTP/1.1
Accept: text/html, application/xhtml+xml, /
Accept-Language: de-DE
User-Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; WOW64; Trident/5.0)
Accept-Encoding: gzip, deflate
Proxy-Connection: Keep-Alive
Host: filehippo.com
Cookie: FHSession=hemorha1nbrr3m45rsgs1fvx; __utmc=144473122; FH_PreferredCulture=en-US; __utma=144473122.1735561034.1330017429.1330117138.1330123514.6; __utmz=144473122.1330017429.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none); __utmb=144473122.1.10.1330123514

It ends regularly with one empty line (\r\n), and has no body.
Its size is 0x21e, and feed returns 0x21d.

[pezcode]

I'm guessing http-parser returns because if it didn't, it would additionally call on_message_complete for messages with an empty body, which is undesirable after explicitly pausing the parser.

Not sure why it returns one byte short though.

Either way, this complicates parsing a little because even if it returned the right byte count, you'd always have to call feed with size 0 to trigger on_message_complete for messages without body.

And that's how a seemingly simple design change blows up in everyone's faces :v

[AndreLouisCaron]

After stepping through the code, I realized that it stops one byte short because I force the parser to stop after the headers are completed (I use the parser's http_parser_pause() function from the on_headers_complete() callback).

The relevant piece of code starts at http_parser.c, line 1546. It's an implementation detail, but the pausing is actually implemented by setting a "temporary" error code, which causes the if branch at http_parser.c, line 1561 to return early.

Indeed, processing the last byte would effectively start the body. Thus, I'm closing this as "by design". Check out the feed loops in the test cases for a simple way to properly process an entire message body,

[pezcode]

Just for clarification...

Doesn't this make telling the header size of a message pretty much impossible? My whole point with the proposed changes was to be able to more precisely tell them apart.

This is my train of thought, anyway:

• Give the parser 300 bytes. If the header ends somewhere before that, return.
• If there is no body, both header_complete and complete should now be set.
• Give the parser the rest of the bytes. If the body ends somewhere before that, return.

[AndreLouisCaron]

No, it's definitely possible. When the headers complete, the parser is stopped, forcing the feed() method to return. So the first time that headers_complete() is true after a call to feed(), the parser has just finished consuming exactly the amount of bytes in the header.

    // Assume the following context (possible data members),
    http::Request request;
    std::size_t head_size = 0;
    std::size_t body_size = 0;

    // and the following callback that processes incoming data
    // (possible a corresponding member function).
    void consume_data ( const char * data, std::size_t size )
    {
        // Track total amount of bytes in 'data' used so far.
        // Invariant: 0 <= used <= size.
        std::size_t used = 0;

        // If we haven't finished reading the headers,
        // keep on feeding the request object.
        if (!request.complete())
        {
            std::size_t pass = 0;
            while ((used < size) && !request.headers_complete())
            {
                pass = request.feed(data+used, size-used);

                used += pass, head_size += pass;
            }

            if (request.headers_complete())
            {
                std::cout
                    << "Head was " << head_size << " bytes."
                    << std::endl;
            }
        }

        // Finished reading the headers, now process the request body.
        while ((used < size) && !request.complete())
        {
            pass = request.feed(data+used, size-used);

            used += pass, body_size += pass;
        }

        if (request.complete())
        {
            std::cout
                << "Body was " << head_size << " bytes."
                << std::endl;

            // ...

            // Prepare for handling another request.
            request.clear();
        }

Although, now that I think about this again, I think this will incorrectly return 1 as the body size for a request with an empty body. If so, then I'll see if I can patch the http-parser library to handle this case especially.

[AndreLouisCaron]

Oops, the second loop should print body_size, not head_size. And there's a missing closing brace } at the end of the snippet.

[pezcode]

The problem is that it returns 1 byte too short but sets the header_complete flag.
Now feed tells me it read 99 bytes and the header is complete. That's a bug if you ask me.
Even worse, if I receive EOF, and pass size 0 to feed, http_parser_execute will return 0 (because state != HPE_OK). It didn't process anything, but the error check in feed doesn't fire and won't unpause the parser.

I'm starting to think the pause function in http-parser is a little half-assed. Is there documentation for http-parser somewhere?

[AndreLouisCaron]

Yup, it seems like a recent addition to the library. Unfortunately, the http-parser project suffers from "the documentation is the code" syndrome, though there are some comments in the http_parser.h file with the following deluxe documentation for the pausing function:

/* Pause or un-pause the parser; a nonzero value pauses */
void http_parser_pause(http_parser *parser, int paused);

I'll report a bug on the http-parser issue tracker linking here and see how that goes.

[AndreLouisCaron]

There is a somewhat related issue on the http-parser issue tracker about detecting the exact position of the end of headers in a request. However, it seems there is some controversy related to the suggested fix.

[AndreLouisCaron]

I submitted this issue to the http-parser bug tracker. We'll see if we can get a library fix for this.

[AndreLouisCaron]

I pushed an experimental fix for this issue in branch raw-message-sizes. See commit 608af01eee7cfdad579381f61edf70778eedaa58. Preliminary testing shows that it works, though I haven't tested with chunked requests.