VCL failure undefined behavior

[dridi]

Consider the following test case:

varnishtest "backend fetch despite http_max_hdr overflow"

server s1 {
	rxreq
	expect req.http.hdr20 == set
	expect req.http.hdr32 == <undef>
	txresp
} -start

varnish v1 -cliok "param.set http_max_hdr 32"
varnish v1 -vcl+backend {
	sub vcl_backend_fetch {
		set bereq.http.hdr01 = "set";
		set bereq.http.hdr02 = "set";
		set bereq.http.hdr03 = "set";
		set bereq.http.hdr04 = "set";
		set bereq.http.hdr05 = "set";
		set bereq.http.hdr06 = "set";
		set bereq.http.hdr07 = "set";
		set bereq.http.hdr08 = "set";
		set bereq.http.hdr09 = "set";
		set bereq.http.hdr10 = "set";
		set bereq.http.hdr11 = "set";
		set bereq.http.hdr12 = "set";
		set bereq.http.hdr13 = "set";
		set bereq.http.hdr14 = "set";
		set bereq.http.hdr15 = "set";
		set bereq.http.hdr16 = "set";
		set bereq.http.hdr17 = "set";
		set bereq.http.hdr18 = "set";
		set bereq.http.hdr19 = "set";
		set bereq.http.hdr19 = "set";
		set bereq.http.hdr20 = "set";
		set bereq.http.hdr21 = "set";
		set bereq.http.hdr22 = "set";
		set bereq.http.hdr23 = "set";
		set bereq.http.hdr24 = "set";
		set bereq.http.hdr25 = "set";
		set bereq.http.hdr26 = "set";
		set bereq.http.hdr27 = "set";
		set bereq.http.hdr28 = "set";
		set bereq.http.hdr29 = "set";
		set bereq.http.hdr30 = "set";
		set bereq.http.hdr31 = "set";
		set bereq.http.hdr32 = "set";
	}
} -start

client c1 {
	txreq
	rxresp
	expect resp.status == 200
} -run

It passes, but arguably, it shouldn't. Even worse, the problem is caught and logged, but not acted upon and it points us in a wrong direction:

**** v1    vsl|       1002 BereqHeader     b hdr23: set
**** v1    vsl|       1002 LostHeader      b hdr24: set
**** v1    vsl|       1002 Error           b out of workspace (bo)
**** v1    vsl|       1002 LostHeader      b hdr25: set
**** v1    vsl|       1002 Error           b out of workspace (bo)
**** v1    vsl|       1002 LostHeader      b hdr26: set
**** v1    vsl|       1002 Error           b out of workspace (bo)
**** v1    vsl|       1002 LostHeader      b hdr27: set
**** v1    vsl|       1002 Error           b out of workspace (bo)
**** v1    vsl|       1002 LostHeader      b hdr28: set
**** v1    vsl|       1002 Error           b out of workspace (bo)
**** v1    vsl|       1002 LostHeader      b hdr29: set
**** v1    vsl|       1002 Error           b out of workspace (bo)
**** v1    vsl|       1002 LostHeader      b hdr30: set
**** v1    vsl|       1002 Error           b out of workspace (bo)
**** v1    vsl|       1002 LostHeader      b hdr31: set
**** v1    vsl|       1002 Error           b out of workspace (bo)
**** v1    vsl|       1002 LostHeader      b hdr32: set
**** v1    vsl|       1002 Error           b out of workspace (bo)
**** v1    vsl|       1002 VCL_return      b fetch

We started looking into this at v-s after a case where things were failing (I'll cover this aspect below) but bumping workspace didn't solve anything. Of course that would be expected when the limit we exceed is not workspace but max_http_hdr. It would only be fair to have an error message like Exceeded max_http_hdr parameter instead.

Something equivalent on the client side would fail, because before resp delivery we have a workspace overflow safety net that we seem to be lacking on the backend side.

Leaving the max_http_hdr overflow aside, even this test case passes today:

varnishtest "backend fetch despite workspace overflow"

server s1 {
        rxreq
        txresp
} -start

varnish v1 -vcl+backend {
        import vtc;

        sub vcl_backend_fetch {
                vtc.workspace_overflow(backend);
        }
} -start

client c1 {
        txreq
        rxresp
        expect resp.status == 200
} -run

Considering first that HTTP is an application protocol, and that headers act as variable placeholders in VCL, losing any header could result in a misbehaving application. Waiting until near the end of a task is already too late to fail the task. For VCL execution, we have the ability to VRT_fail() the task, and every single VCL statement is followed by a VRT failure check. We should do the same with header failures.

The problem with HTTP headers in the cache process is that they log errors directly on the VSL buffer and (ab)use the workspace to signal a failure. We can't (and probably shouldn't) trigger a VRT failure because we can't have a stable VRT_CTX for the duration of the task. Instead I suggest a subtle change:

