New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

missing NULL handling for VCC-generated VRT_priv_(task|top) arguments #2708

Closed
nigoroll opened this Issue Jun 14, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@nigoroll
Contributor

nigoroll commented Jun 14, 2018

seen in vmod_var, but exposing a generic issue:

Panic at: Wed, 13 Jun 2018 15:31:45 GMT
Wrong turn at cache/cache_main.c:284:
Signal 11 (Segmentation fault) received at (nil) si_code 1
version = varnish-trunk revision ed5c43be0fdb120d19c1b703233d26b7593eb3b8, vrt api = 7.0
ident = Linux,3.13.0-126-generic,x86_64,-junix,-smalloc,-sdefault,-hcritbit,epoll
now = 1299978.179502 (mono), 1528903149.864093 (real)
Backtrace:
  0x439dfb: /usr/sbin/varnishd() [0x439dfb]
  0x497c72: /usr/sbin/varnishd(VAS_Fail+0x42) [0x497c72]
  0x435e9a: /usr/sbin/varnishd() [0x435e9a]
  0x7f53485a7330: /lib/x86_64-linux-gnu/libpthread.so.0(+0x10330) [0x7f53485a7330]
  0x7f53332b0099: ./vmod_cache/_vmod_var.XYAH@ZH@FGRKVOXFMTPHDGJQEACRFON@(+0x1099) [0x7f53332b0099]
  0x7f53332b05ee: ./vmod_cache/_vmod_var.XYAH@ZH@FGRKVOXFMTPHDGJQEACRFON@(vmod_set_backend+0x1e) [0x7f53332b05ee]
  0x7f53384deeb0: vcl_boot.1528793899.887292147/vgc.so(VGC_function_vcl_backend_fetch+0x3c0) [0x7f53384deeb0]
  0x44acfa: /usr/sbin/varnishd() [0x44acfa]
  0x44c60d: /usr/sbin/varnishd(VCL_backend_fetch_method+0x5d) [0x44c60d]
  0x4291ef: /usr/sbin/varnishd() [0x4291ef]
thread = (cache-worker)
thr.req = (nil) {
},
thr.busyobj = 0x7f4b4f50e020 {
  ws = 0x7f4b4f50e060 {
    OVERFLOWED id = \"Bo\",
    {s, f, r, e} = {0x7f4b4f5175c0, +59960, (nil), +59960},
  },
...

  vmods = {
    std = {Varnish trunk ed5c43be0fdb120d19c1b703233d26b7593eb3b8, 0.0},
    directors = {Varnish trunk ed5c43be0fdb120d19c1b703233d26b7593eb3b8, 0.0},
    bodyaccess = {Varnish trunk ed5c43be0fdb120d19c1b703233d26b7593eb3b8, 0.0},
    re = {Varnish trunk ed5c43be0fdb120d19c1b703233d26b7593eb3b8, 7.0},
    re2 = {Varnish trunk ed5c43be0fdb120d19c1b703233d26b7593eb3b8, 0.0},
    dispatch = {Varnish trunk ed5c43be0fdb120d19c1b703233d26b7593eb3b8, 0.0},
    dcs = {Varnish trunk ed5c43be0fdb120d19c1b703233d26b7593eb3b8, 0.0},
    var = {Varnish trunk ed5c43be0fdb120d19c1b703233d26b7593eb3b8, 7.0},
    vtc = {Varnish trunk ed5c43be0fdb120d19c1b703233d26b7593eb3b8, 0.0},
    header = {Varnish trunk ed5c43be0fdb120d19c1b703233d26b7593eb3b8, 7.0},
  },
},

coredump forensics:

#0  0x00007f5348204c37 in raise () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) bt
...
#4  0x0000000000435e9a in child_signal_handler (s=11, si=<optimized out>, c=<optimized out>)
    at cache/cache_main.c:282
#5  <signal handler called>
#6  get_vh (priv=<optimized out>, priv=<optimized out>) at vmod_var.c:135
#7  0x00007f53332b05ee in vmod_set_backend (ctx=ctx@entry=0x7f5222387fe0, priv=<optimized out>, 
    name=name@entry=0x7f53384f2923 "mybackendvar", value=value@entry=0x7f5347812920) at vmod_var.c:235
