Skip Menu |
Report information
Id: 129322
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: brian.carpenter [at] gmail.com
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: unknown
Perl Version: (no value)
Fixed In: (no value)



Subject: S_invlist_clear(SV *): Assertion `invlist' failed (regcomp.c:8527)
Download (untitled) / with headers
text/plain 290b
Triggered in Perl v5.25.5 (v5.25.4-130-g7aa7bbc). ./perl -e '/(?[[!]&[0]^[!]&[0]])/' The regex_sets feature is experimental in regex; marked by <-- HERE in m/(?[ <-- HERE [!]&[0]^[!]&[0]])/ at -e line 1. perl: regcomp.c:8527: void S_invlist_clear(SV *): Assertion `invlist' failed. Aborted
RT-Send-CC: perl5-porters [...] perl.org, public [...] khwilliamson.com
Download (untitled) / with headers
text/plain 283b
The call from regcomp.c:15315 (S_handle_regex_sets) has: SV* u = NULL; [...] _invlist_union(lhs, rhs, &u); .. but that calls Perl__invlist_union_maybe_complement_2nd() with &u as 'output', which is documented as "SHOULD BE DEFINED upon input". Karl, can you take a look? Hugo
Date: Thu, 6 Oct 2016 10:53:01 -0600
From: Karl Williamson <public [...] khwilliamson.com>
CC: perl5-porters [...] perl.org
To: perlbug-followup [...] perl.org
Subject: Re: [perl #129322] S_invlist_clear(SV *): Assertion `invlist' failed (regcomp.c:8527)
Download (untitled) / with headers
text/plain 580b
On 10/06/2016 09:09 AM, Hugo van der Sanden via RT wrote: Show quoted text
> The call from regcomp.c:15315 (S_handle_regex_sets) has: > SV* u = NULL; > [...] > _invlist_union(lhs, rhs, &u); > .. but that calls Perl__invlist_union_maybe_complement_2nd() with &u as 'output', which is documented as "SHOULD BE DEFINED upon input". > > Karl, can you take a look?
I plan to. What the docs mean is that u must be initialized, which it is, to NULL. What wording would be clearer? Show quoted text
> > Hugo > > --- > via perlbug: queue: perl5 status: new > https://rt.perl.org/Ticket/Display.html?id=129322 >
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.3k
On Thu Oct 06 09:53:44 2016, public@khwilliamson.com wrote: Show quoted text
> What the docs mean is that u must be initialized, which it > is, to NULL. What wording would be clearer?
I don't know - the docs go on to say: if it points to one of the two lists [...] otherwise just its contents will be modified .. which does not appear to allow for it being NULL. I'd probably write it as "... must point to an invlist or NULL ...", but I find the semantics strange and confusing - I would have expected a signature more like: SV* Perl__invlist_union_maybe_complement_2nd(pTHX_ SV* a, SV* b, bool complement_b) /* Constructs and returns an invlist that represents [xxx]; it may return a or b if * the result would be identical. */ .. and I assume there's a good reason why it isn't, but that doesn't jump out at me. In any case, that suggests the core dump is due to a missing guard rather than an error in the caller; I tried hacking in a simplistic fix: if (_invlist_len(a) == 0) { - invlist_clear(*output); + if (*output == NULL) + *output = _new_invlist(0); + else + invlist_clear(*output); return; } .. which avoids the coredump, but clearly there's more involved since it now gives a double-free on 'final' at the 'bad_syntax' label. Hugo
Date: Wed, 19 Oct 2016 22:04:12 -0600
CC: perl5-porters [...] perl.org
Subject: Re: [perl #129322] S_invlist_clear(SV *): Assertion `invlist' failed (regcomp.c:8527)
To: perlbug-followup [...] perl.org
From: Karl Williamson <public [...] khwilliamson.com>
Download (untitled) / with headers
text/plain 2.8k
On 10/07/2016 05:36 AM, Hugo van der Sanden via RT wrote: Show quoted text
> On Thu Oct 06 09:53:44 2016, public@khwilliamson.com wrote:
>> What the docs mean is that u must be initialized, which it >> is, to NULL. What wording would be clearer?
> > I don't know - the docs go on to say: > > if it points to one of the two lists [...] otherwise just its contents will be modified > > .. which does not appear to allow for it being NULL. > > I'd probably write it as "... must point to an invlist or NULL ...", but I find the semantics strange and confusing - I would have expected a signature more like: > > SV* Perl__invlist_union_maybe_complement_2nd(pTHX_ SV* a, SV* b, bool complement_b) > /* Constructs and returns an invlist that represents [xxx]; it may return a or b if > * the result would be identical. */ > > .. and I assume there's a good reason why it isn't, but that doesn't jump out at me.
The original reason was to have to write less code, by letting the function do it. In the majority of calls to these non-API functions, the result is supposed to overwrite one or the other of the inputs. By doing it this way, the function itself can take the most efficient action, and prevent leaks. Otherwise, the calls would have to be something like this pseudo code: temp = union(a, b); if (is_not_mortal(b)) decrement_ref_count(b); b = temp; It's easier to say union(a, b, &b); and now the function knows it is to overwrite b, and can take shortcuts. Later it turned out that a lot of mortalized variables were being generated before getting cleared, and memory needs were too high. Fixing this required changing only the functions and not all the calls to them. Show quoted text
> > In any case, that suggests the core dump is due to a missing guard rather than an error in the caller; I tried hacking in a simplistic fix: > > if (_invlist_len(a) == 0) { > - invlist_clear(*output); > + if (*output == NULL) > + *output = _new_invlist(0); > + else > + invlist_clear(*output); > return; > } > > .. which avoids the coredump, but clearly there's more involved since it now gives a double-free on 'final' at the 'bad_syntax' label.
This is now fixed by a5540cf9741163e5c13e99582ebe3a6ba4f3d3fa. The problem was actually that my changes to the union and intersection to handle mortalized inputs were not consistent, and so the caller in rare instances got something they weren't expecting. This was a violation of encapsulation, and I'm somewhat disturbed I wrote code like that. Now, things are more generalized, and an assertion will fail right off the bat if the inputs to union and intersection aren't correct. And the encapsulation is preserved. Show quoted text
> > Hugo > > --- > via perlbug: queue: perl5 status: open > https://rt.perl.org/Ticket/Display.html?id=129322 >
Download (untitled) / with headers
text/plain 313b
Thank you for filing this report. You have helped make Perl better. With the release today of Perl 5.26.0, this and 210 other issues have been resolved. Perl 5.26.0 may be downloaded via: https://metacpan.org/release/XSAWYERX/perl-5.26.0 If you find that the problem persists, feel free to reopen this ticket.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org