develooper Front page | perl.perl5.porters | Postings from June 2016

Re: [perl #128313] Memory leak in perl 5.24.0 when use re qw[strict]is used

Thread Previous
From:
demerphq
Date:
June 9, 2016 08:14
Subject:
Re: [perl #128313] Memory leak in perl 5.24.0 when use re qw[strict]is used
Message ID:
CANgJU+XH44efQVYNLYpoAS2L+k7KXtNuRM3Xhn1u4U_QsVFiWQ@mail.gmail.com
On 8 Jun 2016 20:38, "Dan Collins via RT" <perlbug-followup@perl.org> wrote:
>
> Yves' patch changes warn_text from leaking to mortal if posix_warnings is
set and the function ends early, no change in any other case. In other
words, it /never/ called SvREFCNT_dec_NN(warn_text), either before or now.
>
> Alternately, since posix_warnings isn't set if the function returns
early, we can SvREFCNT_dec(warn_text) if we return early. My limited
testing suggests that the memory advantage is minimal, but it's probably
"more right" to free it immediately before returning. Is the attached patch
what you were thinking? It still fixes this bug, and all tests still pass.
>
> (Patch is against blead /before/ Yves' patch was applied, if you'd prefer
a patch against blead, that can be arranged.

Wait, wait. :-)

Seems to me that if this is a real concern, and i trust karls judgement
there although i admit to being surprised, then this av should just become
part of the RExC struct and it should be reused each time.

Cheers
Yves

>
> ---
> via perlbug:  queue: perl5 status: open
> https://rt.perl.org/Ticket/Display.html?id=128313
>
> From b04aa5cec7c85fa03e65b7dd86394d818a3233eb Mon Sep 17 00:00:00 2001
> From: Dan Collins <dcollinsn@gmail.com>
> Date: Wed, 8 Jun 2016 14:26:05 -0400
> Subject: [PATCH] [perl #128313] Fix memory leak in POSIX class warnings
>
> Certain classes that "may be" POSIX classes result in POSIX warnings
> being added to warn_text, but never freed, resulting in a slow but
> present memory leak. We need to ensure that warn_text is freed.
>
> warn_text is presently mortalized late in the function, when it is
> assigned to *posix_warnings. However, certain cases can generate
> a POSIX warning while also having the function return before that
> point. If a POSIX warning is generated and the function returns
> before warn_text can be made mortal, it will never be freed.
>
> This patch performs a REFCNT_dec on warn_text immediately before
> any early return.
> ---
>  regcomp.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/regcomp.c b/regcomp.c
> index 0b1e606..6bd903d 100644
> --- a/regcomp.c
> +++ b/regcomp.c
> @@ -13772,6 +13772,10 @@ S_populate_ANYOF_from_invlist(pTHX_ regnode
*node, SV** invlist_ptr)
>                                               REPORT_LOCATION_ARGS(p)));
   \
>          }
   \
>      } STMT_END
> +#define RETURN_EARLY(retval) STMT_START {
   \
> +        if (warn_text) SvREFCNT_dec_NN(warn_text);
    \
> +        return (retval);
    \
> +    } STMT_END
>
>  STATIC int
>  S_handle_possible_posix(pTHX_ RExC_state_t *pRExC_state,
> @@ -13913,7 +13917,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
>      PERL_ARGS_ASSERT_HANDLE_POSSIBLE_POSIX;
>
>      if (p >= e) {
> -        return NOT_MEANT_TO_BE_A_POSIX_CLASS;
> +        RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
>      }
>
>      if (*(p - 1) != '[') {
> @@ -14002,7 +14006,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
>                      *updated_parse_ptr = (char *) temp_ptr;
>                  }
>
> -                return OOB_NAMEDCLASS;
> +                RETURN_EARLY(OOB_NAMEDCLASS);
>              }
>          }
>
> @@ -14072,7 +14076,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
>          /* We consider something like [^:^alnum:]] to not have been
intended to
>           * be a posix class, but XXX maybe we should */
>          if (complement) {
> -            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
> +            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
>          }
>
>          complement = 1;
> @@ -14099,7 +14103,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
>           * this leaves this construct looking like [:] or [:^], which
almost
>           * certainly weren't intended to be posix classes */
>          if (has_opening_bracket) {
> -            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
> +            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
>          }
>
>          /* But this function can be called when we parse the colon for
> @@ -14116,7 +14120,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
>              /* XXX We are currently very restrictive here, so this code
doesn't
>               * consider the possibility that, say, /[alpha.]]/ was
intended to
>               * be a posix class. */
> -            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
> +            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
>          }
>
>          /* Here we have something like 'foo:]'.  There was no initial
colon,
> @@ -14286,7 +14290,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
>              }
>
>              /* Otherwise, it can't have meant to have been a class */
> -            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
> +            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
>          }
>
>          /* If we ran off the end, and the final character was a
punctuation
> @@ -14336,7 +14340,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
>           * class name.  (We can do this on the first pass, as any second
pass
>           * will yield an even shorter name) */
>          if (name_len < 3) {
> -            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
> +            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
>          }
>
>          /* Find which class it is.  Initially switch on the length of
the name.
> @@ -14495,7 +14499,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
>              }
>
>              /* Here neither pass found a close-enough class name */
> -            return NOT_MEANT_TO_BE_A_POSIX_CLASS;
> +            RETURN_EARLY(NOT_MEANT_TO_BE_A_POSIX_CLASS);
>          }
>
>      probably_meant_to_be:
> @@ -14530,13 +14534,9 @@ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
>              }
>
>              if (warn_text) {
> -                if (posix_warnings) {
> -                    /* mortalize to avoid a leak with FATAL warnings */
> -                    *posix_warnings = (AV *) sv_2mortal((SV *)
warn_text);
> -                }
> -                else {
> -                    SvREFCNT_dec_NN(warn_text);
> -                }
> +                /* warn_text is only true if posix_warnings is true */
> +                assert(posix_warnings);
> +                *posix_warnings = (AV *) sv_2mortal((SV *) warn_text);
>              }
>          }
>          else if (class_number != OOB_NAMEDCLASS) {
> @@ -14562,6 +14562,7 @@ S_handle_possible_posix(pTHX_ RExC_state_t
*pRExC_state,
>      return OOB_NAMEDCLASS;
>  }
>  #undef ADD_POSIX_WARNING
> +#undef RETURN_EARLY
>
>  STATIC unsigned  int
>  S_regex_set_precedence(const U8 my_operator) {
> --
> 2.8.1
>
>

Thread Previous


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About