Forgot to add varnish-dev on this.<br><br>-Martin<br><br><div class="gmail_quote">---------- Forwarded message ----------<br>From: <b class="gmail_sendername">Martin Blix Grydeland</b> <span dir="ltr"><<a href="mailto:martin@varnish-software.com">martin@varnish-software.com</a>></span><br>
Date: Mon, Oct 25, 2010 at 14:21<br>Subject: Re: Request for code review of <a href="http://varnish-cache.org">varnish-cache.org</a> ticket #780 (Read CRLF in fetch_chunked)<br>To: Poul-Henning Kamp <<a href="mailto:phk@phk.freebsd.dk">phk@phk.freebsd.dk</a>><br>
<br><br><div class="gmail_quote"><div class="im">On Mon, Oct 18, 2010 at 16:52, Poul-Henning Kamp <span dir="ltr"><<a href="mailto:phk@phk.freebsd.dk" target="_blank">phk@phk.freebsd.dk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">


So I think the right logic is more like:<br>
<br>
        c = Rx one char<br>
        if (c == CR)<br>
                Rx one char<br>
        if (c != LF)<br>
                accept fetch<br>
                log error notice<br>
                close connection to prevent reuse<br>
<br clear="all"></blockquote></div><div><br>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. <br>

<br>Martin<br><br>Index: bin/varnishtest/tests/r00776.vtc<br>===================================================================<br>--- bin/varnishtest/tests/r00776.vtc    (revision 5463)<br>+++ bin/varnishtest/tests/r00776.vtc    (working copy)<div class="im">
<br>
@@ -6,6 +6,7 @@<br>     rxreq<br>     txresp -nolen -hdr "Transfer-encoding: chunked"<br>     chunkedlen 4096<br>+    send "\r\n"<br> } -start<br> <br> varnish v1 \<br></div>Index: bin/varnishtest/tests/r00387.vtc<br>

===================================================================<br>--- bin/varnishtest/tests/r00387.vtc    (revision 5463)<br>+++ bin/varnishtest/tests/r00387.vtc    (working copy)<div class="im"><br>@@ -10,6 +10,7 @@<br>
     send "004\r\n1234\r\n"<br>
     send "000000000000000000001\r\n@\r\n"<br>     send "00000000\r\n"<br>+    send "\r\n"<br> } -start<br> <br> varnish v1 -vcl+backend {} -start<br></div>Index: bin/varnishtest/tests/r00694.vtc<br>

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

===================================================================<br>--- bin/varnishtest/tests/b00007.vtc    (revision 5463)<br>+++ bin/varnishtest/tests/b00007.vtc    (working copy)<div class="im"><br>@@ -10,6 +10,7 @@<br>
     send "\r\n"<br>
     send "00000004\r\n1234\r\n"<br>     send "00000000\r\n"<br>+    send "\r\n"<br> <br>     rxreq <br>     expect req.url == "/foo"<br>@@ -19,6 +20,7 @@<br>     send "00000004\r\n1234\r\n"<br>

     chunked "1234"<br>     chunked ""<br>+    send "\r\n"<br> } -start<br> <br> varnish v1 -vcl+backend {} -start<br></div>Index: bin/varnishd/cache_fetch.c<br>===================================================================<br>

--- bin/varnishd/cache_fetch.c    (revision 5463)<br>+++ bin/varnishd/cache_fetch.c    (working copy)<br>@@ -207,6 +207,16 @@<div class="im"><br>         q = bp = buf + v;<br>     }<br> <br>+    /* Expect a CRLF to trail the chunks */<br>
</div>
+    i = HTC_Read(htc, buf, 1);<br>+    if (i == 1 && buf[0] == '\r')<br>+        i = HTC_Read(htc, buf, 1);<br>+    if (i != 1 || (i==1 && buf[0] != '\n')) {<br>+        WSP(sp, SLT_FetchError,<br>

+            "chunked missing trailing crlf");<br>+        return 1;    /* Accept fetch, but do not reuse connection */<br>+    }<div class="im"><br>+<br>     if (st != NULL && st->len == 0) {<br>         VTAILQ_REMOVE(&sp->obj->store, st, list);<br>

         STV_free(st);<br> </div></div></div><div><div></div><div class="h5"><br>-- <br>Martin Blix Grydeland<br>Varnish Software AS<br><br>
</div></div></div><br><br clear="all"><br>-- <br>Martin Blix Grydeland<br>Varnish Software AS<br><br>