Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

S_invlist_clear(SV *): Assertion `invlist' failed (regcomp.c:8527) #15620

Closed
p5pRT opened this issue Sep 20, 2016 · 9 comments

Comments

@p5pRT
Copy link

commented Sep 20, 2016

Migrated from rt.perl.org#129322 (status was 'resolved')

Searchable as RT129322$

@p5pRT

This comment has been minimized.

Copy link
Author

commented Sep 20, 2016

From @geeknik

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

@p5pRT

This comment has been minimized.

Copy link
Author

commented Oct 6, 2016

From @hvds

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

@p5pRT

This comment has been minimized.

Copy link
Author

commented Oct 6, 2016

The RT System itself - Status changed from 'new' to 'open'

@p5pRT

This comment has been minimized.

Copy link
Author

commented Oct 6, 2016

From @khwilliamson

On 10/06/2016 09​:09 AM, Hugo van der Sanden via RT wrote​:

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?

Hugo

---
via perlbug​: queue​: perl5 status​: new
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=129322

@p5pRT

This comment has been minimized.

Copy link
Author

commented Oct 7, 2016

From @hvds

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.

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

@p5pRT

This comment has been minimized.

Copy link
Author

commented Oct 20, 2016

From @khwilliamson

On 10/07/2016 05​:36 AM, Hugo van der Sanden via RT wrote​:

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.

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 a5540cf. 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.

Hugo

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=129322

@p5pRT

This comment has been minimized.

Copy link
Author

commented Oct 26, 2016

@khwilliamson - Status changed from 'open' to 'pending release'

@p5pRT

This comment has been minimized.

Copy link
Author

commented May 30, 2017

From @khwilliamson

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.

@p5pRT

This comment has been minimized.

Copy link
Author

commented May 30, 2017

@khwilliamson - Status changed from 'pending release' to 'resolved'

@p5pRT p5pRT closed this May 30, 2017
@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.