BUG/MEDIUM: compression: use a trash chunk as large as the buffer for the output

Since commit a0b37fb47 ("BUG/MEDIUM: compression: fix incompatibility
between compression and large-buffers"), slz_encode() writes directly into
the output chunk passed by the compression filter, which is the global
trash, sized like regular buffers. But with large buffers, a DATA block may
be larger than this trash. The input is then cut to the trash size by the
test added in commit f82ace414 ("MEDIUM: compression: Be sure to never
compress more than a chunk at once"), leaving no room for the deflater's
overhead. On incompressible data, the first call emits the input plus 5
bytes for the stored block plus the 10 bytes of the gzip header, which
overflows the trash:

  FATAL: bug condition "b->data + count > b->size" matched at include/haproxy/buf.h:395
  #0 b_add (b=..., count=32783) at include/haproxy/buf.h:395
  #1 comp_rfc195x_add_data (..., in_len=32768, out=...) at src/compression.c:404

This was seen in production on a PDF served with 32 kB buffers and
"http-response wait-for-body use-large-buffer", where the body formed a
37 kB DATA block. Without DEBUG_STRICT=2, the overflow goes unnoticed.

The output replaces the input in the HTX, so the right size for the output
chunk is the one of the buffer it comes from. It is now obtained with
get_best_trash_chunk(), which returns a large trash chunk for large buffers
and the regular one otherwise. htx_compression_buffer_init() still ensures
that the HTX has the room for the deflater's overhead, so a DATA block
plus this overhead always fits in such a chunk. A side effect is that the
data of a large buffer are now compressed in a single call, as intended by
the use of large buffers for compression.

The reg-test compression/large_buffers.vtc reproduces the crash with
unpatched versions built with DEBUG_STRICT=2.

This must be backported wherever a0b37fb47 is backported.

diff --git a/reg-tests/compression/large_buffers.vtc b/reg-tests/compression/large_buffers.vtc
new file mode 100644 (file)
index 0000000..8e24a5e
--- /dev/null
+++ b/reg-tests/compression/large_buffers.vtc
@@ -0,0 +1,64 @@
+varnishtest "Compression filter: incompressible contents in large buffers"
+
+# The compressed output replaces the input in the HTX buffer, and is built in
+# a trash chunk. With large buffers, a DATA block may be larger than the
+# regular trash, so the chunk must be as large as the buffer, otherwise the
+# deflater's output exceeds it on incompressible data (input + 5 bytes per
+# stored block + the gzip header). Here the response is waited for into a
+# large buffer, so that its body forms a single DATA block of up to 200 kB,
+# well beyond the 16 kB of the regular buffers. The client decompresses it,
+# which checks the gzip trailer's CRC and length, and compares the length to
+# the one announced by the service.
+feature cmd "$HAPROXY_PROGRAM -cc '(feature(ZLIB) || feature(SLZ)) && feature(LUA)'"
+feature ignore_unknown_macro
+
+haproxy h1 -conf {
+    global
+        tune.bufsize 16384
+        tune.bufsize.large 262144
+        tune.lua.bool-sample-conversion normal
+        lua-load ${testdir}/output_margin.lua
+
+    defaults
+        mode http
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    frontend fe-gzip
+        bind "fd@${fe_gzip}"
+        compression algo gzip
+        compression type text/plain
+        default_backend be
+
+    backend be
+        http-response wait-for-body time 5s use-large-buffer
+        server gen ${h1_gen_addr}:${h1_gen_port}
+
+    listen gen
+        bind "fd@${gen}"
+        http-request use-service lua.gen
+} -start
+
+client c1 -connect ${h1_fe_gzip_sock} {
+    txreq -url "/?p=random&c=whole&n=20000" -hdr "Accept-Encoding: gzip"
+    rxresp
+    expect resp.status == 200
+    expect resp.http.content-encoding == "gzip"
+    gunzip
+    expect resp.bodylen == 20000
+
+    txreq -url "/?p=random&c=whole&n=200000" -hdr "Accept-Encoding: gzip"
+    rxresp
+    expect resp.status == 200
+    expect resp.http.content-encoding == "gzip"
+    gunzip
+    expect resp.bodylen == 200000
+
+    txreq -url "/?p=mixed&c=whole&n=200000" -hdr "Accept-Encoding: gzip"
+    rxresp
+    expect resp.status == 200
+    expect resp.http.content-encoding == "gzip"
+    gunzip
+    expect resp.bodylen == 200000
+} -run

diff --git a/src/flt_http_comp.c b/src/flt_http_comp.c
index 71dffb859b2907d77bb87a3a0e7eb880f0e06196..9649dc5afd4cfef820b9fbc2e43d82d9d0e14ce3 100644 (file)
--- a/src/flt_http_comp.c
+++ b/src/flt_http_comp.c
@@ -296,6 +296,7 @@ comp_http_payload(struct stream *s, struct filter *filter, struct http_msg *msg,
        struct htx *htx = htxbuf(&msg->chn->buf);
        struct htx_ret htxret = htx_find_offset(htx, offset);
        struct htx_blk *blk, *next;
+       struct buffer *out;
        int ret, consumed = 0, to_forward = 0, last = 0, flush;
 
        blk = htxret.blk;
@@ -311,7 +312,14 @@ comp_http_payload(struct stream *s, struct filter *filter, struct http_msg *msg,
                if (!(st->flags & COMP_STATE_PROCESSING))
                        goto consume;
 
-               if (htx_compression_buffer_init(htx, &trash) < 0) {
+               /* The output replaces the input in the HTX, so the chunk must
+                * be as large as the buffer, which may be a large one.
+                */
+               out = get_best_trash_chunk(&msg->chn->buf, b_size(&msg->chn->buf));
+               if (!out)
+                       goto error;
+
+               if (htx_compression_buffer_init(htx, out) < 0) {
                        msg->chn->flags |= CF_WAKE_WRITE;
                        goto end;
                }
@@ -325,9 +333,9 @@ comp_http_payload(struct stream *s, struct filter *filter, struct http_msg *msg,
                                        last = 0;
                                        v.len = len;
                                }
-                               if (v.len > b_size(&trash)) {
+                               if (v.len > b_size(out)) {
                                        last = 0;
-                                       v.len = b_size(&trash);
+                                       v.len = b_size(out);
                                }
 
                                /* The output only needs to be flushed when no other
@@ -342,15 +350,15 @@ comp_http_payload(struct stream *s, struct filter *filter, struct http_msg *msg,
                                 */
                                flush = last || v.len == len || !next || htx_get_blk_type(next) != HTX_BLK_DATA;
 
-                               ret = htx_compression_buffer_add_data(st, v.ptr, v.len, &trash, dir);
-                               if (ret < 0 || (flush && htx_compression_buffer_end(st, &trash, last, dir) < 0))
+                               ret = htx_compression_buffer_add_data(st, v.ptr, v.len, out, dir);
+                               if (ret < 0 || (flush && htx_compression_buffer_end(st, out, last, dir) < 0))
                                        goto error;
                                BUG_ON(v.len != ret);
 
-                               if (ret == sz && !b_data(&trash))
+                               if (ret == sz && !b_data(out))
                                        next = htx_remove_blk(htx, blk);
                                else {
-                                       blk = htx_replace_blk_value(htx, blk, v, ist2(b_head(&trash), b_data(&trash)));
+                                       blk = htx_replace_blk_value(htx, blk, v, ist2(b_head(out), b_data(out)));
                                        if (!blk)
                                                goto error;
                                        next = htx_get_next_blk(htx, blk);
@@ -358,7 +366,7 @@ comp_http_payload(struct stream *s, struct filter *filter, struct http_msg *msg,
 
                                len -= ret;
                                consumed += ret;
-                               to_forward += b_data(&trash);
+                               to_forward += b_data(out);
                                if (last) {
                                        st->flags &= ~COMP_STATE_PROCESSING;
 
@@ -379,17 +387,17 @@ comp_http_payload(struct stream *s, struct filter *filter, struct http_msg *msg,
 
                        case HTX_BLK_TLR:
                        case HTX_BLK_EOT:
-                               if (htx_compression_buffer_end(st, &trash, 1, dir) < 0)
+                               if (htx_compression_buffer_end(st, out, 1, dir) < 0)
                                        goto error;
-                               if (b_data(&trash)) {
-                                       struct htx_blk *last = htx_add_last_data(htx, ist2(b_head(&trash), b_data(&trash)));
+                               if (b_data(out)) {
+                                       struct htx_blk *last = htx_add_last_data(htx, ist2(b_head(out), b_data(out)));
                                        if (!last)
                                                goto error;
                                        blk = htx_get_next_blk(htx, last);
                                        if (!blk)
                                                goto error;
                                        next = htx_get_next_blk(htx, blk);
-                                       to_forward += b_data(&trash);
+                                       to_forward += b_data(out);
                                }
                                st->flags &= ~COMP_STATE_PROCESSING;
                                __fallthrough;