Return an error early if header list is overrun#912
Conversation
|
Ok, looking into the test failures. Didn't realize there's more than just |
IgorBelyi
force-pushed
the
shortcut-header-list-oversize
branch
from
922e5c7 to
6a48c58
Compare
| ) -> Result<(), DecoderError> | ||
| where | ||
| F: FnMut(Header), | ||
| F: FnMut(Header) -> bool, |
There was a problem hiding this comment.
I think returning a bool can be somewhat confusing, both for the calling (do I return true or false), and when it's used (what does that if mean again).
I'd suggest either using ControlFlow, or a Result. I think. What do you think of that?
There was a problem hiding this comment.
I was trying to stick with smallest change possible but ControlFlow indeed makes sense. Will switch.
| false | ||
| } | ||
| } { | ||
| break; |
There was a problem hiding this comment.
What's the effects of skipping the rest? Does it the table get out of sync?
Would it safer to have some heuristic that just says, "oversize by some we'll just ignore and reset you, but oversize by double (or whatever) and we'll GOAWAY your connection, you bad bad peer"?
There was a problem hiding this comment.
I'm not sure about that. Will need to take a closer look at how it fits into global flow. I was trusting tests to catch anything fishy and just wanted to improve performance to skip unnecessary code.
|
Closing in favor of a better thought through #915 |
2 participants
I've ran into a problem that excessive headers still consume CPU even after condition of exceeding
max_header_list_sizeis detected. This PR is a proposed fix for that problem. It allows closure passed intoDecoder::decode()to report that it won't be accepting any more headers thus allowing decoder to return earlier skipping unnecessary memory allocations. I did verify locally that CPU overrun is addressed with this change.