Skip to content

Add better frame size validation for AMQP#2167

Merged
cshannon merged 3 commits into
apache:mainfrom
cshannon:frame-size-validation
Jul 2, 2026
Merged

Add better frame size validation for AMQP#2167
cshannon merged 3 commits into
apache:mainfrom
cshannon:frame-size-validation

Conversation

@cshannon

@cshannon cshannon commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Add a negative size check for AMQP NIO transports. This also adds a similar check to OpenWire. For OpenWire the same IllegalArgumentException is thrown but this improves the error message in the NIO transport.

Add a negative size check for AMQP NIO transports.
This also adds a similar check to OpenWire. For OpenWire
the same IllegalArgumentException is thrown but this
improves the error message in the NIO transport.
tabish121
tabish121 previously approved these changes Jul 1, 2026

@tabish121 tabish121 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes seem fine, unsure how well covered this is in the test suite though so not sure if you want to add any additional tests for these conditions.

@cshannon

cshannon commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

The changes seem fine, unsure how well covered this is in the test suite though so not sure if you want to add any additional tests for these conditions.

I didn't add anything because I didn't think it would be very easy to fake a negative value for AMQP with the client, unless you have any suggestions. I suppose just doing some mocking or calling the validation directly would be the only option otherwise.

@tabish121

Copy link
Copy Markdown
Contributor

The changes seem fine, unsure how well covered this is in the test suite though so not sure if you want to add any additional tests for these conditions.

I didn't add anything because I didn't think it would be very easy to fake a negative value for AMQP with the client, unless you have any suggestions. I suppose just doing some mocking or calling the validation directly would be the only option otherwise.

Why would you need a client when AmqpWireFormat

The changes seem fine, unsure how well covered this is in the test suite though so not sure if you want to add any additional tests for these conditions.

I didn't add anything because I didn't think it would be very easy to fake a negative value for AMQP with the client, unless you have any suggestions. I suppose just doing some mocking or calling the validation directly would be the only option otherwise.

I don't think you'd need a client or proton to test it since the NIO bit reads from a DataInput and the AmqpFrameParser reads from a ByteBuffer so you can just simulate the values

@tabish121 tabish121 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@cshannon cshannon merged commit 6e9dd5b into apache:main Jul 2, 2026
10 checks passed
@cshannon cshannon deleted the frame-size-validation branch July 2, 2026 00:24
@github-project-automation github-project-automation Bot moved this from Backlog to Done in Apache ActiveMQ v6.3.0 Jul 2, 2026
cshannon added a commit that referenced this pull request Jul 2, 2026
Add a negative size check for AMQP NIO transports.
This also adds a similar check to OpenWire. For OpenWire
the same IllegalArgumentException is thrown but this
improves the error message in the NIO transport.

(cherry picked from commit 6e9dd5b)
cshannon added a commit that referenced this pull request Jul 2, 2026
Add a negative size check for AMQP NIO transports.
This also adds a similar check to OpenWire. For OpenWire
the same IllegalArgumentException is thrown but this
improves the error message in the NIO transport.

(cherry picked from commit 6e9dd5b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants