transport: http2 server must validates header list size when early aborting stream #8769
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8769 +/- ##
==========================================
- Coverage 83.42% 83.32% -0.11%
==========================================
Files 419 418 -1
Lines 32556 32875 +319
==========================================
+ Hits 27161 27394 +233
- Misses 4015 4086 +71
- Partials 1380 1395 +15
🚀 New features to boost your workflow:
|
easwars
added
Type: Bug
Area: Transport
|
@eshitachandwani: I'm moving this to @arjan-bal for second review as he was the one who filed the issue in the first place. |
joybestourous
force-pushed
the
joy.bestourous/header-check
branch
from
41d5c22 to
2ceced5
Compare
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
Mostly looks good, left some minor comments.
| } | ||
| if p := istatus.RawStatusProto(stat); len(p.GetDetails()) > 0 { | ||
| stBytes, err := proto.Marshal(p) | ||
| if err == nil { |
There was a problem hiding this comment.
nit: Can you please bring back the error log when protobuf serialization fails?
internal/transport/http2_server.go
Outdated
| func (t *http2Server) checkForHeaderListSize(it any) bool { | ||
| if t.maxSendHeaderListSize == nil { | ||
| // checkForHeaderListSize checks if the header list size exceeds the limit set | ||
| // by the peer. It returns false if the limit is exceeded. | ||
| func checkForHeaderListSize(hf []hpack.HeaderField, maxSendHeaderListSize *uint32) bool { |
There was a problem hiding this comment.
Instead of adding a new function that accepts a []hpack.HeaderField, can the existing method be updated to func (t *http2Server) checkForHeaderListSize(hf []hpack.HeaderField) bool?
That should make diff smaller.
internal/transport/http2_server.go
Outdated
| status: status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()), | ||
| rst: !frame.StreamEnded(), | ||
| }) | ||
| t.writeEarlyAbort(s.id, s.contentSubtype, status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error()), http.StatusOK, !frame.StreamEnded()) |
There was a problem hiding this comment.
nit: Can you move the definition of status to a new line to keep this line short?
st := status.New(codes.DeadlineExceeded, context.DeadlineExceeded.Error())
t.writeEarlyAbort(/*omitted*/)
internal/transport/http2_server.go
Outdated
| } | ||
|
|
||
| // buildEarlyAbortHF builds the header fields for an early abort response. | ||
| func buildEarlyAbortHF(httpStatus uint32, contentSubtype string, stat *status.Status) []hpack.HeaderField { |
There was a problem hiding this comment.
nit: Since there's only one caller for this function, can we merge it into writeEarlyAbort? We can split the method in the future if it becomes too long.
4 participants
Modifies
earlyAbortStreamHandlerto include check the header list size when early aborting, and returns a RST_STREAM if the max size is exceeded.Fixes #8766
RELEASE NOTES: