-
Notifications
You must be signed in to change notification settings - Fork 849
multiplexer: fix consume of too many bytes #11499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| TS_INLINE void | ||
| IOBufferReader::consume(int64_t n) | ||
| { | ||
| ink_assert(read_avail() >= n); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this function should handle this case, a). handle it as error case or b). consume only read_avail(). This assertion is a good start 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to suggestions, but I don't think there's a good way to handle this, not here in consume. There's not enough context. In production, we don't want to spend the time in the read_avail(). If needed the caller should do that as a part of fulfilling its contract for this API. The debug assert here seems like a reasonable way to catch issues in debug builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good.
IOBufferReader::consume contractually assumes that all callers of it will not consume more bytes than read_avail for the buffer chain. This adds a debug assertion for this and fixes multiplexer so that it doesn't violate this invariant.
e0ff10a to
cf084be
Compare
|
Cherry-picked to v10.0.x |
IOBufferReader::consume contractually assumes that all callers of it will not consume more bytes than read_avail for the buffer chain. This adds a debug assertion for this and fixes multiplexer so that it doesn't violate this invariant. (cherry picked from commit 6774984)
|
do we still need this on 9.2? |
This one liner is pretty valuable since it keeps multiplexer from potentially reading past the end of a buffer. We've never observed an issue in production, but better safe than sorry. |
IOBufferReader::consume contractually assumes that all callers of it will not consume more bytes than read_avail for the buffer chain. This adds a debug assertion for this and fixes multiplexer so that it doesn't violate this invariant. (cherry picked from commit 6774984)
|
9.2.x PR: |
IOBufferReader::consume contractually assumes that all callers of it will not consume more bytes than read_avail for the buffer chain. This adds a debug assertion for this and fixes multiplexer so that it doesn't violate this invariant. (cherry picked from commit 6774984) Co-authored-by: Brian Neradt <brian.neradt@gmail.com>
IOBufferReader::consume contractually assumes that all callers of it will not consume more bytes than read_avail for the buffer chain. This adds a debug assertion for this and fixes multiplexer so that it doesn't violate this invariant.