Skip to content

Commit

Permalink
avoid double-freeing regex code blocks
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
iabyn committed Feb 1, 2017
1 parent dc0dad9 commit f8def6c
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 12 deletions.
20 changes: 10 additions & 10 deletions regcomp.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
Expand Down Expand Up @@ -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++;
}

{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion regexp.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
};
Expand Down
11 changes: 10 additions & 1 deletion t/re/pat_re_eval.t
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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

Expand Down

0 comments on commit f8def6c

Please sign in to comment.