#8  0x00007f53384deeb0 in VGC_function_vcl_backend_fetch (ctx=0x7f5222387fe0) at vgc.c:39152
...

So the issue clearly is related to de2b431 :

While, previously, the PRIV came from the heap, now that it comes from the workspace, it is much more likely to be NULL.

in vcc_priv_arg https://github.com/varnishcache/varnish-cache/blob/master/lib/libvcc/vcc_expr.c#L379 we generate a VRT_priv_(task|top) call for PRIV_(TASK|TOP) arguments to vmod calls.

The obvious solution would be to add a null check to all vmods for PRIV_* arguments, but I think we should better solve this in core and never pass a NULL PRIV_* argument.

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Jun 14, 2018

@bsdphk bsdphk added the a=bugwash label Jun 18, 2018

@bsdphk

This comment has been minimized.

Contributor

bsdphk commented Jun 18, 2018

It's pretty involved to solve it in VCC, it may be easier to wrap all VMOD functions in a macro which automatically tests the args.

@nigoroll

This comment has been minimized.

Contributor

nigoroll commented Jun 18, 2018

discussed during bugwash:

  • we do not want to impose the priv check on vmod authors
  • the two alternatives on the table are a VCC vs. vmodtool solution
  • @bsdphk wants to ponder the idea of pre-allocating all the PRIV_(TASK|TOP) per task in VCC generated code

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Jun 18, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Jun 21, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Jun 26, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Jun 26, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Jun 26, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Jun 26, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Jul 12, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Jul 12, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Aug 6, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Aug 7, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Aug 14, 2018

initialize PRIV_TASK and PRIV_TOP vmod arguments once per subroutine
... and fail the VCL unless successful.

Providing the PRIVs to vmods is a core function, so error handling
should happen outside vmods.

Besides being safe, this initialization can be more efficient than
previous code for PRIVs used frequently within the same subroutine.

An alternative approach would be to initialize all privs once per
task / top request, but unless all privs are actually used in a VCL,
this approach could impose significant overhead, both in terms of time
and memory. By initializing privs once per sub, we impose overhead for
privs which are referenced but not actually used in a subroutine, but
not for all of the vcl.

Fixes varnishcache#2708
@nigoroll

This comment has been minimized.

Contributor

nigoroll commented Aug 14, 2018

implemented, see #2740

  • for the reasons given in #2740, I think pre-allocation is out: In a larger VCL, many PRIV calls will exist in subroutines which are never being called. While pre-allocation per subroutine may impose overhead already for privs which are referenced but never used, that overhead is contained to each subroutine.
  • I fail to see how the vmodtool macro idea would work as this basically would need to return from a function argument as in the toy example below
const char *
f(const char *a) {
	return a;
}


int main() {
	const char *a = NULL;

	if (f(a == NULL ? return 0 : a))
		return 1;
}

@nigoroll nigoroll added the SEP18REL label Aug 15, 2018

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Aug 16, 2018

initialize PRIV_TASK and PRIV_TOP vmod arguments once per subroutine
... and fail the VCL unless successful.

Providing the PRIVs to vmods is a core function, so error handling
should happen outside vmods.

Besides being safe, this initialization can be more efficient than
previous code for PRIVs used frequently within the same subroutine.

An alternative approach would be to initialize all privs once per
task / top request, but unless all privs are actually used in a VCL,
this approach could impose significant overhead, both in terms of time
and memory. By initializing privs once per sub, we impose overhead for
privs which are referenced but not actually used in a subroutine, but
not for all of the vcl.

Fixes varnishcache#2708

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Aug 16, 2018

initialize PRIV_TASK and PRIV_TOP vmod arguments once per subroutine
... and fail the VCL unless successful.

Providing the PRIVs to vmods is a core function, so error handling
should happen outside vmods.

Besides being safe, this initialization can be more efficient than
previous code for PRIVs used frequently within the same subroutine.

