Bound buffer for reading stats #14523
Conversation
knative-prow
bot
added
size/S
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14523 +/- ##
==========================================
+ Coverage 86.08% 86.13% +0.04%
==========================================
Files 196 196
Lines 14887 14889 +2
==========================================
+ Hits 12815 12824 +9
+ Misses 1761 1756 -5
+ Partials 311 309 -2
☔ View full report in Codecov by Sentry. |
| ProcessUptime: 12345.678, | ||
| Timestamp: 1697431278, | ||
| }, | ||
| expectedErr: "unmarshalling failed: unexpected EOF", |
There was a problem hiding this comment.
nit: any way to distinguish we hit the limit vs EOF on the wire?
There was a problem hiding this comment.
Not easily. Technically, we could also end up skipping a bunch of extra garbage after a valid proto, but since the proto contains a length at the start, we can tell whether we read the whole proto or not just by reading the beginning. (In fact, it would be nice if the generated proto code had a method that read from an io.Reader, but it doesn't seem to.)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, evankanderson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
the
approved
|
/cherrypick release-1.10 |
|
@evankanderson: new pull request created: #14541 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherrypick release-1.11 |
|
@evankanderson: new pull request created: #14542 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| @@ -82,7 +82,9 @@ func statFromProto(body io.Reader) (Stat, error) { | |||
| b := pool.Get().(*bytes.Buffer) | |||
| b.Reset() | |||
| defer pool.Put(b) | |||
| _, err := b.ReadFrom(body) | |||
| // 6 8-byte fields (+2 bytes marshalling), one hostname, 20 bytes extra space | |||
| r := io.LimitedReader{R: body, N: 6*10 + 256 + 20} | |||
There was a problem hiding this comment.
nit: Should not this be 255 for the max hostname? 🤔
There was a problem hiding this comment.
Extra-nit: you might need two bytes for the tag + length if the string is actually 256 bytes long, plus another possible tag + length for the entire message.
Proto doesn't have a way to express / enforce that a string is less than a certain size. The goal here is simply to avoid accidentally reading e.g. 5MB or more into an in-memory buffer when we know the actual report should be less than 1KB.
There was a problem hiding this comment.
Out of curiosity (haven't checked), what about using grpc instead?
Proposed Changes
Release Note