(Mac)Debug ALL assertion fails when instancing embedded code before a named capture followed by named back reference #19350
Comments
|
I closed since the example worked on online perl but maybe this is a macos exclusive issue. |
|
On Wed, 19 Jan 2022 at 01:38, 6a4h8 ***@***.***> wrote:
Module: re
*Description*
Assertion failed when I use qw(Debug ALL) in my program.
*Steps to Reproduce*
use re qw(Debug ALL);
qr{(?{callfunc})(?<name>\g{absqualifsbasic}}
*Expected behavior*
Not assert fail.
*Perl configuration*
Summary of my perl5 (revision 5 version 30 subversion 3) configuration:
No assert fail in 5.28.1:
$ perl -Mre=Debug,ALL -e'qr{(?{callfunc})(?<name>\g{absqualifsbasic}}'
Assembling pattern from 2 elements
Compiling REx "(?{callfunc})(?<name>\g{absqualifsbasic}"
Starting first pass (sizing)
<(?{callfunc>...| 1| reg
| | brnc
| | piec
| | atom
<?{callfunc}>...| | reg
<(?<name>\g{>...| 4| piec
| | atom
<?<name>\g{a>...| | reg
<\g{absquali>...| 6| brnc
| | piec
| | atom
Unmatched ( in regex; marked by <-- HERE in m/(?{callfunc})( <-- HERE
?<name>\g{absqualifsbasic}/ at -e line 1.
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
Did you try on a mac? what is your perl version? |
AnFunctionArray
changed the title
|
On Wed, 19 Jan 2022 at 06:15, 6a4h8 ***@***.***> wrote:
On Wed, 19 Jan 2022 at 01:38, 6a4h8 *@*.***> wrote: Module: re
*Description* Assertion failed when I use qw(Debug ALL) in my program. *Steps
to Reproduce* use re qw(Debug ALL);
qr{(?{callfunc})(?\g{absqualifsbasic}} *Expected behavior* Not assert
fail. *Perl configuration* Summary of my perl5 (revision 5 version 30
subversion 3) configuration: No assert fail in 5.28.1:
$ perl -Mre=Debug,ALL -e'qr{(?{callfunc})(?\g{absqualifsbasic}}'
Assembling pattern from 2 elements Compiling REx
"(?{callfunc})(?\g{absqualifsbasic}" Starting first pass (sizing)
<(?{callfunc>...| 1| reg | | brnc | | piec | | atom <?{callfunc}>...| | reg
<(?\g{>...| 4| piec | | atom <?\g{a>...| | reg <\g{absquali>...| 6| brnc |
| piec | | atom Unmatched ( in regex; marked by <-- HERE in
m/(?{callfunc})( <-- HERE ?\g{absqualifsbasic}/ at -e line 1.
… <#m_-5255632312895881260_>
-- perl -Mre=debug -e "/just|another|perl|hacker/"
Did you try on a mac?
Nope. I have a linux box.
what is your perl version?
Stated above 5.28.1
yves.
|
Well it seems to be an issue only with Mac - (tested on mac mini m1 with both bult-in version and blead and also on a vm inside the mac). |
|
Fails with the latest blead perl on Linux: |
|
On Sat, 22 Jan 2022 at 21:27, E. Choroba ***@***.***> wrote:
Fails with the latest blead perl on Linux:
$blead -e'use re Debug=>"ALL";qr{(?{a})(?<b>\g{c}})'
Assembling pattern from 2 elements
Compiling REx "(?{a})(?<b>\g{c}"
Starting parse and generation
<(?{a})(?<b>>...| 1| reg
| | brnc
| | piec
| | atom
<?{a})(?<b>\>...| | reg
<(?<b>\g{c}> | 4| piec
| | atom
<?<b>\g{c}> | | reg
| | Setting open paren #1 to 4
<\g{c}> | 6| brnc
| | piec
| | atom
<> | 8| tail~ OPEN1 'b' (4) -> REFN
| | Setting close paren #1 to 8
| 10| lsbrperl5.35.9: re_comp.c:21443: my_regprop: Assertion `PL_valid_types_PVX[SvTYPE(_svpvx) & SVt_MASK]' failed.
/home/choroba/bin/blead: line 26: 18460 Aborted (core dumped) "${exec[0]}" "$@"
$ blead -v
This is perl 5, version 35, subversion 9 (v5.35.9 (v5.35.8-7-g8030c9e253)) built for x86_64-linux-thread-multi
(with 1 registered patch, see perl -V for more detail)
Thanks. This is broken by:
commit 2a98b8c
Author: Richard Leach ***@***.***>
Date: Tue Nov 23 23:35:58 2021 +0000
newSVpvn_flags().. is more efficient than sv_2mortal(newSVpvn(..))
The same holds for newSVpvs* wrappers around newSVpvn* functions.
It is from the op.c change. I think something about this patch is tickling
a compiler bug as the code appears to be correct but it randomly produces
different results.
I would push a fix patch, but my config is out of date, ill try to do it
later.
Yves
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
demerphq
added a commit
that referenced
this issue
Jan 23, 2022
This reverts the op.c change from "newSVpvn_flags().. is more efficient than sv_2mortal(newSVpvn(..))" full commit id 2a98b8c For some reason the op.c change tickles a compiler bug. Various minor changes to the text result in this working as expected, and then other changes make it fail, etc. I was not able to work out why, the code as presented is not wrong, although it hides some stuff underneath. The expression is: newSVpvn_flags( PadnamePV(name)+1,PadnameLEN(name)-1, (PadnameUTF8(name)) ? SVf_UTF8|SVs_TEMP : SVs_TEMP ); Which should be the same as: sv_2mortal(newSVpvn_utf8( PadnamePV(name)+1,PadnameLEN(name)-1, PadnameUTF8(name) )); However for some reason the compiler does something different. An interesting subtlety is that PadnameUTF8() always returns 1. So you would think that it could be replaced by newSVpvn_flags( PadnamePV(name)+1,PadnameLEN(name)-1, SVf_UTF8|SVs_TEMP); but when I compile that it seems to randomly produce the right result or not. I tried various formulations of parenthesizing the arguments, reducing the ternary to its natural result (above) etc, and the compiler seemed to produce random results. The only formulation that reliably produced the correct results is the original one.
demerphq
added a commit
that referenced
this issue
Jan 23, 2022
This reverts the op.c change from "newSVpvn_flags().. is more efficient than sv_2mortal(newSVpvn(..))" full commit id 2a98b8c For some reason the op.c change tickles a compiler bug. Various minor changes to the text result in this working as expected, and then other changes make it fail, etc. I was not able to work out why, the code as presented is not wrong, although it hides some stuff underneath. The expression is: newSVpvn_flags( PadnamePV(name)+1,PadnameLEN(name)-1, (PadnameUTF8(name)) ? SVf_UTF8|SVs_TEMP : SVs_TEMP ); Which should be the same as: sv_2mortal(newSVpvn_utf8( PadnamePV(name)+1,PadnameLEN(name)-1, PadnameUTF8(name) )); However for some reason the compiler does something different. An interesting subtlety is that PadnameUTF8() always returns 1. So you would think that it could be replaced by newSVpvn_flags( PadnamePV(name)+1,PadnameLEN(name)-1, SVf_UTF8|SVs_TEMP); but when I compile that it seems to randomly produce the right result or not. I tried various formulations of parenthesizing the arguments, reducing the ternary to its natural result (above) etc, and the compiler seemed to produce random results. The only formulation that reliably produced the correct results is the original one.
|
On Sun, 23 Jan 2022 at 08:42, demerphq ***@***.***> wrote:
On Sat, 22 Jan 2022 at 21:27, E. Choroba ***@***.***> wrote:
>
> <> | 8| tail~ OPEN1 'b' (4) -> REFN
> | | Setting close paren #1 to 8
> | 10| lsbrperl5.35.9: re_comp.c:21443: my_regprop: Assertion `PL_valid_types_PVX[SvTYPE(_svpvx) & SVt_MASK]' failed.
> /home/choroba/bin/blead: line 26: 18460 Aborted (core dumped) "${exec[0]}" "$@"
>
> $ blead -v
>
> This is perl 5, version 35, subversion 9 (v5.35.9 (v5.35.8-7-g8030c9e253)) built for x86_64-linux-thread-multi
> (with 1 registered patch, see perl -V for more detail)
>
>
>
Thanks. This is broken by:
Actually I recant. I will dig more. There is something heisenbug about
this. If I run the test repeatedly it fails or pases randomly, which is
making it hard to find the patch that caused this.
cheers
Yves
|
|
On Sun, 23 Jan 2022 at 10:18, demerphq ***@***.***> wrote:
Actually I recant. I will dig more. There is something heisenbug about
this. If I run the test repeatedly it fails or pases randomly, which is
making it hard to find the patch that caused this.
Ok, once I realized I had to run the 10 times or more to reliably see if
the assert fail was triggered I was able to bisect to the below patch.
The test I used was:
(for i in {1..10}; do ./perl -Ilib -e'use re
Debug=>"ALL";qr{(?{a})(?<b>\g{c}})'; done;) 2>&1 | grep Assertion
7c932d0 is the first bad commit
commit 7c932d0
Author: Karl Williamson ***@***.***>
Date: Fri Oct 19 09:48:34 2018 -0600
Remove sizing pass from regular expression compiler
This commit removes the sizing pass for regular expression compilation.
It attempts to be the minimum required to do this. Future patches are
in the works that improve it,, and there is certainly lots more that
could be done.
This is being done now for security reasons, as there have been several
bugs leading to CVEs where the sizing pass computed the size improperly,
and a crafted pattern could allow an attack. This means that simple
bugs can too easily become attack vectors.
This is NOT the AST that people would like, but it should make it easier
to move the code in that direction.
Instead of a sizing pass, as the pattern is parsed, new space is
malloc'd for each regnode found. To minimize the number of such mallocs
that actually go out and request memory, an initial guess is made, based
on the length of the pattern being compiled. The guessed amount is
malloc'd and then immediately released. Hopefully that memory won't be
gobbled up by another process before we actually gain control of it.
The guess is currently simply the number of bytes in the pattern.
Patches and/or suggestions are welcome on improving the guess or this
method.
This commit does not mean, however, that only one pass is done in all
cases. Currently there are several situations that require extra
passes. These are:
a) If the pattern isn't UTF-8, but encounters a construct that
requires it to become UTF-8, the parse is immediately stopped,
the translation is done, and the parse restarted. This is
unchanged from how it worked prior to this commit.
b) If the pattern uses /d rules and it encounters a construct that
requires it to become /u, the parse is immediately stopped and
restarted using /u rules. A future enhancement is to only
restart if something has been encountered that would generate
something different than what has already been generated, as
many operations are the same under both /d and /u. Prior to
this commit, in rare circumstances was the parse immediately
restarted. Only those few that changed the sizing did so.
Instead the sizing pass was allowed to complete and then the
generation pass ran, using /u. Some CVEs were caused by faulty
implementation here.
c) Very large patterns may need to have long jumps in their
program. Prior to this commit, that was determined in the
sizing pass, and all jumps were made long during generation.
Now, the first time the need for a long jump is detected, the
parse is immediately restarted, and all jumps are made long. I
haven't investigated enough to be sure, but it might be
sufficient to just redo the current jump, making it long, and
then switch to using just long jumps, without having to restart
the parse from the beginning.
d) If a reference that could be to capturing parentheses doesn't
find such parentheses, a flag is set. For references that could
be octal constants, they are assumed to be those constants
instead of a capturing group. At the end of the parse, if the
flag indicates either that the assumption(s) were wrong or that
it is a fatal reference to a non-existent group, the pattern is
reparsed knowing the total number of these groups.
e) If (?R) or (?0) are encountered, the flag listed in item d)
above is set to force a reparse. I did have code in place that
avoided doing the reparse, but I wasn't confident enough that
our test suite exercises that area of the code enough to have
exposed all the potential interaction bugs, and I think this
construct is used rarely enough to not worry about avoiding the
reparse at this point in the development.
f) If (?|pattern) is encountered, the behavior is the same as in
item e) above. The pattern will end up being reparsed after the
total number of parenthesized groups are known. I decided not
to invest the effort at this time in trying to get this to work
without a reparse.
It might be that if we are continuing the parse to just count
parentheses, and we encounter a construct that normally would restart
the parse immediately, that we could defer that restart. This would cut
down the maximum number of parses required. As of this commit, the
worst case is we find something that requires knowing all the
parentheses; later we have to switch to /u rules and so the parse is
restarted. Still later we have to switch to long jumps, and the parse
is restarted again. Still later we have to upgrade to UTF-8, and the
parse is restarted yet again. Then the parse is completed, and the
final total of parentheses is known, so everything is redone a final
time. Deferring those intermediate restarts would save a bunch of
reparsing.
Prior to this commit, warnings were issued only during the code
generation pass, which didn't get called unless the sizing pass(es)
completed successfully. But now, we don't know if the pass will
succeed, fail, or whether it will have to be restarted. To avoid
outputting the same warning more than once, the position in the parse of
the last warning generated is kept (across parses). The code looks at
that position when it is about to generate a warning. If the parsing
has previously gotten that far, it assumes that the warning has already
been generated, and suppresses it this time. The current state of
parsing is such that I believe this assumption is valid. If the parses
had divergent paths, that assumption could become invalid.
:100644 100644 8f338ae926ad01022e5e63632857bffb0c297a3e
65991f4b4c0995c64e462b7fd3abe343e576db8d M regcomp.c
:100644 100644 f2ce68fb715e40a9388bb913e88d148fde67d12a
c0b2c07ec5044e20da4de261dc85d6d723f73e50 M regcomp.h
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
|
On Sun, 23 Jan 2022 at 11:40, demerphq ***@***.***> wrote:
On Sun, 23 Jan 2022 at 10:18, demerphq ***@***.***> wrote:
>
> Actually I recant. I will dig more. There is something heisenbug about
> this. If I run the test repeatedly it fails or pases randomly, which is
> making it hard to find the patch that caused this.
>
>
Ok, once I realized I had to run the 10 times or more to reliably see if
the assert fail was triggered I was able to bisect to the below patch.
The test I used was:
(for i in {1..10}; do ./perl -Ilib -e'use re
Debug=>"ALL";qr{(?{a})(?<b>\g{c}})'; done;) 2>&1 | grep Assertion
7c932d0 is the first bad commit
commit 7c932d0
Author: Karl Williamson ***@***.***>
Date: Fri Oct 19 09:48:34 2018 -0600
Remove sizing pass from regular expression compiler
Unfortunately this means the fix for this is not going to be obvious. :-(
Yves
|
|
On Sun, 23 Jan 2022 at 11:52, demerphq ***@***.***> wrote:
On Sun, 23 Jan 2022 at 11:40, demerphq ***@***.***> wrote:
> On Sun, 23 Jan 2022 at 10:18, demerphq ***@***.***> wrote:
>
>>
>> Actually I recant. I will dig more. There is something heisenbug about
>> this. If I run the test repeatedly it fails or pases randomly, which is
>> making it hard to find the patch that caused this.
>>
>>
> Ok, once I realized I had to run the 10 times or more to reliably see if
> the assert fail was triggered I was able to bisect to the below patch.
>
> The test I used was:
>
> (for i in {1..10}; do ./perl -Ilib -e'use re
> Debug=>"ALL";qr{(?{a})(?<b>\g{c}})'; done;) 2>&1 | grep Assertion
>
> 7c932d0 is the first bad commit
> commit 7c932d0
> Author: Karl Williamson ***@***.***>
> Date: Fri Oct 19 09:48:34 2018 -0600
>
> Remove sizing pass from regular expression compiler
>
Unfortunately this means the fix for this is not going to be obvious. :-(
And it isnt. I *really* dont understand Karl's patch. It also fails on a
legit pattern:
./perl -Ilib -e'use re Debug=>"ALL";qr{(?{a})(?<b>\g{c})(?<c>x)}'
and this segfaults totally:
./perl -Ilib -e'use re Debug=>"ALL";qr{(?<b>\g{c})(?<c>x)}'
I haven't worked out what is going on, but it feels like an uninitialized
variable somewhere. I just haven't figured out which yet.
I have to admit I am finding this new "mostly one pass, but sometimes many
pass" compiler strategy super hard to understand.
cheers,
Yves
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
|
On Sun, 23 Jan 2022 at 14:27, demerphq ***@***.***> wrote:
On Sun, 23 Jan 2022 at 11:52, demerphq ***@***.***> wrote:
> On Sun, 23 Jan 2022 at 11:40, demerphq ***@***.***> wrote:
>
>> On Sun, 23 Jan 2022 at 10:18, demerphq ***@***.***> wrote:
>>
>>>
>>> Actually I recant. I will dig more. There is something heisenbug about
>>> this. If I run the test repeatedly it fails or pases randomly, which is
>>> making it hard to find the patch that caused this.
>>>
>>>
>> Ok, once I realized I had to run the 10 times or more to reliably see if
>> the assert fail was triggered I was able to bisect to the below patch.
>>
>> The test I used was:
>>
>> (for i in {1..10}; do ./perl -Ilib -e'use re
>> Debug=>"ALL";qr{(?{a})(?<b>\g{c}})'; done;) 2>&1 | grep Assertion
>>
>> 7c932d0 is the first bad commit
>> commit 7c932d0
>> Author: Karl Williamson ***@***.***>
>> Date: Fri Oct 19 09:48:34 2018 -0600
>>
>> Remove sizing pass from regular expression compiler
>>
>
> Unfortunately this means the fix for this is not going to be obvious. :-(
>
>
And it isnt. I *really* dont understand Karl's patch. It also fails on a
legit pattern:
./perl -Ilib -e'use re Debug=>"ALL";qr{(?{a})(?<b>\g{c})(?<c>x)}'
and this segfaults totally:
./perl -Ilib -e'use re Debug=>"ALL";qr{(?<b>\g{c})(?<c>x)}'
I haven't worked out what is going on, but it feels like an uninitialized
variable somewhere. I just haven't figured out which yet.
I have to admit I am finding this new "mostly one pass, but sometimes many
pass" compiler strategy super hard to understand.
I am probably going to wrap up for the day so I thought i should dump my
findings so far. The issues seems to be related to using parno in the
progi->data->data[] array. It doesnt seem to make sense, we dont stick the
name of a named buffer/reference into the parno slot, and parno in the code
as executed is 0, where it should be >0. I added the assert marked below
with a comment and YJO, and it catches all the issues. The first block
acting on name_list makes sense to me, but i dont understand the part where
we try to read the name out of:
progi->data->data[ parno ]
Things in progi->data->data[] aren't indexed by parno, but rather by the
index returned by add_data(), but we dont seem to use that in the case of
an OPEN/CLOSE, I also cant find a corresponding write to this structure
with the name. Maybe I have missed it. But i feel like something is way off
here.
regcomp.c: 21435:
if (name_list) {
assert(parno>0); /* YJO: parno should be > 0 */
if ( k != REF || (OP(o) < REFN)) {
SV **name= av_fetch(name_list, parno, 0 );
if (name)
Perl_sv_catpvf(aTHX_ sv, " '%" SVf "'", SVfARG(*name));
}
else {
SV *sv_dat= MUTABLE_SV(progi->data->data[ parno ]);
I32 *nums=(I32*)SvPVX(sv_dat);
SV **name= av_fetch(name_list, nums[0], 0 );
Cheers,
Yves
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
demerphq
added a commit
that referenced
this issue
Jan 23, 2022
In 7c932d0 karl changed the regex parser to not do two passes always, and instead do one if it could. However in some edge cases it still must do a second past and some of the info needed for the Debug => "All" would not be available during the first pass. This was compounded by S_add_data() validly returning a 0 index for data that was stored in the data array, which meant that there was no way to tell the difference between "we dont have the data to call S_add_data() at all" and "we called S_add_data() and it returned 0", this in turn would cause the dumping logic to try to access data that was not there, and misinterpret it as a valid SV, with ensure assert fails or worse segfaults. This patch changes S_add_data() so it always returns a non-zero return, so that the regex debug logic can tell that it shouldnt do a lookup into the data array. This means create a new "what" type of "%", which is used SOLELY to populate the ->data[0] and ->what[0] with a dummy entry. With this done we can add guard statements to places that lookup things in the data array, if the index is 0 it can not be valid.
|
On Sun, 23 Jan 2022 at 14:44, demerphq ***@***.***> wrote:
On Sun, 23 Jan 2022 at 14:27, demerphq ***@***.***> wrote:
> On Sun, 23 Jan 2022 at 11:52, demerphq ***@***.***> wrote:
>
>> On Sun, 23 Jan 2022 at 11:40, demerphq ***@***.***> wrote:
>>
>>> On Sun, 23 Jan 2022 at 10:18, demerphq ***@***.***> wrote:
>>>
>>>>
>>>> Actually I recant. I will dig more. There is something heisenbug about
>>>> this. If I run the test repeatedly it fails or pases randomly, which is
>>>> making it hard to find the patch that caused this.
>>>>
>>>>
>>> Ok, once I realized I had to run the 10 times or more to reliably see
>>> if the assert fail was triggered I was able to bisect to the below patch.
>>>
>>> The test I used was:
>>>
>>> (for i in {1..10}; do ./perl -Ilib -e'use re
>>> Debug=>"ALL";qr{(?{a})(?<b>\g{c}})'; done;) 2>&1 | grep Assertion
>>>
>>> 7c932d0 is the first bad commit
>>> commit 7c932d0
>>> Author: Karl Williamson ***@***.***>
>>> Date: Fri Oct 19 09:48:34 2018 -0600
>>>
>>> Remove sizing pass from regular expression compiler
>>>
>>
>> Unfortunately this means the fix for this is not going to be obvious. :-(
>>
>>
> And it isnt. I *really* dont understand Karl's patch. It also fails on a
> legit pattern:
>
> ./perl -Ilib -e'use re Debug=>"ALL";qr{(?{a})(?<b>\g{c})(?<c>x)}'
>
> and this segfaults totally:
>
> ./perl -Ilib -e'use re Debug=>"ALL";qr{(?<b>\g{c})(?<c>x)}'
>
> I haven't worked out what is going on, but it feels like an uninitialized
> variable somewhere. I just haven't figured out which yet.
>
> I have to admit I am finding this new "mostly one pass, but sometimes
> many pass" compiler strategy super hard to understand.
>
> I am probably going to wrap up for the day so I thought i should dump my
findings so far. The issues seems to be related to using parno in the
progi->data->data[] array. It doesnt seem to make sense, we dont stick the
name of a named buffer/reference into the parno slot, and parno in the code
as executed is 0, where it should be >0. I added the assert marked below
with a comment and YJO, and it catches all the issues. The first block
acting on name_list makes sense to me, but i dont understand the part where
we try to read the name out of:
progi->data->data[ parno ]
Things in progi->data->data[] aren't indexed by parno, but rather by the
index returned by add_data(), but we dont seem to use that in the case of
an OPEN/CLOSE, I also cant find a corresponding write to this structure
with the name. Maybe I have missed it. But i feel like something is way off
here.
regcomp.c: 21435:
if (name_list) {
assert(parno>0); /* YJO: parno should be > 0 */
if ( k != REF || (OP(o) < REFN)) {
SV **name= av_fetch(name_list, parno, 0 );
if (name)
Perl_sv_catpvf(aTHX_ sv, " '%" SVf "'", SVfARG(*name));
}
else {
SV *sv_dat= MUTABLE_SV(progi->data->data[ parno ]);
I32 *nums=(I32*)SvPVX(sv_dat);
SV **name= av_fetch(name_list, nums[0], 0 );
And just after I posted that I had an epiphany. Basically this was a
version of the empty predicate problem.
In some cases the code can't resolve things until the second pass, in the
first pass it uses a 0 to represent "nothing added to the data array", but
0 is a legit return value for S_add_data(), so there was no way to
differentiate "no value" from a legit 0 index.
I pushed yves/fix_19350 which changes S_add_data() so it reserves the 0
slot of the data array for an empty unused value. It then always returns a
true value to its callers. This then allows us to tell apart "no value" 0
from a legit value. Anything indexing the data array should be using a
non-zero index into it. This then allows us to ensure that the debug code
doesnt try to look up data->data[0] when it isnt supposed to.
Karl, I think you should review, but this makes sense to me. I also think
there was an off by one error in S_add_data, where it did not allocate
enough elements.
Branch includes a test for this assert, however it really needs to be run K
times to detect a fail, which i am not sure we have as a feature in
fresh_perl_is(). We also should add more tests for the other cases
including the segfault, but I'm out of time for now.
One thing I am not 100% sure I can explain is the heisenbug nature of the
original bug. But this patch seems to reliably fix the problem.
Fixing this took about 6 hours of dev time.
cheers.
Yves
$ git log -2
commit 5be0fb5 (HEAD -> blead,
origin/yves/fix_19350)
Author: Yves Orton ***@***.***>
Date: Sun Jan 23 09:58:54 2022 +0100
Test for Issue #19250, assert fail with Debug=>ALL
commit 50b5798
Author: Yves Orton ***@***.***>
Date: Sun Jan 23 15:55:13 2022 +0100
Fixup Issue #19350 - Assert error under: use re Debug=>"ALL"
In 7c932d0 karl changed the regex
parser
to not do two passes always, and instead do one if it could. However in
some edge cases it still must do a second past and some of the info
needed
for the Debug => "All" would not be available during the first pass.
This was compounded by S_add_data() validly returning a 0 index for
data that was stored in the data array, which meant that there was no
way to tell the difference between "we dont have the data to call
S_add_data()
at all" and "we called S_add_data() and it returned 0", this in turn
would cause the dumping logic to try to access data that was not there,
and misinterpret it as a valid SV, with ensure assert fails or worse
segfaults.
This patch changes S_add_data() so it always returns a non-zero
return, so that the regex debug logic can tell that it shouldnt do
a lookup into the data array. This means create a new "what" type
of "%", which is used SOLELY to populate the ->data[0] and ->what[0]
with a dummy entry.
With this done we can add guard statements to places that lookup
things in the data array, if the index is 0 it can not be valid.
|
demerphq
added a commit
that referenced
this issue
Jan 23, 2022
This test should be run multiple times, and we should add more tests for the segfaulting cases. Will do it tomorrow.
|
I'm examining this in detail. I think that clang asan and valgrind don't have the heisenbug problem. asan revealed undefined behavior in a 32 bit int word which I've fixed in blead. |
|
On Sun, 23 Jan 2022 at 23:31, Karl Williamson ***@***.***> wrote:
I'm examining this in detail. I think that clang asan and valgrind don't
have the heisenbug problem. asan revealed undefined behavior in a 32 bit
int word which I've fixed in blead.
FWIW, I am pretty confident of my analysis except for one thing (fencepost
error), and reverting my fix but keeping the test and rebasing on top of
your most recent change fails test. IOW, whatever you did didnt fix the
problem.
In the following all the line numbers are from before my patch:
in S_handle_named_backref() we initialize "num" to be 0, and then
initialise sv_dat from reg_scan_name():
regcomp.c line 11093:
S_handle_named_backref(pTHX_ RExC_state_t *pRExC_state,
I32 *flagp,
char * parse_start,
char ch
)
{
regnode_offset ret;
char* name_start = RExC_parse;
U32 num = 0;
SV *sv_dat = reg_scan_name(pRExC_state, REG_RSN_RETURN_DATA);
If we look into S_reg_scan_name we see:
regcomp.c line 9032 S_reg_scan_name:
else if (flags==REG_RSN_RETURN_DATA) {
HE *he_str = NULL;
SV *sv_dat = NULL;
if ( ! sv_name ) /* should not happen*/
Perl_croak(aTHX_ "panic: no svname in reg_scan_name");
if (RExC_paren_names)
he_str = hv_fetch_ent( RExC_paren_names, sv_name, 0, 0 );
if ( he_str )
sv_dat = HeVAL(he_str);
if ( ! sv_dat ) { /* Didn't find group */
/* It might be a forward reference; we can't fail until we
* know, by completing the parse to get all the groups, and
* then reparsing */
if (ALL_PARENS_COUNTED) {
vFAIL("Reference to nonexistent named group");
}
else {
REQUIRE_PARENS_PASS;
}
}
return sv_dat;
}
So if ALL_PARENS_COUNTED is false we trigger REQUIRE_PARENS_PASS, and then
return a null sv_dat back to S_handle_named_backref, which then eventually
reaches this code:
regcomp.c line 11117 S_handle_named_backref():
if (sv_dat) {
num = add_data( pRExC_state, STR_WITH_LEN("S"));
RExC_rxi->data->data[num]=(void*)sv_dat;
SvREFCNT_inc_simple_void_NN(sv_dat);
}
RExC_sawback = 1;
ret = reganode(pRExC_state,
((! FOLD)
? REFN
: (ASCII_FOLD_RESTRICTED)
? REFFAN
: (AT_LEAST_UNI_SEMANTICS)
? REFFUN
: (LOC)
? REFFLN
: REFFN),
num);
As we can see, if sv_data is null, we do not call add_data(), and we
construct a regnode with an argument of num=0. This would be the same thing
we would do if this was the first item we had added data to while parsing
the pattern. Later on we end up calling Perl_reg_prop(), where we fall into
this logic:
regcomp.c line 21442:
SV *sv_dat= MUTABLE_SV(progi->data->data[ parno ]);
I32 *nums=(I32*)SvPVX(sv_dat);
parno is not actually a parno in this context, it is the arg value set up
previously in "num" in S_handle_named_backref() which would be 0 for the
first pass. We then end up dereferencing the contents of data[parno] as an
SV, but crucially *we never wrote an SV* into the slot, whether this
asserts, works, or segfaults really depends on what was in that memory slot.
Which is why i changed it to so that S_add_data() would always return a
non-zero value, we can now distinguish "this arg is bogus, ignore it" from
"this arg references an item in the data array".
The only part I am not sure about is this:
Renewc(RExC_rxi->data,
- sizeof(*RExC_rxi->data) + sizeof(void*) * (count + n - 1),
+ sizeof(*RExC_rxi->data) + (sizeof(void*) * (count + n)),
char, struct reg_data);
I changed the expression (count + n -1) to (count + n) because it read
wrong to me that we would allocate 0 elements on the first try, but
checking the definition of the struct:
struct reg_data {
U32 count;
U8 *what;
void* data[1];
};
I think I am wrong as the structure will always contain at least one
element. I will revise the patch and add a comment explaining the -1. I
will also add more tests, especially for the segfaulting case.
Cheers,
Yves
|
demerphq
added a commit
that referenced
this issue
Jan 24, 2022
…SE") In 7c932d0 karl changed the regex parser to not do two passes always, and instead do one if it could. However in some edge cases it still must do a second past and some of the info needed for the Debug => "All" would not be available during the first pass. This was compounded by S_add_data() validly returning a 0 index for data that was stored in the data array, which meant that there was no way to tell the difference between "we dont have the data to call S_add_data() at all" and "we called S_add_data() and it returned 0", this in turn would cause the dumping logic to try to access data that was not there, and misinterpret it as a valid SV, with ensure assert fails or worse segfaults. This patch changes S_add_data() so it always returns a non-zero return, so that the regex debug logic can tell that it shouldnt do a lookup into the data array. This means create a new "what" type of "%", which is used SOLELY to populate the ->data[0] and ->what[0] with a dummy entry. With this done we can add guard statements to places that lookup things in the data array, if the index is 0 it can not be valid.
demerphq
added a commit
that referenced
this issue
Jan 24, 2022
Note that 'use re Debug => "ALL"' and 'use re Debug => "PARSE"' are basically the same as far as this issue goes. ALL implies PARSE along with other things, the fault is in how the PARSE part works, and the omitting the full output of ALL and just testing PARSE is sufficient to test this issue, and avoids any need to deal with memory addresses or word size issues that are irrelevant to the issue anyway.
|
On Mon, 24 Jan 2022 at 05:17, demerphq ***@***.***> wrote:
I will revise the patch and add a comment explaining the -1. I will also
add more tests, especially for the segfaulting case.
I have force pushed yves/fix_19350 with both of these matters reflected.
Added a test for the segfaulting valid pattern, and made
the test run 10 times to make sure we notice the bug. If you
revert c982e53 you should see the tests
added in a1fc433 start to fail.
cheers,
Yves
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
|
I'm examining this in detail. I think that clang asan and valgrind don't have the heisenbug problem. asan revealed undefined behavior in a 32 bit int word which I've fixed in blead. |
|
I have no problems with the patch, besides the trivial ones of a typo in a comment, and in the commit message. And there are new trailing white spaces that git complains about. github seems to hide these. Do we have an official position on these? Thanks for the patch and analysis. I wasn't convinced that there was a one-off error, but adding one as a precaution seems fine to me. My real concern is that the test feels brittle. It would fail if we make even trivial changes in the output of Debug, which becomes a future maintenance time waster. Isn't the segfault a legitimate reduction for the initial problem? If so why not use it. If not, I think the fact that valgrind and asan don't exhibit heisenbug issues means we don't have to run the test x number of times |
|
On Tue, 25 Jan 2022 at 04:37, Karl Williamson ***@***.***> wrote:
I have no problems with the patch, besides the trivial ones of a typo in a
comment, and in the commit message.
Happy to fix that up. I'll review, but given i missed them already it might
be helpful if you point out which.
And there are new trailing white spaces that git complains about. github
seems to hide these. Do we have an official position on these?
Afaik those are in the test and are part of the output of the script, since
we are testing the output we have to keep them as far as I understand.
If there is any trailing whitespace in the non-heredoc part of the patch
I'll remove it but I think I have already, I will double check.
Thanks for the patch and analysis.
I wasn't convinced that there was a one-off error, but adding one as a
precaution seems fine to me.
I am with you there, I was able to satisfy myself that my original
intuition was wrong and I have adjusted the most recent version of the
branch to remove that change and instead to add a comment explaining why it
looks a bit weird.
My real concern is that the test feels brittle. It would fail if we make
even trivial changes in the output of Debug, which becomes a future
maintenance time waster.
I feel like if we had had a test like this then this bug would have been
noticed when you made the patch, so having such a test seems a good idea.
It isnt that hard to fixup if the output changes IMO. AFAIK we have no
tests for re Debug => ALL/PARSE. Maybe in a future patch I could look into
make the test a bit more future proof.
Isn't the segfault a legitimate reduction for the initial problem?
Testing for *that* segfault in this logic doesn't test for other issues in
the Debug => ALL/PARSE output.
If so why not use it. If not, I think the fact that valgrind and asan
don't exhibit heisenbug issues means we don't have to run the test x number
of times
My view is the test should show the error being tested if the patch that
fixes it is reverted. Under a normal build with a single run that does not
seem to be guaranteed, so I made it run 10 times. I dont think we normally
build with asan or valgrind. But it's not a hill I want to die on, if you
really prefer we run it once then that is fine too.
cheers,
Yves
|
khwilliamson
pushed a commit
that referenced
this issue
Feb 1, 2022
In 7c932d0 karl changed the regex parser to not do two passes always, and instead do one if it could. However in some edge cases it still must do a second past and some of the info needed for the Debug => "All" would not be available during the first pass. This was compounded by S_add_data() validly returning a 0 index for data that was stored in the data array, which meant that there was no way to tell the difference between "we dont have the data to call S_add_data() at all" and "we called S_add_data() and it returned 0", this in turn would cause the dumping logic to try to access data that was not there, and misinterpret it as a valid SV, with ensure assert fails or worse segfaults. This patch changes S_add_data() so it always returns a non-zero return, so that the regex debug logic can tell that it shouldnt do a lookup into the data array. This means create a new "what" type of "%", which is used SOLELY to populate the ->data[0] and ->what[0] with a dummy entry. With this done we can add guard statements to places that lookup things in the data array, if the index is 0 it can not be valid.
|
You persuaded me. I fixed one typo, reflowed overlong comments, and pushed otherwise unchanged. This issue should now be fixed by a963d6d |
|
Ah thanks. I had already prepped a patch but I was too busy to get it
upstream. I can't push to github while on my corporate VPN and the last
days I was feverishly working on a project with a hard deadline. All done
now. I had planned to push today. I added more comments in my unused branch
so I'll check what is missing and push a follow up.
Yves
Yves
…On Wed, 2 Feb 2022, 06:51 Karl Williamson, ***@***.***> wrote:
You persuaded me. I fixed one typo, reflowed overlong comments, and pushed
otherwise unchanged. This issue should now be fixed by a963d6d
<a963d6d>
—
Reply to this email directly, view it on GitHub
<#19350 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAZ5RZAU4OLAQRAQQZ32A3UZBPWRANCNFSM5MIRV2UQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
On Wed, 2 Feb 2022 at 00:54, demerphq ***@***.***> wrote:
Ah thanks. I had already prepped a patch but I was too busy to get it
upstream. I can't push to github while on my corporate VPN and the last
days I was feverishly working on a project with a hard deadline. All done
now. I had planned to push today. I added more comments in my unused branch
so I'll check what is missing and push a follow up.
Hrm I guess I did push to my branch before you did your thing, I couldn't
see anything missing. Thanks!
IF this was reopened by this email please close!
Yves
|
steve-m-hay
pushed a commit
that referenced
this issue
Feb 26, 2022
In 7c932d0 karl changed the regex parser to not do two passes always, and instead do one if it could. However in some edge cases it still must do a second past and some of the info needed for the Debug => "All" would not be available during the first pass. This was compounded by S_add_data() validly returning a 0 index for data that was stored in the data array, which meant that there was no way to tell the difference between "we dont have the data to call S_add_data() at all" and "we called S_add_data() and it returned 0", this in turn would cause the dumping logic to try to access data that was not there, and misinterpret it as a valid SV, with ensure assert fails or worse segfaults. This patch changes S_add_data() so it always returns a non-zero return, so that the regex debug logic can tell that it shouldnt do a lookup into the data array. This means create a new "what" type of "%", which is used SOLELY to populate the ->data[0] and ->what[0] with a dummy entry. With this done we can add guard statements to places that lookup things in the data array, if the index is 0 it can not be valid. (cherry picked from commit a963d6d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Module: re
NOTE: Just seems to be a Mac only issue please post your version if you are reporting as a false positive.
I just tested on a Mac 12.1 virtual machine (in case my original installation was corrupt) and is still reproducible).
Description
Assertion failed when I use
qw(Debug ALL)in my program (on my default built-in perl mac os 12.1).Steps to Reproduce
Expected behavior
Not assert fail.
Perl configuration
The text was updated successfully, but these errors were encountered: