Segfault (memory access violation) in Perl__invlist_intersection_maybe_complement_2nd in Perl 5.30.0 #17154
Comments
From steven.bush@3ds.comContent-Type: multipart/mixed; boundary="------------1.41.perlbug" This is a multi-part message in MIME format. This is a bug report for perl from steven.bush@3ds.com, Summary: Segfault (memory access violation) in Perl__invlist_intersection_maybe_complement_2nd during eval() after Perl parser embedded in C++ program is destructed pe_destruct() after upgrading from perl-5.26.3 to perl-5.30.0. Also seen in perl-5.28.2 Description: With Perl-5.26.3 and earlier, this works successfully. #0 0x000000336d689a54 in memcpy () from /lib64/libc.so.6 I compiled perl-5.30.0 with debugging enabled and tracked the problem down as follows: The crash is occurring with regex expressions containing character sets "[abc]" During second Perl evaluation, the debugger broke on an assert inside S__invlist_len which is called from Perl__invlist_intersection_maybe_complement_2nd. if ((len_a == 0) || ((len_b = _invlist_len(b)) == 0)) { In this case, "b" is an SV struct which is invalid: Looking further up the stack to S_populate_ANYOF_from_invlist(), b has been passed in PL_InBitmap which is a global variable defined in embedvar.h PL_InBitmap is initialized within Perl_re_op_compile() which has changed within regcomp.c from perl-5.26.1 to perl-5.30.0 as follows: PL_AboveLatin1 = _new_invlist_C_array(AboveLatin1_invlist); /* This is calculated here, because the Perl program that generates the 5.30.0/5.28.2: /* This is calculated here, because the Perl program that generates the In 5.30.0, PL_AboveLatin1 is initialized in a function at the bottom of regcomp.c: On the first call to Perl_re_op_compile() during the first script evaluation, PL_InBitmap = NULL and the initializer is called and PL_InBitmap's contents are valid. However, after calling perl_destruct(), PL_InBitmap is no longer NULL, but the contents of PL_InBitmap have been wiped out. Stepping through perl_destruct() in perl.c after the first script eval, PL_InBitmap becomes corrupt when this code executes: This actually wipes out the contents of all of the PL_ globals (such as PL_AboveLatin1, PL_Latin1, PL_UpperLatin1), however those globals are reinitialized regardless of current status during perl_construct() when the function calls init_uniprops() I modified the code in perl_destruct() as follows: After recompiling, all of our product tests passed successfully. Flags: Site configuration information for perl 5.30.0: Configured by scitegicuser at Thu Jun 6 13:23:20 2019. Summary of my perl5 (revision 5 version 30 subversion 0) configuration: Platform: @INC for perl 5.30.0: Environment for perl 5.30.0: --------------1.41.perlbug --- C:/dev/perl.c Tue Sep 17 13:21:31 2019 +++ C:/dev/perl.c-FIXED Tue Sep 17 12:12:36 2019 @@ -1254,6 +1254,7 @@ /* the 2 is for PL_fdpid and PL_strtab */ while (sv_clean_all() > 2) ; + PL_InBitmap = NULL; #ifdef USE_ITHREADS Safefree(PL_stashpad); /* must come after sv_clean_all */ --------------1.41.perlbug-- If you are not one of the named recipients or have received this email in error, (i) you should not read, disclose, or copy it, (ii) please notify sender of your receipt by reply email and delete this email and all attachments, (iii) Dassault Systèmes does not accept or assume any liability or responsibility for any use of or reliance on this email. Please be informed that your personal data are processed according to our data privacy policy as described on our website. Should you have any questions related to personal data protection, please contact 3DS Data Protection Officer at 3DS.compliance-privacy@3ds.com<mailto:3DS.compliance-privacy@3ds.com> For other languages, go to https://www.3ds.com/terms/email-disclaimer |
From @tonycozOn Tue, 17 Sep 2019 14:50:07 -0700, steven.bush@3ds.com wrote:
Do you have a more complete example of this? I've tried some simple code that exercises regexps with [abc], but haven't managed to make it fault, even with valgrind running the executable. My test code is running the code twice and PL_InBitmap is the same after each run, so I can see there is a problem, but I'm not seeing the crash, which I'd like for a regression test. Thanks, |
|
The RT System itself - Status changed from 'new' to 'open' |
From @khwilliamsonOn Tue, 17 Sep 2019 23:03:05 -0700, tonyc wrote:
It would be great to have the data for a regression test, but Steven has done an admirable job of diagnosing the problem. I prefer the attached patch, as it keeps the initialization of all the similar variables in the same place |
From @khwilliamson0005-PATCH-perl-134339-Segfault-in-embedded-use.patchFrom e2af073f0263f3725a7f775de425bfd9f0a5dcbd Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Wed, 18 Sep 2019 09:18:56 -0600
Subject: [PATCH 5/5] PATCH: [perl #134339] Segfault in embedded use.
Insert verbiage here
---
regcomp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/regcomp.c b/regcomp.c
index 531cf306c9..f176c472ea 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -22191,6 +22191,8 @@ Perl_init_uniprops(pTHX)
PL_Latin1 = _new_invlist_C_array(Latin1_invlist);
PL_UpperLatin1 = _new_invlist_C_array(UpperLatin1_invlist);
+ PL_InBitmap = NULL; /* Force run-time initialization [perl #134439] */
+
PL_Assigned_invlist = _new_invlist_C_array(uni_prop_ptrs[UNI_ASSIGNED]);
PL_utf8_perl_idstart = _new_invlist_C_array(uni_prop_ptrs[UNI__PERL_IDSTART]);
--
2.17.1
|
From steven.bush@3ds.comApologies if my message is duplicated. I replied in e-mail to the comments earlier today, but the response did not appear on the site (maybe I'm not patient enough). I agree that khw's patch is better than my initial patch: it has the same behavior but keeps things centralized. I put together the following C++ test harness that reliably replicates the crashing behavior on my laptop. It requires File::Spec. The crash occurs while parsing the regex from PathTools Cwd.pm: "^[ET1]" #include <EXTERN.h> int main(int argc, char* argv[]) PERL_SYS_INIT3(&args_c, &args_v, NULL); perl_construct(my_perl); eval_pv(TestPerlScript, TRUE); perl_destruct(my_perl); my_perl = perl_alloc(); /* CRASHES HERE */ perl_destruct(my_perl); PERL_SYS_TERM(); On Wed, 18 Sep 2019 08:23:38 -0700, khw wrote:
|
From @tonycozOn Wed, Sep 18, 2019 at 08:23:38AM -0700, Karl Williamson via RT wrote:
I wonder if this is just generally unsafe. Could PL_InBitmap be initialized by a child thread? This would leave PL_InBitmap invalid once the child thread exits. It's also possible a CPAN module could call perl_construct(), and The other inversion list globals have similar issues, as far as I can Tony |
From steven.bush@3ds.comHi Tony, I've had the same problem of trying to create a simple test harness for this. The perl regcomp code crashes consistently within our full sized application, but the script we are loading is moderately complex and includes a number of other modules. In our case, from stepping through the debugger on Windows, the regex that is causing problems is coming from Cwd.pm: But when I try that in a test harness, it does not crash The code that we have that pulls in the code is this: I put together the following test C++ based test harness that seems to do the trick. This was wrapped into a simple main() with #include <EXTERN.h> and <perl.h> at the top static const char* TestPerlScript = "use File::Spec;\n"; PERL_SYS_INIT3(&argc, &argv, NULL); perl_construct(my_perl); eval_pv(TestPerlScript, TRUE); perl_destruct(my_perl); printf("PASS 1 Success\n----\n"); my_perl = perl_alloc(); eval_pv(TestPerlScript, TRUE); //<-------- SEGFAULTS HERE perl_destruct(my_perl); PERL_SYS_TERM(); -----Original Message----- On Tue, 17 Sep 2019 14:50:07 -0700, steven.bush@3ds.com wrote:
Do you have a more complete example of this? I've tried some simple code that exercises regexps with [abc], but haven't managed to make it fault, even with valgrind running the executable. My test code is running the code twice and PL_InBitmap is the same after each run, so I can see there is a problem, but I'm not seeing the crash, which I'd like for a regression test. Thanks, If you are not one of the named recipients or have received this email in error, (i) you should not read, disclose, or copy it, (ii) please notify sender of your receipt by reply email and delete this email and all attachments, (iii) Dassault Systèmes does not accept or assume any liability or responsibility for any use of or reliance on this email. Please be informed that your personal data are processed according to our data privacy policy as described on our website. Should you have any questions related to personal data protection, please contact 3DS Data Protection Officer at 3DS.compliance-privacy@3ds.com<mailto:3DS.compliance-privacy@3ds.com> For other languages, go to https://www.3ds.com/terms/email-disclaimer |
|
Looking at perl.c, I see that it skips cleaning up the ENV variables if it isn't the main thread. Could that work for this issue as well? |
|
The problem is all SVs are released when the interpreter (per thread) is released, so a PL_InBitmap allocated by a child thread is released when child thread ends. Similarly in this embedded case where a process creates an interpreter, populates PL_InBitmap, and releases the interpreter, freeing the SV, leaving PL_InBitmap invalid. I can only see two fixes:
The current initialization has a race condition too - it's possible for two child threads to reach the initialization in re_op_compile() and both try to initialize PL_InBitmap. |
|
I already have a patch prepared that makes PL_InBitmap work just like all the other inversion list globals. Earlier you said "The other inversion list globals have similar issues, as far as I can Would those issues go away if they were only cleaned up by the main thread? |
|
The problem the other inversion list globals is with code like threads::lite that call perl_construct(). Normal threading code calls perl_clone() which duplicates the parent interpreter and doesn't call init_uniprops() so it leaves the inversion list globals alone. Since the parent waits on the child threads there's no lifetime issue. threads::lite, and possibly other code (mod_perl?) calls perl_construct() which initializes PL_AboveLatin1 etc, so you can get a sequence of events like:
threads::lite isn't doing anything horrible by calling perl_construct(), this should be safe for it to do, so it seems like a fault in the way those variables are handled. They're all initialized from static arrays and don't copy those arrays, so they're relatively cheap to create, so I could see them as interpreter variables, but there are a lot of them Also, using globals is broken if the code ever iterates over them, though I don't know if any code ever does. (Moving the iterator state out of the SV would fix that.) |
This is part of fixing gh #17154 This scenario from the ticket (#17154 (comment)) shows why this fix is necessary: main interpreter initializes PL_AboveLatin1 to an SV it owns loads threads::lite and creates a new thread/interpreter which initializes PL_AboveLatin1 to a SV owned by the new interpreter threads::lite child interpreter finishes, freeing all of its SVs, PL_AboveLatin1 is now invalid main interpreter uses a regexp that relies on PL_AboveLatin1, dies horribly. By making these interpreter level variables, this is avoided. There is extra copying, but it is just the SV headers, as the real data is kept as static C arrays.
This makes it consistent with the other inversion lists for this sort of thing, and finishes the fix for GH #17154
|
This is now fixed in blead by the two commits referred to above. I'm working on a patch suitable for backporting to 5.30. Steve, let me know if you want that. |
|
Changing interpreter struct size breaks BOOT checks, so can't be backported. |
This makes it consistent with the other inversion lists for this sort of thing, and finishes the fix for GH #17154
|
I looked at blead and found that commit. It is needed to fix 5.32, and it is in blead: commit ffffbf0 |
|
I would be interested in a backported fix for 5.30 as it affects nginx: See https://bugs.mageia.org/show_bug.cgi?id=26397 |
|
Also affected by this on 5.30. |
|
I am willing to worth with people to get a patch available for earlier versions, even if it can't be an official backport. |
This resolves #17774. This ticket is because the fixes in GH #17154 failed to get every case, leaving this one outlier to be fixed by this commit. The text in #17154 gives extensive details as to the problem. But briefly, in an attempt to speed up interpreter cloning, I moved certain SVs from interpreter level to global level in e80a011 (v5.27.11, March 2018). This was doable, we thought, because the content of these SVs is constant throughout the life of the program, so no need to copy them when cloning a new interpreter or thread. However when an interpreter exits, all its SVs get cleaned up, which caused these to become garbage in applications where another interpreter remains running. This circumstance is rare enough that the bug wasn't reported until September 2019, #17154. I made an initial attempt to fix the problem, and closed that ticket, but I overlooked one of the variables, which was reported in #17774, which this commit addresses. Effectively the behavior is reverted to the way it was before e80a011.
This resolves #17774. This ticket is because the fixes in GH #17154 failed to get every case, leaving this one outlier to be fixed by this commit. The text in #17154 gives extensive details as to the problem. But briefly, in an attempt to speed up interpreter cloning, I moved certain SVs from interpreter level to global level in e80a011 (v5.27.11, March 2018). This was doable, we thought, because the content of these SVs is constant throughout the life of the program, so no need to copy them when cloning a new interpreter or thread. However when an interpreter exits, all its SVs get cleaned up, which caused these to become garbage in applications where another interpreter remains running. This circumstance is rare enough that the bug wasn't reported until September 2019, #17154. I made an initial attempt to fix the problem, and closed that ticket, but I overlooked one of the variables, which was reported in #17774, which this commit addresses. Effectively the behavior is reverted to the way it was before e80a011.
Migrated from rt.perl.org#134439 (status was 'open')
Searchable as RT134439$
The text was updated successfully, but these errors were encountered: