This patch fixes GitHub Issue #988. Commit
ce9e7b25217c46db1ac636b2c885a05bf91ae57e
was not sufficient, because it fell back to a hash comparison if the bitmap
of known encodings was not acceptable instead of directly returning the the
cached response is not compatible.
This patch also extends the reg-test to test the hash collision that was
mentioned in #988.
Vary handling is 2.4, no backport needed.
---
reg-tests/cache/vary_accept_encoding.vtc | 32 ++++++++++++++++++++++++
src/cache.c | 29 +++++++++++++--------
2 files changed, 51 insertions(+), 10 deletions(-)
diff --git a/reg-tests/cache/vary_accept_encoding.vtc
b/reg-tests/cache/vary_accept_encoding.vtc
index afd2cfbd5..59e819a75 100644
--- a/reg-tests/cache/vary_accept_encoding.vtc
+++ b/reg-tests/cache/vary_accept_encoding.vtc
@@ -73,6 +73,21 @@ server s1 {
-hdr "Cache-Control: max-age=5" \
-hdr "Content-Encoding: unknown_encoding" \
-bodylen 119
+
+
+ rxreq
+ expect req.url == "/hash-collision"
+ txresp -hdr "Vary: accept-encoding" \
+ -hdr "Cache-Control: max-age=5" \
+ -hdr "Content-Encoding: br" \
+ -bodylen 129
+
+ rxreq
+ expect req.url == "/hash-collision"
+ txresp -hdr "Vary: accept-encoding" \
+ -hdr "Cache-Control: max-age=5" \
+ -hdr "Content-Encoding: gzip" \
+ -bodylen 139
} -start
@@ -300,4 +315,21 @@ client c1 -connect ${h1_fe_sock} {
expect resp.bodylen == 119
expect resp.http.X-Cache-Hit == 0
+ #
+ # Hash collision (https://github.com/haproxy/haproxy/issues/988)
+ #
+ # crc32(gzip) ^ crc32(br) ^ crc32(xxx) ^ crc32(jdcqiab) == crc32(gzip)
+ txreq -url "/hash-collision" -hdr "Accept-Encoding: br,gzip,xxx,jdcqiab"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.content-encoding == "br"
+ expect resp.bodylen == 129
+ expect resp.http.X-Cache-Hit == 0
+
+ txreq -url "/hash-collision" -hdr "Accept-Encoding: gzip"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.content-encoding == "gzip"
+ expect resp.bodylen == 139
+ expect resp.http.X-Cache-Hit == 0
} -run
diff --git a/src/cache.c b/src/cache.c
index c740818e9..23883df96 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -2398,19 +2398,28 @@ static int accept_encoding_hash_cmp(const void
*ref_hash, const void *new_hash,
/* All the bits set in the reference bitmap correspond to the
* stored response' encoding and should all be set in the new
* encoding bitmap in order for the client to be able to manage
- * the response. */
- if ((ref.encoding_bitmap & new.encoding_bitmap) ==
ref.encoding_bitmap) {
- /* The cached response has encodings that are accepted
- * by the client, it can be served directly by the cache
- * (as far as the accept-encoding part is concerned). */
- return 0;
- }
+ * the response.
+ *
+ * If this is the case the cached response has encodings that
+ * are accepted by the client. It can be served directly by
+ * the cache (as far as the accept-encoding part is concerned).
+ */
+
+ return (ref.encoding_bitmap & new.encoding_bitmap) !=
ref.encoding_bitmap;
}
+ else {
+ /* We must compare hashes only when the the response contains
+ * unknown encodings.
+ * Otherwise we might serve unacceptable responses if the hash
+ * of a client's `accept-encoding` header collides with a
+ * known encoding.
+ */
- ref.hash = read_u32(ref_hash+sizeof(ref.encoding_bitmap));
- new.hash = read_u32(new_hash+sizeof(new.encoding_bitmap));
+ ref.hash = read_u32(ref_hash+sizeof(ref.encoding_bitmap));
+ new.hash = read_u32(new_hash+sizeof(new.encoding_bitmap));
- return ref.hash != new.hash;
+ return ref.hash != new.hash;
+ }
}
--
2.29.0
