Skip to content
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

mc: increase utf8 scanning limit for longstr conversions. #11715

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

kjnilsson
Copy link
Contributor

The AMQP 0.9.1 longstr type is problematic as it can contain arbitrary binary data but is typically used for utf8 by users.

The current conversion into AMQP avoids scanning arbitrarily large longstr to see if they only contain valid utf8 by treating all longstr data longer than 255 bytes as binary. This is in hindsight too strict and thus this commit increases the scanning limit to 4096 bytes - enough to cover the vast majority of AMQP 0.9.1 header values.

This change also conversts the AMQP binary types into longstr to ensure that existing data (held in streams for example) is converted to an AMQP 0.9.1 type most likely what the user intended.

The AMQP 0.9.1 longstr type is problematic as it can contain arbitrary
binary data but is typically used for utf8 by users.

The current conversion into AMQP avoids scanning arbitrarily large
longstr to see if they only contain valid utf8 by treating all
longstr data longer than 255 bytes as binary. This is in hindsight
too strict and thus this commit increases the scanning limit to
4096 bytes - enough to cover the vast majority of AMQP 0.9.1 header
values.

This change also conversts the AMQP binary types into longstr to
ensure that existing data (held in streams for example) is converted
to an AMQP 0.9.1 type most likely what the user intended.
@ansd
Copy link
Member

ansd commented Jul 15, 2024

This change also conversts the AMQP binary types into longstr to ensure that existing data (held in streams for example) is converted to an AMQP 0.9.1 type most likely what the user intended.

... that's perfectly fine since according to the AMQP 0.9.1 spec:

Long strings, used to hold chunks of binary data

@ansd ansd merged commit 131379a into main Jul 15, 2024
269 checks passed
@ansd ansd deleted the mc-longstr-adjustments branch July 15, 2024 12:07
@michaelklishin
Copy link
Member

Looks like if both labels are set, our greatly simplified Mergify backporting setup does nothing 😅

@Mergifyio backport v4.0.x v3.13.x

@acogoluegnes
Copy link
Contributor

@Mergifyio backport v4.0.x v3.13.x

Copy link

mergify bot commented Jul 17, 2024

backport v4.0.x v3.13.x

✅ Backports have been created

acogoluegnes added a commit that referenced this pull request Jul 17, 2024
mc: increase utf8 scanning limit for longstr conversions. (backport #11715)
acogoluegnes added a commit to rabbitmq/rabbitmq-stream-java-client that referenced this pull request Jul 17, 2024
github-actions bot pushed a commit to rabbitmq/rabbitmq-stream-java-client that referenced this pull request Jul 17, 2024
acogoluegnes added a commit that referenced this pull request Jul 17, 2024
mc: increase utf8 scanning limit for longstr conversions. (backport #11715)
Gsantomaggio added a commit to rabbitmq/rabbitmq-stream-dotnet-client that referenced this pull request Jul 25, 2024
Update the test conversion from amqp 091 to stream due of rabbitmq/rabbitmq-server#11715

Signed-off-by: Gabriele Santomaggio <g.santomaggio@gmail.com>
Gsantomaggio added a commit to rabbitmq/rabbitmq-stream-dotnet-client that referenced this pull request Jul 25, 2024
* Update RabbitMQ version and the test
* Update the test conversion from amqp 091 to stream due of rabbitmq/rabbitmq-server#11715
---------

Signed-off-by: Gabriele Santomaggio <g.santomaggio@gmail.com>
acogoluegnes added a commit to rabbitmq/rabbitmq-stream-java-client that referenced this pull request Jul 26, 2024
github-actions bot pushed a commit to rabbitmq/rabbitmq-stream-java-client that referenced this pull request Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants