Skip to content

Conversation

@bneradt
Copy link
Contributor

@bneradt bneradt commented Jul 1, 2024

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.

@bneradt bneradt added Core multiplexer multiplexer plugin labels Jul 1, 2024
@bneradt bneradt added this to the 10.1.0 milestone Jul 1, 2024
@bneradt bneradt self-assigned this Jul 1, 2024
TS_INLINE void
IOBufferReader::consume(int64_t n)
{
ink_assert(read_avail() >= n);
Copy link
Contributor

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 👍

Copy link
Contributor Author

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.

masaori335
masaori335 previously approved these changes Jul 5, 2024
Copy link
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good.

@masaori335 masaori335 dismissed their stale review July 5, 2024 00:40

Start wondering multiplexer change.

@bryancall bryancall requested a review from serrislew August 5, 2024 22:29
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.
@bneradt bneradt force-pushed the add_debug_assert_in_consume branch from e0ff10a to cf084be Compare August 6, 2024 23:06
@bneradt bneradt merged commit 6774984 into apache:master Aug 7, 2024
@bneradt bneradt deleted the add_debug_assert_in_consume branch August 7, 2024 16:52
@cmcfarlen cmcfarlen modified the milestones: 10.1.0, 10.0.0 Aug 12, 2024
@cmcfarlen
Copy link
Contributor

Cherry-picked to v10.0.x

cmcfarlen pushed a commit that referenced this pull request Aug 12, 2024
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)
@ezelkow1
Copy link
Member

do we still need this on 9.2?

@bneradt
Copy link
Contributor Author

bneradt commented Jan 16, 2025

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.

ezelkow1 pushed a commit to ezelkow1/trafficserver that referenced this pull request Jan 16, 2025
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)
@bneradt
Copy link
Contributor Author

bneradt commented Jan 16, 2025

9.2.x PR:
#11965

ezelkow1 added a commit that referenced this pull request Jan 17, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Core multiplexer multiplexer plugin

Projects

Status: picked-10.0.0

Development

Successfully merging this pull request may close these issues.

5 participants