http: only accept HTTP client magic at the start of buffer#8232
http: only accept HTTP client magic at the start of buffer#8232zuercher merged 7 commits intoenvoyproxy:masterfrom
Conversation
Risk Level: low Testing: added test case Docs Changes: n/a Release Notes: n/a Fixes: envoyproxy#8229 Signed-off-by: Stephan Zuercher <zuercher@gmail.com>
| // us the first few bytes of the HTTP/2 prefix since in all public cases we use SSL/ALPN. For | ||
| // internal cases this should practically never happen. | ||
| if (-1 != data.search(Http2::CLIENT_MAGIC_PREFIX.c_str(), Http2::CLIENT_MAGIC_PREFIX.size(), 0)) { | ||
| if (0 == data.search(Http2::CLIENT_MAGIC_PREFIX.c_str(), Http2::CLIENT_MAGIC_PREFIX.size(), 0)) { |
There was a problem hiding this comment.
This is no less performant than the previous, but we could avoid a lot of comparisons if we added a method to Buffer that implemented comparison at an exact spot, something like:
if (data.rangeEquals(0, Http2::Client_MAGIC_PERFIX.size(), Http2::CLIENT_MAGIC_PREFIX.c_str()) {
return Http2::ALPN_STRING;
}
(I could also use linearize and memcmp, but would have to modify several callsites to pass a non-const Buffer reference to allow linearize to be called here).
What do folks think?
There was a problem hiding this comment.
How about using copyOut to a stack allocated char[] and compare with CLIENT_MAGIC_PREFIX? CLIENT_MAGIC_PREFIX is short so that shouldn't cost much.
There was a problem hiding this comment.
I wonder if we even need to handle arbitrary points, and can just have something like a prefix check. I suspect that's what we want most of the time.
Either way I'm happy with this change as an incremental improvement. My one request would be a release note (version history note, whaever) just because it fixes a bug on the data plane, and no one knows if someone is depending on this particular bug in production.
There was a problem hiding this comment.
Added Buffer::startsWith and switch to using that.
|
@alyssawilk do you mind taking a look at this? |
Signed-off-by: Stephan Zuercher <zuercher@gmail.com>
Signed-off-by: Stephan Zuercher <zuercher@gmail.com>
Signed-off-by: Stephan Zuercher <zuercher@gmail.com>
Signed-off-by: Stephan Zuercher <zuercher@gmail.com>
alyssawilk
left a comment
There was a problem hiding this comment.
+1 to stringview, and one other nit but otherwise looks good!
Signed-off-by: Stephan Zuercher <zuercher@gmail.com>
Signed-off-by: Stephan Zuercher <zuercher@gmail.com>
…y#8232) http: only accept HTTP client magic at the start of buffer Risk Level: low Testing: added test case Docs Changes: n/a Release Notes: updated Fixes: envoyproxy#8229 Signed-off-by: Stephan Zuercher <zuercher@gmail.com>
…y#8232) http: only accept HTTP client magic at the start of buffer Risk Level: low Testing: added test case Docs Changes: n/a Release Notes: updated Fixes: envoyproxy#8229 Signed-off-by: Stephan Zuercher <zuercher@gmail.com>
…y#8232) http: only accept HTTP client magic at the start of buffer Risk Level: low Testing: added test case Docs Changes: n/a Release Notes: updated Fixes: envoyproxy#8229 Signed-off-by: Stephan Zuercher <zuercher@gmail.com>
Risk Level: low
Testing: added test case
Docs Changes: n/a
Release Notes: updated
Fixes: #8229
Signed-off-by: Stephan Zuercher zuercher@gmail.com