In perl.git, the branch blead has been updated <http://perl5.git.perl.org/perl.git/commitdiff/f8def6c7f38b614db0e8ac0ba76999e9b8cfd1d6?hp=dc0dad9b91adb09c774c7248bc91a44b7a777d4d>
- Log ----------------------------------------------------------------- commit f8def6c7f38b614db0e8ac0ba76999e9b8cfd1d6 Author: David Mitchell <[email protected]> Date: Wed Feb 1 15:50:14 2017 +0000 avoid double-freeing regex code blocks RT #130650 heap-use-after-free in S_free_codeblocks When compiling qr/(?{...})/, a reg_code_blocks structure is allocated and various SVs are attached to it. Initially this is set to be freed via a destructor on the savestack, in case of early dying. Later the structure is attached to the compiling regex, and a boolean flag in the structure, 'attached', is set to true to show that the destructor no longer needs to free the struct. However, it is possible to get three orders of destruction: 1) allocate, push destructor, die early 2) allocate, push destructor, attach to regex, die 2) allocate, push destructor, attach to regex, succeed In 2, the regex is freed (via the savestack) before the destructor is called. In 3, the destructor is called, then later the regex is freed. It turns out perl can't currently handle case 2: qr'(?{})\6' Fix this by turning the 'attached' boolean field into an integer refcount, then keep a count of whether the struct is referenced from the savestack and/or the regex. Since it normally has a value of 1 or 2, it's similar to a boolean flag, but crucially it no longer just indicates that the regex has a pointer to it ('attached'), but that at least one of the savestack and regex have a pointer to it. So order of freeing no longer matters. I also updated S_free_codeblocks() so that it nulls out SV pointers in the reg_code_blocks struct before freeing them. This is is generally good practice to avoid double frees, although is probably not needed at the moment. ----------------------------------------------------------------------- Summary of changes: regcomp.c | 20 ++++++++++---------- regexp.h | 2 +- t/re/pat_re_eval.t | 11 ++++++++++- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/regcomp.c b/regcomp.c index 660be76858..850a6c1544 100644 --- a/regcomp.c +++ b/regcomp.c @@ -6133,10 +6133,13 @@ S_free_codeblocks(pTHX_ struct reg_code_blocks *cbs) { int n; - if (cbs->attached) + if (--cbs->refcnt > 0) return; - for (n = 0; n < cbs->count; n++) - SvREFCNT_dec(cbs->cb[n].src_regex); + for (n = 0; n < cbs->count; n++) { + REGEXP *rx = cbs->cb[n].src_regex; + cbs->cb[n].src_regex = NULL; + SvREFCNT_dec(rx); + } Safefree(cbs->cb); Safefree(cbs); } @@ -6148,7 +6151,7 @@ S_alloc_code_blocks(pTHX_ int ncode) struct reg_code_blocks *cbs; Newx(cbs, 1, struct reg_code_blocks); cbs->count = ncode; - cbs->attached = FALSE; + cbs->refcnt = 1; SAVEDESTRUCTOR_X(S_free_codeblocks, cbs); if (ncode) Newx(cbs->cb, ncode, struct reg_code_block); @@ -7168,8 +7171,7 @@ Perl_re_op_compile(pTHX_ SV ** const patternp, int pat_count, if (pm_flags & PMf_IS_QR) { ri->code_blocks = pRExC_state->code_blocks; if (ri->code_blocks) - /* disarm earlier SAVEDESTRUCTOR_X */ - ri->code_blocks->attached = TRUE; + ri->code_blocks->refcnt++; } { @@ -19533,10 +19535,8 @@ Perl_regfree_internal(pTHX_ REGEXP * const rx) if (ri->u.offsets) Safefree(ri->u.offsets); /* 20010421 MJD */ #endif - if (ri->code_blocks) { - ri->code_blocks->attached = FALSE; + if (ri->code_blocks) S_free_codeblocks(aTHX_ ri->code_blocks); - } if (ri->data) { int n = ri->data->count; @@ -19764,7 +19764,7 @@ Perl_regdupe_internal(pTHX_ REGEXP * const rx, CLONE_PARAMS *param) reti->code_blocks->cb[n].src_regex = (REGEXP*) sv_dup_inc((SV*)(ri->code_blocks->cb[n].src_regex), param); reti->code_blocks->count = ri->code_blocks->count; - reti->code_blocks->attached = TRUE; + reti->code_blocks->refcnt = 1; } else reti->code_blocks = NULL; diff --git a/regexp.h b/regexp.h index 601a214207..9a2b61a18e 100644 --- a/regexp.h +++ b/regexp.h @@ -88,7 +88,7 @@ struct reg_code_block { /* array of reg_code_block's plus header info */ struct reg_code_blocks { - bool attached; /* we're attached to a regex (don't need freeing) */ + int refcnt; /* we may be pointed to from a regex and from the savestack */ int count; /* how many code blocks */ struct reg_code_block *cb; /* array of reg_code_block's */ }; diff --git a/t/re/pat_re_eval.t b/t/re/pat_re_eval.t index a51d1d3e03..4df88aff57 100644 --- a/t/re/pat_re_eval.t +++ b/t/re/pat_re_eval.t @@ -22,7 +22,7 @@ BEGIN { } -plan tests => 528; # Update this when adding/deleting tests. +plan tests => 529; # Update this when adding/deleting tests. run_tests() unless caller; @@ -1244,6 +1244,15 @@ sub run_tests { pass "RT #129140"; } + # RT #130650 code blocks could get double-freed during a pattern + # compilation croak + + { + # this used to panic or give ASAN errors + eval 'qr/(?{})\6/'; + like $@, qr/Reference to nonexistent group/, "RT #130650"; + } + } # End of sub run_tests -- Perl5 Master Repository
