1. 07 Feb, 2021 1 commit
    • Nils Goroll's avatar
      Enable calling a dynamic sub · 5687511c
      Nils Goroll authored
      that is,
      
      	call vmod.returning_sub();
      
      in VCL.
      
      Background:
      
      The existing
      
      	call sub;
      
      is special in that it instantiates "sub", if it does not exist
      already, enabling use-before-definition semantics.
      
      The doctrine of this change is to preserve this behaviour and to not
      make existing, static calls any less efficient.
      
      Implementation:
      
      To support calling both literal SUBs as well as SUB expressions, we
      peek into the symbol table to check if the called expression is a
      function, based on the knowledge that, as of now, only functions can
      have a non-literal SUB return. We also know that no other expression
      can yield a SUB. So if peeking ahead tells us that a function follows,
      we call into expression evaluation.
      
      Otherwise, we expect a literal SUB as before.
      
      Null check:
      
      For dynamic SUB calls from vmods via VRT_call(), we require the SUB
      argument be non-NULL. But we can not for call: To signal error with a
      SUB return, a vmod function needs to be allowed to return NULL,
      otherwise it would need to have a dummy return value available for the
      VRT_fail() case. So we need to check for NULL in VGC.
      
      Alternatives considered:
      
      - We could make expression evaluation return SUB also for literal SUB
      symbols, turning all calls into sub->func(ctx, ...) C statements. I
      tried this path and it resulted in a change of behaviour with
      cc_command="clang -g -O2 ...": Where, previously, sub code was
      inlined, it would now generate an explicit C function call always,
      despite the fact that the C function is known at compile time (because
      all named VCL_SUB structs are).
      
      So this option was dismissed for violating the doctrine.
      
      - The null check could go to VPI, via a VPI_call(), basically identical
      to VRT_call(), but checking for NULL. This option was dismissed because
      it would foreclose the requirement to allow for SUB arguments in the
      future.
      5687511c
  2. 06 Feb, 2021 15 commits
    • Nils Goroll's avatar
      Add m00054.vtc: panic when calling sub from a wrong vcl · 1c5ba7da
      Nils Goroll authored
      VMODs must not use SUB references across VCLs. This test checks for a
      panic if they do.
      1c5ba7da
    • Nils Goroll's avatar
      Add m00053.vtc: Test dynamic calls · 3b5015ca
      Nils Goroll authored
      This test case checks dynamic calls, and, in particular, the runtime
      context checks and recursion detection. Recursions have to be caught at
      the first instance.
      
      Also test interop with the PRIV_TOP compile time restriction
      
      Ref #3498
      Ref 6d49b18f
      
      The test has been manually annotated with information regarding the
      results expted from VCCs call tree walk.
      
      Explanation:
      
      .called == call, but every walk counts (for instance, barbarbar can be
      reached twice, via bar and barbar)
      
      .nref == sum(refs): Each VCL ref gains a ref, and each call from a sub
      which is itself dynamic (has nref > called).
      3b5015ca
    • Nils Goroll's avatar
      Support dynamic SUBs with VRT_call() / VRT_check_call() · ec4e6617
      Nils Goroll authored
      This allows for VCL subs to be called dynamically at runtime: vmods
      may store references to VCL_SUBs and call them some time later.
      
      Calling a VCL_SUB pointer requires to conduct two fundamental checks
      also at runtime, which previously were implemented by VCC at compile
      time only:
      
      * checking the allowed context
      * checking for recursive calls
      
      The foundation for both has been laid in previous commits and has
      been made available through VPI_Call_*().
      
      Note that we do not change the VCC checks in any way for static calls,
      we only add runtime checks for dynamic calls.
      
      This commit adds a VRT interface for dynamic calls and changes VGC to
      ensure proper runtime checking (as suggested by phk).
      
      We add to the vcl_func_f signature
      - a vcl_func_call_e argument denoting the type of call and
      - a vcl_func_fail_e value argument to optionally return a failure code
      
      On vcl_func_call_e:
      
      VSUB_CHECK allows runtime callers to preflight a "can this sub be
      called" check, which is the basis for VRT_check_call().
      
      VSUB_DYNAMIC must be used by any calls outside VGC.
      
      VSUB_STATIC is for VGC only and, strictly speaking, would not be
      required. It is added for clarity and safety and used for "call"
      actions from VCL.
      
      On vcl_func_fail_e:
      
      If the argument is present, any error will be returned in
      it. Otherwise, the generated code fails the VCL.
      
      On the implementation:
      
      To minimize the overhead for runtime context and recursion checks, we
      only add them where needed.
      
      In previous commits, we changed the VCC walks over SUBs to ensure that
      any dynamically referenced SUBs and all SUBs reachable from these via
      "call" can be identified by having more references than calls
      (p->sym->nref > p->called).
      
      So we now know, at compile time, for which SUBs we need to add runtime
      checks. Read
      https://github.com/varnishcache/varnish-cache/pull/3163#issuecomment-770266098
      for more details.
      
      Depending on whether the SUB is dynamically callable or not, we either
      add runtime checks or keep the code identical to before (except for a
      changed signature and some assertions).
      
      For the dynamic case, we need to wrap the actual SUB in order to clear
      the recursion check bit when it returns. Note that this happens
      independend of whether the actuall call is static or dynamic - because
      it has to. The recursion check for dynamic calls requires any previous
      static calls to be registered.
      
      The VGC for the dynamic case looks like this (slightly edited for
      clarity):
      
      static void
      VGC_function_foo_checked(VRT_CTX)
      {
        assert(ctx->method & (VCL_MET_DELIVER|VCL_MET_SYNTH));
        { { VPI_count(ctx, 1);
            // ... as before
      
      void v_matchproto_(vcl_func_f)
      VGC_function_foo(VRT_CTX, enum vcl_func_call_e call,
          enum vcl_func_fail_e *failp)
      {
        enum vcl_func_fail_e fail;
      
        fail = VPI_Call_Check(ctx, &VCL_conf, 0x300, 0);
        if (failp)
          *failp = fail;
        else if (fail == VSUB_E_METHOD)
          VRT_fail(ctx, "call to \"sub foo{}\" not allowed from here");
        else if (fail == VSUB_E_RECURSE)
          VRT_fail(ctx, "Recursive call to \"sub foo{}\" not allowed from here");
        else
          assert(fail == VSUB_E_OK);
      
        if (fail != VSUB_E_OK || call == VSUB_CHECK)
          return;
        VPI_Call_Begin(ctx, 0);
        VGC_function_foo_checked(ctx);
        VPI_Call_End(ctx, 0);
      }
      
      The actual function body remains unchanged, but is contained in the
      static function named "*_checked". The vcl_func_f is now a wrapper.
      
      The wrapper
      
      - checks for context errors using VPI_Call_Check()
      - either returns any error in the failp argument or fails the VCL
      - encoses the call to the "_checked" function with VPI_Call_Begin /
        VPI_Call_End which set/clear the recursion marker bit.
      ec4e6617
    • Nils Goroll's avatar
      Add VPI interface for runtime recursion tracking & checks · d5c93b10
      Nils Goroll authored
      This implements the first bit of phks suggestion in
      https://github.com/varnishcache/varnish-cache/pull/3163#issuecomment-769718143
      
      These VPI functions implement the operations on the recursion check
      bitmask for dynamic SUB calls. Strictly speaking, the VPI subs would
      not need to be concerned with the methods bitmask check and the
      assertion that calls originate from the right VCL, but we centralise
      these aspects for simplicity.
      
      We also add an enum to denote check failures.
      d5c93b10
    • Nils Goroll's avatar
      Add a bitmask tracking SUB calls to detect recursions · fa3168ae
      Nils Goroll authored
      To allow dynamic SUB calls, we need to check for recursions at
      runtime, reflecting the compile time recursion check.
      
      We create a bitmask to note each dynamically called SUB by its unique
      number. To check for recursions, we only need to test if the
      respective bit for the sub number is already set.
      
      We only conduct this check if the track_call argument of
      vcl_call_method() is true. By setting this argument to the number of
      SUB references, we enable call tracking only for VCLs with dynamic SUB
      references (when (struct VCL_conf).nsub is non-zero).
      
      VCC already guarantees that static calls are loop free - so loops can
      only be introduced through dynamic SUB references.
      
      Call tracking with a bitmask was suggested by phk
      fa3168ae
    • Nils Goroll's avatar
      vgc: Advise C compiler not optimize housekeeping subs *only* · bb107a8e
      Nils Goroll authored
      Before the addition of the okmask, we could only selectively turn off
      optimizations for the built-in housekeeping subs (vcl_init {} /
      vcl_fini {}), not for any custom subs called from there.
      
      The previous commit changed this and turned off optimizations for any
      custom subs called from there.
      
      We pull this tighter again by only turning off optimizations for subs
      which can _only_ be called from housekeeping.
      
      Ref f1c47e48
      Ref #3230
      bb107a8e
    • Nils Goroll's avatar
      vgc: Assert on ctx->method also for custom subs · d4588ee5
      Nils Goroll authored
      Improve safety by asserting that a sub is never called from a context
      which is not allowed as per its okmask.
      
      This is to ensure correctness of our compile- and runtime-checks.
      d4588ee5
    • Nils Goroll's avatar
      vcc: Run compile time recursion check also on dynamic-only SUBs · 382ff038
      Nils Goroll authored
      We previously missed to run the static recursion check on SUBs which
      are only referenced symbolically (and thus might be called
      dynamically).
      
      This changes the error reporting tested in v00021.vtc because the
      recursion is detected on the recursing subs alone before the call tree
      from vcl_recv {} is walked.
      382ff038
    • Nils Goroll's avatar
      vcc: Mark subs statically reachable via dynamic subs as "dynamic" · a8262159
      Nils Goroll authored
      In follow up commits, we will need a marker for subs which can be
      reachable via dynamically referenced SUBs.
      
      We use "more references than calls" is this marker.  When walking
      the call trees, we add one reference for all subs which are called from
      a SUB having this marker, thus marking those, too.
      a8262159
    • Nils Goroll's avatar
      vcc: Repurpose (struct proc).called and add info to struct vcl_sub · 886aa58a
      Nils Goroll authored
      The .called attribute was not used.
      
      We now increment it only for custom subs which are actually called
      statically (with "call"), that is, not for the builtin subs. This is
      to simplify and clarify the use case in the next commits.
      
      The increment happens for each VCC walk of the SUBs.
      
      We also add the number of symbolic references and calls to (struct
      vcl_sub) of the VGC to facilitate reviews and debugging.
      886aa58a
    • Nils Goroll's avatar
      vcc: record if any SUB references have been encountered · e0bca94e
      Nils Goroll authored
      For follow up commits, we will need to know if a VCL contains any
      references to the SUB type, that is, if the SUB type was used as a vmod
      function or method argument.
      e0bca94e
    • Nils Goroll's avatar
      vcc SUB type: store C symbols for struct vcl_sub and the function · 965cd22f
      Nils Goroll authored
      We change the semantics of the rname and lname attributes of SUB
      symbols: rname now is the name of the (struct vcl_sub), lname is the
      cname, the name of the function to be called directly from VGC.
      965cd22f
    • Nils Goroll's avatar
      vcc: check for impossible subs at compile time · a42020cd
      Nils Goroll authored
      We call those subs impossible which use a combination of variables not
      being available together from any of the built-in subroutines (aka
      methods), for example::
      
              sub foo {
                      set req.http.impossible = beresp.reason;
              }
      
      For this example, there is no built-in subroutine for which both req
      and beresp were available, so it can not possibly be valid in any
      context.
      
      As long as subs were actually called directly or indirectly from a
      built-in sub, VCC did already detect the problem. For the above
      example, if `sub vcl_recv { call foo; }` existed, VCC would complain
      about beresp.reason not being accessible from vcl_recv.
      
      Except, when vcc_err_unref was disabled, the example would pass
      without error, and likewise it will with calls which we are about to
      add.
      
      We now add a check to detect impossible subroutines. It happens in a
      second walk over the VCL's SUB symbols in order to keep the more
      precise error messages for calls rooting in one of the built-in subs.
      
      This change also changes the order of error reporting slightly and
      incorporates, indirectly, the vcc_CheckUses() check moved in
      ed36b638.
      
      v00020.vtc now has the "Impossible Subroutine with vcc_err_unref off"
      check, while the newly added m00053.vtc has been changed to test
      failure of the impossible sub at compile time rather than at runtime.
      
      This change was triggered by feedback from Dridi
      a42020cd
    • Nils Goroll's avatar
      vcc: Expand SUB type, add possible calling methods · 4e01761c
      Nils Goroll authored
      Until now, the VCC SUB type represented the literal name of a
      vcc-generated C function, which was used to expand the VCL "call"
      action.
      
      We now add a struct to describe VCL subs and calculate a bitmask which
      represents the bilt-in subs which a SUB may be called from (the
      "okmask"),
      
      This is in preparation of the goal to add a VCL_SUB vmod type which
      will allow VMODs to call into vcl subs.
      
      We add to the vcl shared object a struct vcl_sub for each sub, which
      contains:
      
      	- a methods bitmask defining which built-in subs this sub may
      	  be called from (directly or indirectly)
      	- the name as seen from vcl
      	- a pointer back to the VCL_conf
      	- the function pointer to the vcl_func_f
      	- a unique number
      
      About the methods bitmask:
      
      It contains the VCL_MET_* bits of of built-in subs (traditionally
      called methods) which are allowed to call this sub.
      
      It is intended for checks like
      
      	if (sub->methods & ctx->method) {
      		sub->func(ctx);
      		return;
      	} else {
      		VRT_fail(ctx, "not allowed here");
      		return;
      	}
      
      In existing compile-time calls, we already check if used objects and
      returned actions are allowed within the respective calling context.
      
      To build the methods bitmask, we begin with a VCL_MET_TASK_ALL
      bitfield and clear the bits of methods which would not be allowed for
      any used object or returned action.
      
      About the VCL_conf pointer:
      
      Each VCL SUB belongs to a VCL, this pointer will allow to check that
      it is only ever used from the right VCL.
      
      About the unique number:
      
      By numbering vcl subs (per VCL), we can later implement a recursion
      check using a bitmask.
      
      In struct VCL_conf, we record the total number of subs.
      4e01761c
    • Nils Goroll's avatar
      vcc: link procs to their symbol · ee50b462
      Nils Goroll authored
      ee50b462
  3. 04 Feb, 2021 5 commits
  4. 03 Feb, 2021 1 commit
  5. 02 Feb, 2021 6 commits
    • Nils Goroll's avatar
      vcc: teach VCC_SymbolGet() how to be a good partner for life · 5a974c75
      Nils Goroll authored
      In any good relationship, one should be clear about expectations. And
      VCL authors want to know what VCC wants to hear, so tell them.
      
      ;)
      5a974c75
    • Nils Goroll's avatar
      Make v1.vtc vcls syntactically correct · 5f07720a
      Nils Goroll authored
      besides the incorrect bits we want to check for.
      
      Ref c4ec4712
      
      Exposed by #3163
      5f07720a
    • Nils Goroll's avatar
      inline vcc_assert.h in generate.py · 54410f97
      Nils Goroll authored
      as requested by phk.
      
      I found no way to avoid having to escape the backslashes because python,
      even for uninterpolated ''' long strings, sees them as line continuations.
      
      The generated output is identical except for two comments (in the code
      and the code output by the code).
      
      --- ./lib/libvcc/vcc_fixed_token.o.c	2021-02-02 10:40:03.725244373 +0100
      +++ ./lib/libvcc/vcc_fixed_token.c	2021-02-02 10:52:04.921259718 +0100
      @@ -834,10 +834,7 @@
       	    "uct vre **, const char *);\nvoid VPI_re_fini(struct vre *);\n"
       	);
       	VSB_cat(sb, "\n");
      -
      -	/* ../include/vcc_assert.h */
      -
      -	VSB_cat(sb, "/* ---===### include/vcc_assert.h ###===--- */\n\n");
      +	VSB_cat(sb, "/* ---===### vgc asserts (generate.py) ###===--- */\n\n");
       	VSB_cat(sb, "#define assert(e)\t\t\t\t\t\t\t\\\ndo {\t\t\t\t"
       	    "\t\t\t\t\t\\\n\tif (!(e)) {\t\t\t\t\t\t\t\\\n\t\tVPI_Fail(__func"
       	    "__, __FILE__, __LINE__, #e);\t\t\\\n\t}\t\t\t\t\t\t\t\t\\\n"
      54410f97
    • Nils Goroll's avatar
      Trivial refactor of generate.py · 65e68232
      Nils Goroll authored
      to split the output formattings from emit_file() from reading the file
      
      The produced output lib/libvcc/vcc_fixed_token.c is identical
      before/after.
      65e68232
    • Poul-Henning Kamp's avatar
      0d746632
    • Poul-Henning Kamp's avatar
      Whitespace ocd · 9570000b
      Poul-Henning Kamp authored
      9570000b
  6. 01 Feb, 2021 4 commits
    • Nils Goroll's avatar
      assert() for VGC and arm assertions on ctx->method · 88c59685
      Nils Goroll authored
      We wrap VAS_Fail() as VPI_Fail() and add an assert() macro to the
      Varnish Generated C (VGC) code. This assert() definition is basically
      a copy of that in vas.h, but deliberately added to a source file which
      is only used by generate.py for VGC, such that this slightly different
      definition be only visible to VGC.
      
      I also pondered the option to include VCL source information in
      VPI_Fail: By using the information updated by VPI_count(), we could
      have that, but as the assert is, for now, intended only to ensure
      correctness of VCC and core VRT code, I decided against this
      complication.
      
      With assert() in place for VGC, we arm the assertions from
      75acb5cc
      88c59685
    • Nils Goroll's avatar
      Teach coccinelle our favourite macros · edb0ca98
      Nils Goroll authored
      It seems they do not parse attributes and how I read
      --macro-file-builtins they just define them to NIL.
      edb0ca98
    • Nils Goroll's avatar
      Slim struct vrt_ctx by 16 bytes on 64bit · adfac945
      Nils Goroll authored
      How 16, you ask? Consider alignment...
      adfac945
    • Dridi Boukelmoune's avatar
      vcl: Make resp.proto read-only · c4ec4712
      Dridi Boukelmoune authored
      One of the VCL 4.1 changes was to make the protocol read-only, which
      we've successfully done for all variables except resp.proto that
      kept the exact same definition for both 4.0 and 4.1 versions, despite
      having distinct definitions.
      c4ec4712
  7. 31 Jan, 2021 2 commits
  8. 30 Jan, 2021 2 commits
    • Nils Goroll's avatar
      vcc: error check in vcc_walksymbols() for the recursive case · b3f22f4c
      Nils Goroll authored
      Bail out upon first error, as for the non-recursive case.
      b3f22f4c
    • Nils Goroll's avatar
      Add disarmed assertions on ctx->method to vgc · 75acb5cc
      Nils Goroll authored
      in the course of #3163 we want to add a preamble to VCL subs, which,
      ideally, would perform as many checks based on constants as possible.
      
      This is a first step towards such checks: We add a commented-out
      assert() on ctx->method for built-in subs only. To add the same for
      custom subs, we will need the okmask from #3163, for which I would first
      want to coordinate with phk.
      
      Also, I would want to coordinate with him on arming the assertion, as
      there might be a good reason why assert() is not yet defined for vgc (it
      is already used in unused macros, though).
      
      Having assertions would help humans, flexelint and compilers alike. Until
      armed, the assertion helps humans at least.
      
      The resulting vgc looks like this:
      
      void v_matchproto_(vcl_func_f)
      VGC_function_vcl_backend_error(VRT_CTX)
      {
        // assert(ctx->method == (VCL_MET_BACKEND_ERROR));
      
        /* ... from ('Builtin' Line 165 Pos 23) */
      75acb5cc
  9. 29 Jan, 2021 3 commits
  10. 28 Jan, 2021 1 commit