Skip to content

Commit

Permalink
Put some serious red tape on VCL_STRANDS
Browse files Browse the repository at this point in the history
I started suspecting that we need this clarification during the review
of #3123 [1] and was able to verify it with a simple test case. First I
needed a function I put in vmod_debug:

    $Function STRANDS hoard_strands(PRIV_TASK, STRANDS s)

    Return the first value of s for the rest of the task.

The implementation is very straightforward:

    struct hoarder {
           VCL_STRANDS     s;
    };

    VCL_STRANDS
    xyzzy_hoard_strands(VRT_CTX, struct vmod_priv *priv, VCL_STRANDS s)
    {
           struct hoarder *h;

           CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
           AN(priv);

           if (priv->priv == NULL) {
                   h = malloc(sizeof *h);
                   AN(h);
                   h->s = s;
                   priv->priv = h;
                   priv->free = free;
           }

           return (priv->priv);
    }

And then the following test case results in a panic on my system, but I
suspect this is generally undefined behavior and other nasty results may
occur under different circumstances:

    varnishtest "Beware of STRANDS"

    varnish v1 -vcl {
            import debug;
            backend be none;
            sub vcl_recv {
                    debug.hoard_strands("recv: " + req.url);
            }
            sub vcl_miss {
                    debug.hoard_strands("miss: " + req.url);
                    return (synth(200));
            }
            sub vcl_synth {
                    set resp.body = debug.hoard_strands("synth: " + req.url);
                    return (deliver);
            }
    } -start

    client c1 {
            txreq
            rxresp
            expect resp.body ~ recv
    } -run

This also begs the following question: can it ever be safe to let a VMOD
function return a STRANDS? Maybe it should be banned from return types.

[1] #3123 (comment)
  • Loading branch information
Dridi committed Nov 19, 2019
1 parent 34261af commit 11d5514
Showing 1 changed file with 16 additions and 0 deletions.
16 changes: 16 additions & 0 deletions include/vrt.h
Expand Up @@ -180,6 +180,22 @@ struct vsl_log;
struct ws;
struct VSC_main;

/*
* VCL_STRANDS:
*
* An argc+argv type of data structure where n indicates the number of strings
* in the p array. Individual components of a strands may be null.
*
* A STRANDS allows you to work on a strings concatenation with the option to
* collect it into a single STRING, or if possible work directly on individual
* parts.
*
* The memory management is very strict: a VMOD function receiving a STRANDS
* argument should keep no reference after the function returns. Retention of
* a STRANDS further in the ongoing task is undefined behavior and may result
* in a panic or data corruption.
*/

struct strands {
int n;
const char **p;
Expand Down

0 comments on commit 11d5514

Please sign in to comment.