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

[Martin Blix Grydeland]

Forgot to add varnish-dev on this.

-Martin

---------- Forwarded message ----------
From: Martin Blix Grydeland <martin at varnish-software.com>
Date: Mon, Oct 25, 2010 at 14:21
Subject: Re: Request for code review of varnish-cache.org ticket #780 (Read
CRLF in fetch_chunked)
To: Poul-Henning Kamp <phk at phk.freebsd.dk>


On Mon, Oct 18, 2010 at 16:52, Poul-Henning Kamp <phk at phk.freebsd.dk> wrote:

> So I think the right logic is more like:
>
>        c = Rx one char
>        if (c == CR)
>                Rx one char
>        if (c != LF)
>                accept fetch
>                log error notice
>                close connection to prevent reuse
>
>
Second go at this. I've kept the changes to the test cases for them to be
RFC compliant, and implemented your pseudo-code. I leave the closing of the
connection to the calling code, but return a value of 1 to indicate the
connection is not safe to reuse.

Martin

Index: bin/varnishtest/tests/r00776.vtc
===================================================================
--- bin/varnishtest/tests/r00776.vtc    (revision 5463)
+++ 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: bin/varnishtest/tests/r00387.vtc
===================================================================
--- bin/varnishtest/tests/r00387.vtc    (revision 5463)
+++ 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: bin/varnishtest/tests/r00694.vtc
===================================================================
--- bin/varnishtest/tests/r00694.vtc    (revision 5463)
+++ 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: bin/varnishtest/tests/b00007.vtc
===================================================================
--- bin/varnishtest/tests/b00007.vtc    (revision 5463)
+++ 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: bin/varnishd/cache_fetch.c
===================================================================
--- bin/varnishd/cache_fetch.c    (revision 5463)
+++ bin/varnishd/cache_fetch.c    (working copy)
@@ -207,6 +207,16 @@

         q = bp = buf + v;
     }

+    /* Expect a CRLF to trail the chunks */
+    i = HTC_Read(htc, buf, 1);
+    if (i == 1 && buf[0] == '\r')
+        i = HTC_Read(htc, buf, 1);
+    if (i != 1 || (i==1 && buf[0] != '\n')) {
+        WSP(sp, SLT_FetchError,
+            "chunked missing trailing crlf");
+        return 1;    /* Accept fetch, but do not reuse connection */
+    }

+
     if (st != NULL && st->len == 0) {
         VTAILQ_REMOVE(&sp->obj->store, st, list);
         STV_free(st);


-- 
Martin Blix Grydeland
Varnish Software AS




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