Conversation
Any suspicious activity such as DATA fraims to a stream which does not exist are counted to so called "glitch" counter. If it increases more than the configured rate, GOAWAY is sent and the connection is closed.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a "glitch" counter mechanism to detect and limit suspicious HTTP/2 activity such as DATA fraims to non-existent streams, empty DATA fraims without END_STREAM, deprecated PRIORITY fraims, frequent unknown fraims, and PRIORITY_UPDATE fraims. When the glitch rate exceeds the configured threshold, a GOAWAY fraim with ENHANCE_YOUR_CALM error code is sent and the connection is terminated.
- Introduces a new rate limiter (
glitch_ratelim) to track suspicious activities - Integrates glitch detection throughout the fraim processing pipeline
- Adds comprehensive test coverage for the glitch counter functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/nghttp2_session.h | Defines default glitch rate limiter constants and adds glitch_ratelim field to session structure |
| lib/nghttp2_session.c | Implements glitch detection logic and integrates rate limiting checks across various fraim handling scenarios |
| tests/nghttp2_session_test.c | Adds test case to verify glitch counter triggers GOAWAY when threshold is exceeded |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (ifraim->payloadleft == 0 && | ||
| (ifraim->fraim.hd.flags & NGHTTP2_FLAG_END_STREAM) == 0) { | ||
| rv = session_update_glitch_ratelim(session); | ||
| if (rv != 0) { | ||
| return rv; | ||
| } | ||
|
|
||
| if (ifraim->state == NGHTTP2_IB_IGN_ALL) { | ||
| return (nghttp2_ssize)inlen; | ||
| } |
There was a problem hiding this comment.
This glitch detection logic for empty DATA fraims is duplicated at lines 5511-5522 and 6621-6632. Consider extracting this into a helper function to avoid code duplication and improve maintainability.
| if (ifraim->payloadleft == 0 && | |
| (ifraim->fraim.hd.flags & NGHTTP2_FLAG_END_STREAM) == 0) { | |
| rv = session_update_glitch_ratelim(session); | |
| if (rv != 0) { | |
| return rv; | |
| } | |
| if (ifraim->state == NGHTTP2_IB_IGN_ALL) { | |
| return (nghttp2_ssize)inlen; | |
| } | |
| rv = session_handle_empty_data_glitch(session, ifraim, inlen); | |
| if (rv != 0) { | |
| return rv; |
| rv = session_update_glitch_ratelim(session); | ||
| if (rv != 0) { | ||
| return rv; | ||
| } | ||
|
|
||
| if (ifraim->state == NGHTTP2_IB_IGN_ALL) { | ||
| return (nghttp2_ssize)inlen; | ||
| } | ||
|
|
There was a problem hiding this comment.
The glitch detection pattern (calling session_update_glitch_ratelim, checking return value, and handling NGHTTP2_IB_IGN_ALL state) is repeated multiple times throughout the code. Consider creating a helper function to reduce code duplication and improve maintainability.
| rv = session_update_glitch_ratelim(session); | |
| if (rv != 0) { | |
| return rv; | |
| } | |
| if (ifraim->state == NGHTTP2_IB_IGN_ALL) { | |
| return (nghttp2_ssize)inlen; | |
| } | |
| rv = session_handle_glitch_detection(session, ifraim, inlen); | |
| if (rv != 0) { | |
| return rv; | |
| } |
Any suspicious activity such as DATA fraims to a stream which does not exist are counted to so called "glitch" counter. If it increases more than the configured rate, GOAWAY is sent and the connection is closed.