--- bin/varnishd/cache/cache.h
+++ bin/varnishd/cache/cache.h
@@ -155,7 +155,7 @@ struct http {
        uint16_t                nhd;            /* Next free hd */
 
        enum VSL_tag_e          logtag;         /* Must be SLT_*Method */
-       struct vsl_log          *vsl;
+       struct worker           *wrk;
 
        struct ws               *ws;
        uint16_t                status;

With this, the VSL buffer would be one indirection away for actual logging, and for failures we could imagine having a WRK_Fail() function to log an error and fail the current task. That introduces new questions, for example a task may disembark a worker and reembark a new one.

With this, we would make HTTP headers failures converge towards a task failure, so they would be caught in VCL just like any other failure after every single VCL statement. Error conditions should generally converge towards failing the task, instead of requiring that all types of error conditions compound safety nets at some arbitrary point in tasks.

For errors happening in core code, I suggest we add a task failure check in the both req and fetch FSM loops.

[nigoroll]

Thoughts triggered by and taken from bugwash:

• The error message for "too many headers" clearly should be improved (it's an old topic, but for whatever reason we never acted upon it).
• Should vcl be able to explicitly check for overflows (promote vtc.workspace_overflowed() and optionally react with a rollback and some specific error handling?
• Which "bail out" point is best? end of vcl sub sounds sensible.

Consensus: Unhandled overflows in the fsm should result in the equivalent of return(fail)

[hermunn]

I resolving this before the freeze (mid Febuary?) on the table? I think it is time to change the behavior here and merge it in good time before the March release.

[hermunn]

I think deciding something here should be within reach for the next release. I am not asking for a code change, necessarily, but full consensus on what the correct behavior is, should be possible.

Consensus: Unhandled overflows in the fsm should result in the equivalent of return(fail)

Then the question remains: Should VCL fail immediately on overflow, or should the VCL writer be allowed to observe the overflow and handle it in their own way?

One idea is to introduce bereq.n_free_headers which VCL can inspect before adding new headers, and bail out if there are few headers slots left. Then a hard fail is "soft enough" for careful VCL writers, as it is possible to detect it beforehand.

As a reminder: Right now, if a backend sends a number of headers which exceeds the maximum configured, the backend fetch fails with a "semi reasonable" log:

**** v1    vsl|       1002 Timestamp       b Bereq: 1752507906.255599 0.000594 0.000124
**** v1    vsl|       1002 BogoHeader      b Too many headers: ab: 28
**** v1    vsl|       1002 HttpGarbage     b HTTP/1.1
**** v1    vsl|       1002 BerespStatus    b 503
**** v1    vsl|       1002 BerespReason    b Service Unavailable
**** v1    vsl|       1002 FetchError      b http format error
**** v1    vsl|       1002 BackendClose    b 22 s1 close RX_JUNK
**** v1    vsl|       1002 Timestamp       b Beresp: 1752507906.256562 0.001557 0.000962
**** v1    vsl|       1002 Timestamp       b Error: 1752507906.256569 0.001564 0.000006
**** v1    vsl|       1002 BerespProtocol  b HTTP/1.1
**** v1    vsl|       1002 BerespStatus    b 503
**** v1    vsl|       1002 BerespReason    b Backend fetch failed
**** v1    vsl|       1002 BerespHeader    b Date: Mon, 14 Jul 2025 15:45:06 GMT
**** v1    vsl|       1002 BerespHeader    b Server: Varnish
**** v1    vsl|       1002 VCL_call        b BACKEND_ERROR
**** v1    vsl|       1002 BerespHeader    b Content-Type: text/html; charset=utf-8
**** v1    vsl|       1002 BerespHeader    b Retry-After: 5
**** v1    vsl|       1002 VCL_return      b deliver
**** v1    vsl|       1002 Storage         b malloc Transient
**** v1    vsl|       1002 Length          b 281
**** v1    vsl|       1002 BereqAcct       b 148 0 148 315 0 315
**** v1    vsl|       1002 End             b 

I don't like the words junk and format error in there, since the response is only "bad" in that it contains a lot of headers.

If we change how Varnish fails when setting too many headers, I think it would be nice provide a log which is less confusing than what is shown above.

For reference, this is the test case (which fails on purpose):

varnishtest "backend fetch fails when there are too many headers"

server s1 {
	rxreq
	txresp -hdr "a: 1" -hdr "b: 2" -hdr "c: 3" -hdr "d: 4" -hdr "e: 5" -hdr "f: 6" -hdr "g: 7" -hdr "h: 8" -hdr "i: 9" -hdr "j: 10" -hdr "k: 11" -hdr "l: 12" -hdr "m: 13" -hdr "n: 14" -hdr "o: 15" -hdr "p: 16" -hdr "q: 17" -hdr "r: 18" -hdr "s: 19" -hdr "t: 20" -hdr "u: 21" -hdr "v: 22" -hdr "w: 23" -hdr "x: 24" -hdr "y: 25" -hdr "z: 26" -hdr "aa: 27" -hdr "ab: 28" -hdr "ac: 9" -hdr "ad: 30" -hdr "ae: 31" -hdr "af: 32" -hdr "ag: 33"
} -start

varnish v1 -arg "-p http_max_hdr=32" -vcl+backend {

} -start

client c1 {
	txreq
	rxresp
	expect resp.status == 200
} -run