Skip to content

Comments

Fix an indexing panic in compression#578

Merged
seanmonstar merged 1 commit intotower-rs:mainfrom
sulami:fix-compression-panic
Jun 3, 2025
Merged

Fix an indexing panic in compression#578
seanmonstar merged 1 commit intotower-rs:mainfrom
sulami:fix-compression-panic

Conversation

@sulami
Copy link
Contributor

@sulami sulami commented Jun 3, 2025

This guard was backwards, not only breaking the functionality it was intended to provide, but also causing panics under certain conditions.

In this case, the haystack is the pre-existing Vary header on the response, and the needle is "Accept-Econding".

If we are compressing a response, and there is already a Vary header on the response, it is <= 15 bytes and does not match Accept-Encoding, this would cause a range index error as the while statement would not provide an effective guard.

I'm happy to contribute a test or two as well, if so desired.

This guard was backwards, not only breaking the functionality it was
intended to provide, but also causing panics under certain conditions.

In this case, the haystack is the pre-existing Vary header on the
response, and the needle is "Accept-Econding".

If we are compressing a response, and there is already a Vary header on
the response, it is <= 15 bytes and does not match Accept-Encoding, this
would cause a range index error as the while statement would not provide
an effective guard.
@sulami sulami force-pushed the fix-compression-panic branch from df86aed to 43d704f Compare June 3, 2025 01:01
Copy link
Collaborator

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

🙈

@seanmonstar seanmonstar merged commit 359d997 into tower-rs:main Jun 3, 2025
11 checks passed
@jplatte
Copy link
Member

jplatte commented Jun 3, 2025

@seanmonstar Could you yank the latest version and release a new one? (for some reason the crates.io web interface doesn't offer me the Yank button)

@GlenDC
Copy link
Contributor

GlenDC commented Jun 3, 2025

You might want to add some tests first? and perhaps have it as a policy to at least have some tests. Even better if you can reuse this from a utility module or w/e. As that would have prevented the original mistake to begin with.

@jplatte
Copy link
Member

jplatte commented Jul 9, 2025

I'm swamped with staying on top of things already, so I don't have any spare time to write tests. A PR with tests would be very welcome though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants