Request for code review of varnish-cache.org ticket #780 (Read CRLF in fetch_chunked)

[Martin Blix Grydeland]

Hi,

Attached is code proposed to resolve ticket #780 on vanish-cache.org. This
ticket is about the final CRLF in a chunked fetch not being read by varnish.
I have added the code to read 2 more bytes after finishing with the chunks
and test that these are in fact CRLF. If they are not, it returns a fetch
failed (-1).

I was a bit puzzled that this behaviour could exist for so long in varnish
without it being noticed. The reason for that is probably that in most cases
this would still work as the code in http_splitline skips over leading CRLF
when looking at the next response on a reused backend. This would skip the
lingering CRLF in the byte stream at that moment.

Because of the reading of CRLF in http_splitline, I have been unable to
create a test case that would reliable fail without the fix. There was
however a number of test cases that produced non-compliant chunked encoding
data that would fail with the fix applied. (These tests were not adding the
mandatory final CRLF to the byte stream). I have updated these test cases in
the patch to add a final CRLF.

Discussion of trailing headers: With this patch, if a backend was to send us
trailing headers the connection will fail. Without the patch, I would expect
the next connection to fail instead, as the http response line processing
would not match. I think that is worse behaviour than failing on the
connection that has the traling headers. The patch could be extended to skip
over trailing headers to ignore them but keep the connection reusable and
don't fail the fetch, but I am not sure if it is worth the bother as noone
is actually using trailing headers.

Regards,
Martin Blix Grydeland

The patch:

Index: varnish-cache/bin/varnishtest/tests/r00776.vtc
===================================================================
--- varnish-cache/bin/varnishtest/tests/r00776.vtc    (revision 5430)
+++ varnish-cache/bin/varnishtest/tests/r00776.vtc    (working copy)
@@ -6,6 +6,7 @@
     rxreq
     txresp -nolen -hdr "Transfer-encoding: chunked"
     chunkedlen 4096
+    send "\r\n"
 } -start

 varnish v1 \
Index: varnish-cache/bin/varnishtest/tests/r00387.vtc
===================================================================
--- varnish-cache/bin/varnishtest/tests/r00387.vtc    (revision 5430)
+++ varnish-cache/bin/varnishtest/tests/r00387.vtc    (working copy)
@@ -10,6 +10,7 @@
     send "004\r\n1234\r\n"
     send "000000000000000000001\r\n@\r\n"
     send "00000000\r\n"
+    send "\r\n"
 } -start

 varnish v1 -vcl+backend {} -start
Index: varnish-cache/bin/varnishtest/tests/r00694.vtc
===================================================================
--- varnish-cache/bin/varnishtest/tests/r00694.vtc    (revision 5430)
+++ varnish-cache/bin/varnishtest/tests/r00694.vtc    (working copy)
@@ -9,6 +9,7 @@
     send "\r\n"
     # This is chunksize (128k) + 2 to force to chunks to be allocated
     chunkedlen 131074
+    send "\r\n"
 } -start

 varnish v1 -vcl+backend {
Index: varnish-cache/bin/varnishtest/tests/b00007.vtc
===================================================================
--- varnish-cache/bin/varnishtest/tests/b00007.vtc    (revision 5430)
+++ varnish-cache/bin/varnishtest/tests/b00007.vtc    (working copy)
@@ -10,6 +10,7 @@
     send "\r\n"
     send "00000004\r\n1234\r\n"
     send "00000000\r\n"
+    send "\r\n"

     rxreq
     expect req.url == "/foo"
@@ -19,6 +20,7 @@
     send "00000004\r\n1234\r\n"
     chunked "1234"
     chunked ""
+    send "\r\n"
 } -start

 varnish v1 -vcl+backend {} -start
Index: varnish-cache/bin/varnishd/cache_fetch.c
===================================================================
--- varnish-cache/bin/varnishd/cache_fetch.c    (revision 5430)
+++ varnish-cache/bin/varnishd/cache_fetch.c    (working copy)
@@ -207,6 +207,11 @@
         q = bp = buf + v;
     }

+    /* Expect a CRLF to trail the chunks */
+    i = HTC_Read(htc, buf, 2);
+    if (i != 2 || buf[0] != '\r' || buf[1] != '\n')
+        return -1;
+
     if (st != NULL && st->len == 0) {
         VTAILQ_REMOVE(&sp->obj->store, st, list);
         STV_free(st);


-- 
Martin Blix Grydeland
Varnish Software AS
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.varnish-cache.org/lists/pipermail/varnish-dev/attachments/20101015/fbc4cab6/attachment-0003.html>