HTTP: Improved field-line parser #2037
Conversation
Replace with a Tokenizer based Parser and updated for RFC 9110 and RFC 9112 compliance.
There was a problem hiding this comment.
@yadij, before posting new master/v8 PRs, please cooperate on merging PRs (authored by others) that are awaiting your actions for weeks and even months. I have begged you many times already. Since you are not responding to my requests, I will stop making new ones, but the absence of such comments does not indicate that the problem is gone. When/if the problem is actually solved, I will write a dedicated comment.
| { | ||
| namespace One | ||
| { | ||
| class FieldParser : public Http::FieldParser |
There was a problem hiding this comment.
Please do not use inheritance for this API. There are problems with the suggested implementation, and the associated complexity is not needed. Just implement Http::One::FieldParser as a stand-alone class, "inlining" or "merging" Http::FieldParser class into Http::One::FieldParser.
There was a problem hiding this comment.
FTR: HTTP/2 addition to Squid is currently blocked on lack of src/http/Parser.h for the generic RFC 9110 parsing requirements currently stuck in src/http/one/Parser.h class with integrated RFC 9112 behaviour being imposed.
This class inheritance is necessary to prevent the same issues being compounded by this HTTP field parser imposing RFC 9112 behaviours on RFC 9113 and 9114 field parsers.
There was a problem hiding this comment.
The change request stands. The proposed class hierarchy is badly implemented and is currently unnecessary. We will split this class later if necessary.
P.S. HTTP/2 work is blocked for other reasons IMO, but we do not need to agree on that point.
|
|
||
| public: | ||
| /// parse an HTTP header field from the given tokenizer | ||
| void parseFieldLine(::Parser::Tokenizer &, const http_hdr_owner_type); |
There was a problem hiding this comment.
Please convert this method into a class constructor (instead of allowing special FieldParser instances that have not parsed anything at al or half-valid instances that have not finished parsing).
There was a problem hiding this comment.
No. The requested change would lock this parser to only being capable of parsing HTTP/1.x MiME based message headers. This is an RFC 9112 field-line parser, not a RFC 822 / RFC 2616 header parser.
There was a problem hiding this comment.
The requested change would lock this parser to only being capable of parsing HTTP/1.x MiME based message headers. This is an RFC 9112 field-line parser, not a RFC 822 / RFC 2616 header parser.
How would converting this method into constructor prevent this class from parsing RFC 9112 field-lines?
|
|
||
| if (!tok.atEnd()) { | ||
| const auto garbage = tok.remaining(); | ||
| const auto limit = min(garbage.length(), SBuf::size_type(100)); |
There was a problem hiding this comment.
Please move this dangerous and duplicated output limiting code to the Raw class itself, adding Raw::upTo(limit) or a similar method (where we can properly report the actual length and properly add ... to the truncated output).
There was a problem hiding this comment.
You have asked for this before, then vetoed the PR adding the requested change. Will not repeat here.
There was a problem hiding this comment.
I assume you are talking about #466. I did not veto that PR. In fact, I explicitly supported it. Once its problems are fixed, it should be merged (and used here).
| { | ||
| namespace One | ||
| { | ||
| class FieldParser : public Http::FieldParser |
There was a problem hiding this comment.
The change request stands. The proposed class hierarchy is badly implemented and is currently unnecessary. We will split this class later if necessary.
P.S. HTTP/2 work is blocked for other reasons IMO, but we do not need to agree on that point.
|
|
||
| public: | ||
| /// parse an HTTP header field from the given tokenizer | ||
| void parseFieldLine(::Parser::Tokenizer &, const http_hdr_owner_type); |
There was a problem hiding this comment.
The requested change would lock this parser to only being capable of parsing HTTP/1.x MiME based message headers. This is an RFC 9112 field-line parser, not a RFC 822 / RFC 2616 header parser.
How would converting this method into constructor prevent this class from parsing RFC 9112 field-lines?
|
|
||
| if (!tok.atEnd()) { | ||
| const auto garbage = tok.remaining(); | ||
| const auto limit = min(garbage.length(), SBuf::size_type(100)); |
There was a problem hiding this comment.
I assume you are talking about #466. I did not veto that PR. In fact, I explicitly supported it. Once its problems are fixed, it should be merged (and used here).
Replace with a Tokenizer based Parser and
updated for RFC 9110 and RFC 9112 compliance.