An alternative approach would be to initialize all privs once per
task / top request, but unless all privs are actually used in a VCL,
this approach could impose significant overhead, both in terms of time
and memory. By initializing privs once per sub, we impose overhead for
privs which are referenced but not actually used in a subroutine, but
not for all of the vcl.

Fixes varnishcache#2708

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Aug 17, 2018

initialize PRIV_TASK and PRIV_TOP vmod arguments once per subroutine
... and fail the VCL unless successful.

Providing the PRIVs to vmods is a core function, so error handling
should happen outside vmods.

Besides being safe, this initialization can be more efficient than
previous code for PRIVs used frequently within the same subroutine.

An alternative approach would be to initialize all privs once per
task / top request, but unless all privs are actually used in a VCL,
this approach could impose significant overhead, both in terms of time
and memory. By initializing privs once per sub, we impose overhead for
privs which are referenced but not actually used in a subroutine, but
not for all of the vcl.

Fixes varnishcache#2708

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Aug 17, 2018

initialize PRIV_TASK and PRIV_TOP vmod arguments once per subroutine
... and fail the VCL unless successful.

Providing the PRIVs to vmods is a core function, so error handling
should happen outside vmods.

Besides being safe, this initialization can be more efficient than
previous code for PRIVs used frequently within the same subroutine.

An alternative approach would be to initialize all privs once per
task / top request, but unless all privs are actually used in a VCL,
this approach could impose significant overhead, both in terms of time
and memory. By initializing privs once per sub, we impose overhead for
privs which are referenced but not actually used in a subroutine, but
not for all of the vcl.

Fixes varnishcache#2708

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Aug 22, 2018

initialize PRIV_TASK and PRIV_TOP vmod arguments once per subroutine
... and fail the VCL unless successful.

Providing the PRIVs to vmods is a core function, so error handling
should happen outside vmods.

Besides being safe, this initialization can be more efficient than
previous code for PRIVs used frequently within the same subroutine.

An alternative approach would be to initialize all privs once per
task / top request, but unless all privs are actually used in a VCL,
this approach could impose significant overhead, both in terms of time
and memory. By initializing privs once per sub, we impose overhead for
privs which are referenced but not actually used in a subroutine, but
not for all of the vcl.

Fixes varnishcache#2708

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Aug 28, 2018

initialize PRIV_TASK and PRIV_TOP vmod arguments once per subroutine
... and fail the VCL unless successful.

Providing the PRIVs to vmods is a core function, so error handling
should happen outside vmods.

Besides being safe, this initialization can be more efficient than
previous code for PRIVs used frequently within the same subroutine.

An alternative approach would be to initialize all privs once per
task / top request, but unless all privs are actually used in a VCL,
this approach could impose significant overhead, both in terms of time
and memory. By initializing privs once per sub, we impose overhead for
privs which are referenced but not actually used in a subroutine, but
not for all of the vcl.

Fixes varnishcache#2708

nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Aug 29, 2018

initialize PRIV_TASK and PRIV_TOP vmod arguments once per subroutine
... and fail the VCL unless successful.

Providing the PRIVs to vmods is a core function, so error handling
should happen outside vmods.

Besides being safe, this initialization can be more efficient than
previous code for PRIVs used frequently within the same subroutine.

An alternative approach would be to initialize all privs once per
task / top request, but unless all privs are actually used in a VCL,
this approach could impose significant overhead, both in terms of time
and memory. By initializing privs once per sub, we impose overhead for
privs which are referenced but not actually used in a subroutine, but
not for all of the vcl.

Fixes varnishcache#2708

Dridi added a commit that referenced this issue Oct 16, 2018

initialize PRIV_TASK and PRIV_TOP vmod arguments once per subroutine
... and fail the VCL unless successful.

Providing the PRIVs to vmods is a core function, so error handling
should happen outside vmods.

Besides being safe, this initialization can be more efficient than
previous code for PRIVs used frequently within the same subroutine.

An alternative approach would be to initialize all privs once per
task / top request, but unless all privs are actually used in a VCL,
this approach could impose significant overhead, both in terms of time
and memory. By initializing privs once per sub, we impose overhead for
privs which are referenced but not actually used in a subroutine, but
not for all of the vcl.

Fixes #2708
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment