Re: [PATCH] HTTP/2.0 initial support

On 2013-11-19 08:42, Alex Rousskov wrote:
> On 11/15/2013 09:21 AM, Amos Jeffries wrote:
>> On 24/08/2013 3:08 a.m., Alex Rousskov wrote:
>>> Perhaps you misunderstood my suggestion because it does not lead to
>>> any
>>> huge changes. In fact, it would probably simplify your patch. I
>>> suggest
>>> that the existing request parser is adjusted to work like this:
>>>
>>> if (the buffered request content is too short is the magic prefix)
>>> keep accumulating as if we did not receive the end of headers
>>> else if (the buffered request content starts with the magic)
>>> create the corresponding header structures (PRI v2.0)
>>> else
>>> old code
>>>
>>> The above is done in the parser so no caller will have to be changed
>>> unless it has to be changed to handle HTTP2 PRI correctly anyway. And
>>> all changes in the callers will deal with regular header structures.
>>> No
>>> magic, no offset adjustments, no custom HTTP2 parsing code.
>
>
>> Maybe we are miss-aligning on our definition of "parser". I mean
>> HttpParser the class.
>
> I mean the request parsing code, starting with parseHttpRequest() that
> you modify.
>
>
>> The first problem here is that the HttpParser class is currently not
>> responsible for interpreting the first line. Only for splitting it up
>> into SP-delimited pieces and validating that the bytes forming each
>> are
>> valid for the particular positions.
>
> The design I propose does not depend on parsing or interpreting the
> "first line" of HTTP/2 messages. Please see the sketch below for
> details.
>

[ In the interests of simplification and proceeding faster I'm eliding
our side discussions and focusing back on your proposed change. ]

>> NOTE: The shortest possible HTTP/1.1 request is *shorter* by 8 bytes
>> than the HTTP/2.0 magic prefix. And the shortest possible request-line
>> (HTTP/0.9) is shorter by 17 bytes.
>
> Not a problem. The second part of my first "if" condition takes care of
> that: No HTTP/1 or HTTP/0 request-line can be an HTTP/2 prefix.
>
>
> Here are the same proposed conditions again, detailed further using a
> code sketch:
>
> const SBuf HttpParser::Http2magic = // 24 magic octets:
> "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n";
>
> bool HttpParser::tooShortForHttp2magic() const {
> return bufsiz < Http2magic.size();
> }
>
> bool HttpParser::isHttp2magicPrefix() const {
> return the ENTIRE buffer is an Http2magic prefix;

Does this mean?
     return HttpParser::Http2magic.cmp(buf, bufsiz) == 0;

> }
>
> bool HttpParser::startsWithHttp2magic() const {
> return the buffer starts with the ENTIRE Http2magic;

Does this mean?
     return HttpParser::Http2magic.cmp(buf,
HttpParser::Http2magic.size()) == 0;

> }
>
> ...
> parseHttpRequest()
> {
> if (hp->tooShortForHttp2magic() && hp->isHttp2magicPrefix())
> return NULL; // keep accumulating
>
> if (hp->startsWithHttp2magic()) {
> // create the corresponding header structures (HTTP/2 PRI)
> // this is the only place where HTTP/2 structures are created
> // the old code should be adjusted to fail on HTTP/2 requests
> // eventually, full HTTP/2 parser will be engaged here
> createHttp2request();
> } else {
> // old HTTP/[01] parsing/interpretation code stays here
> parseHttp1request();
> }
>
> common HTTP/* interpretation code, if any, stays here
> }
>
>
> The above is just a sketch, of course, but I hope it clarifies where I
> think the HTTP/2 detection should happen and how to leave the old code
> mostly unchanged. It can be polished to further and better separate
> HTTP/2 and HTTP/1 parsing, namespaces, interpretation, etc.
>

Thank you. That clarifies a lot what you are after and I agree it is a
nice way to go.

I still maintain that createHttp2request() and parseHttp1request() in
the current code will be duplicate code. Long term that will change, but
you were after preventing putting hopeful future preparation like that
into the patch on the first informal revision.

That said, if you are happy to let the short-term duplication go through
audit I am okay with it.

>>>>> Why prohibit PRI HTTP/1.1 requests? That method was not reserved
>>>>> and,
>>>>> hence, ought to be a valid extension method for HTTP1.
>>>>
>>>> If PRI is being used as a method in HTTP/1 that is a problem for the
>>>> HTTP/2 magic and we need to start rejecting it
>>>
>>> I do not see why handling HTTP/1.x PRI as before is a problem. What
>>> exactly will it suddenly break?
>
>> What breaks is HTTP/2 failure detection.
>
> Sorry, but I still do not understand. What exactly is this "HTTP/2
> failure" that you want to detect?
>

Client --2.0--> broken firewall --1.1--> Squid.

Where client sends valid magic and Squid receives:

  PRI * HTTP/1.1\r\n
  \r\n
  \r\n
  SM\r\n
  ...

or replace "broken firewall" with squid-2.7 server:

  PRI * HTTP/1.1\r\n
  X-Original-Version: HTTP/0.9\r\n
  <other HTTP/1.1 headers inserted by Squid>
  \r\n
  \r\n
  SM\r\n
  ...

Passing these requests upstream is wrong:
* may cause unknown other damaging side effects
* plain waste of resources of all types in this Squid and upstream
servers
* delays the transaction error detection
* may cause other implementers to try to "fix" the broken traffic
emitted by Squid

not to mention egg on our faces for not having dealt with it from this
initial code

Amos
Received on Mon Nov 18 2013 - 23:22:21 MST