solve the pcre stack overflow #1576 - was: stack vs workspace for pcre and others

[Nils Goroll]

Hi,

as we (UPLEX) have hit the pcre stack overflow (SIGSEGV) issue again in
production, I'd really appreciate if we could get a solution into master and I
volunteer to work on this, once we have consensus about which way to go.

Unfortunately this is bad timing for myself as I will be away for 16 days
starting Monday, but I want to take the opportunity to write up a summary at least.

I would like to continue after my holidays, but I really need the plan to be
backed by phk to ensure the investment in time pays off.

The most important point is that the current default thread_pool_stack (48k) and
pcre_match_limit_recursion (10.000) do not match at all: For 10k pcre
recursions, we'd need a stack size in the order of 2 MB. So at the very least,
we really must use sensible values like 64k stack and pcre_match_limit_recursion
200.

When built with pcre jit, we should default to 80k stack.

-> @phk OK to change as a quick temporary workaround?

But we can do better, here are the aspects I consider relevant based on the
discussion back in September (which happened on irc for the main part, primarily
phk, fgs, tollef and myself):

I will focus on pcre1. pcre2 has been released 2015-01-05 and we should consider
migrating later, but I think it's too early at this point in time.

memory usage of pcre is primarily determined by whether or not JIT is used:

* regular pcre_exec without JIT

  - stack required per pcre recursion on machine stack by default

    - alternatively, can use external alloc/free via callbacks, but
      this can only be enabled at pcre compile time, so it is not an option for
      varnish, unless we want to pull in pcre or change runtime behavior based
      on pcre_config(PCRE_CONFIG_STACKRECURSE). Also there is a relevant
      performance impact (according to the docs, have not checked myself)

  - pcre with PCRE_CONFIG_STACKRECURSE and without JIT has no built-in stack
    checks, it will run into a stack overflow unless we take precautions.

  - estimate of stack frame per recursion can be obtained using magic arguments
    pcre_exec(NULL, NULL, NULL, -999, -999) (documented in pcreapi(3),
    as output by ./pcretest -m -C, e.g. "Match recursion uses stack:
    approximate frame size = 176 bytes" on my install)

  - we can limit the recursion depth per pcre_exec - in recursions, not in
    bytes.

  Viable option I can see based on the work done back in September:

  * at init time:

    - get the pcre stack frame size

  * at init and parameter change time

    - check if the varnish parameters pcre_match_limit_recursion and
      thread_pool_stack make any sense at all and either refuse
      bad values or at least warn and auto-adjust

      suggestion: allow 48k stack for non-pcre and allow
      (approx pcre frame size + 16) * recursion_limit in addition

  * at pcre_exec time

    - check the available stack (see c522f7af7abaf8d847d62f8edaa46a2f5c0178e3
      stack_start/stack_end pointers added to struct worker by phk)

    - further reduce the recursion_limit passed to pcre_exec based on
      avail_stack / (approx pcre frame size + 16) [the 16 is an arbitrary
      safely margin)

    - separate log/stats for PCRE_ERROR_RECURSIONLIMIT

* JIT

  - uses

    - 32K on the machine stack

      - OR -

    - memory external to the machine stack (mmap(RWX, PRIVATE) with MAP_ANON or
      /dev/zero mapping) allocated with pcre_jit_stack_alloc(startsize, maxsize)

    (run time decision)

  - memory requirements have no strong tie to the notion of "pcre recursion",
    but do still depend on the pattern and the data (subject)

  When jit stack is insufficient in size, pcre_exec returns
  PCRE_ERROR_JIT_STACKLIMIT, so there is no built-in stack overflow nuke

  Viable options I can see:

  a) allow additional 32K on the machine stack and live with the fact that REs
     requiring more memory will fail

  b) manage a pool of pre-allocated jit stacks of arbitrary (user-specified)
     size

  c) re-try with memory external to the stack for PCRE_ERROR_JIT_STACKLIMIT
     errors

* DFA: we don't use atm


So I think the main decision point is what do to about JIT: Fist of all, for JIT
we should require 8.32 according to Zoltan:
http://www.mail-archive.com/pcre-dev@exim.org/msg03448.html

Then the pcre guys recommend enabling JIT by default. I'd like to follow this
recommendation, as long as pcre has a 'save' minimum version and there is a way
to change back to non-JIT at run time in case JIT causes trouble.

I'd prefer a per-RE flag in pcre, but we cannot expect this before pcre2, if at all.


So, overall, here's my suggestion on how to proceed:

- replace the compile time jit flag with a run-time (actually VCC time)
  parameter to enable pcre jit.
  On by default for "known good" pcre versions, off by default otherwise

- check/set sensible stack and recursion limit parameter defaults
  and make pcre_exec calls safe as discussed above

- add a way to switch jit on/off per RE at VCC time.

  The best way I can think of is to have options after the RE like

  vcl_recv {	
	if (req.http.foo ~ "^/bar") {
		...
	}
        # /J turns off jit
        # /j turns on
	if (req.http.foo ~ "jit-unsafe-RE"/J) {
		...
	}
  }

  Alternatively, we could only implement a switch for
  https://code.uplex.de/uplex-varnish/libvmod-re only and keep the builtin
  match operator with the (run time) default.

I'd appreciate any feedback, in particular

- your thoughts on the JIT a/b/c options above
- your thoughts on the overall plan
- better ideas for per-RE flags

Thanks, Nils