Upgrade HTTP header field-line parser #1908
Conversation
.. with new Tokenizer based parser. Update specification references to latest RFCs. There are no behavioural differences from this. Also, remove one more SquidString allocate+copy. Although this is replaced with an SBuf allocate+copy so only bug fixes, no performance gain expected.
rousskov
changed the title
There was a problem hiding this comment.
I adjusted PR title/description to avoid saying that we are replacing a field parser (with some other parser): This PR changes our field parser code, but we cannot get rid of a field parser itself since it is a part of HTTP syntax, of course. I also fixed spelling.
I left one important high-level change request (about inheritance) and a minor one (about controversial TODO removal). I assume that failing CI tests imply implementation bugs that will need to be fixed as well.
Before advancing this new PR and requesting my re-review, please cooperate to merge PRs awaiting your review for a long time, including these three:
- Upgrade generated parse_line() to accept constant SBuf #1840 opened Jun 11, 2024: "Upgrade generated parse_line() to accept constant SBuf"
- Bug 5363: Handle IP-based X.509 SANs better #1793 opened Apr 25, 2024: "Bug 5363: Handle IP-based X.509 SANs better"
- Bug 5383: handleNegotiationResult() level-2 debugs() crash #1856 opened Jun 30, 2024: "Bug 5383: handleNegotiationResult() level-2 debugs() crash"
| * RFC 9112 section 5: | ||
| * field-line = field-name ":" OWS field-value OWS | ||
| */ | ||
| class FieldParser : public Http1::Parser |
There was a problem hiding this comment.
Please do not derive a field parser from a message prefix parser. There is no "X is a Y" relationship between these two concepts.
There was a problem hiding this comment.
Sigh. You vetoed extracting the useful parts of Http1::Parser out into a separate class this one should have been importing them from. I see this is going to have to be even simpler for you.
rousskov
added
the
S-waiting-for-author
... to a dedicated Tokenizer-based class.
Update specification references to latest RFCs. There are no
behavioral differences from this.
Also, remove one more SquidString allocate+copy. Although this
is replaced with an SBuf allocate+copy so only bug fixes, no
performance gain expected.