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
PL_keyword_plugin needs thread-safe wrap setter #16229
Comments
From @maukeCreated by @maukePL_keyword_plugin (see perldoc perlapi) is global to the entire process and | [...] take a copy of PL_keyword_plugin before assigning your own function But it does not show any example code of how this "taking a copy" is supposed The XS::APItest module in the core test suite does it the following way | static int (*next_keyword_plugin)(pTHX_ char *, STRLEN, OP **); This code has found its way into various CPAN modules (e.g. It is also not thread-safe: % perl -Mthreads -e 'threads->create(sub { require Function::Parameters })->join for 1 .. 2' This is because each thread runs BOOT separately, but PL_keyword_plugin is Then the next call to my_keyword_plugin that enters the 'else' block will In other words, the list of linked handlers contains a loop, leading to a stack overflow. PL_check (the other global variable documented in perlapi) has this paragraph | For thread safety, modules should not write directly to this array. Instead, This equally applies to PL_keyword_plugin, but a corresponding Perl Info
|
From @leonerdOn Tue, 07 Nov 2017 09:44:49 -0800
In fact this issue came to light precisely because someone found it in https://rt.cpan.org/Ticket/Display.html?id=123547 I've applied the same fix that mauke did; namely to borrow the Since that only turned up in perl 5.14, I've had to document it as a https://metacpan.org/pod/release/PEVANS/Syntax-Keyword-Try-0.09/lib/Syntax/Keyword/Try.pm#KNOWN-BUGS I'd suggest an easy fix would be to either document that this really is While this fix now protects S:K:T against concurrent loading of itself, -- leonerd@leonerd.org.uk | https://metacpan.org/author/PEVANS |
The RT System itself - Status changed from 'new' to 'open' |
From @maukeOn Tue, 07 Nov 2017 09:44:49 -0800, mauke- wrote:
I have done this in a branch: https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/mauke/keyword-plugin-mutex If no one screams, I'll merge this in a couple of days (if I remember). |
From @leonerdOn Wed, 08 Nov 2017 16:34:25 -0800
From the perspective of a potential user of this change, I think that One question: will there be an accompanying update to ppport.h or a -- leonerd@leonerd.org.uk | https://metacpan.org/author/PEVANS |
From @xsawyerxOn 11/09/2017 04:26 AM, Paul "LeoNerd" Evans wrote:
At the moment there is no active maintainer to Devel::PPPort, so authors We've also discussed removing support for any Perl under 5.6. At the Lukas, would you be able to backport this to Devel::PPPort? |
From @maukeAm 09.11.2017 um 10:27 schrieb Sawyer X:
I have no idea how Devel::PPPort works. My suggestion for older perls would be: #ifndef wrap_keyword_plugin Rationale: - To be useful, the compat code must use one single shared mutex. It - OP_CHECK_MUTEX is technically the wrong mutex (it protects access to - OP_CHECK_MUTEX_LOCK / OP_CHECK_MUTEX_UNLOCK are not part of the public - This leaves 5.12 and 5.14 out in the cold. I don't know what to do there. Thoughts? -- |
From zefram@fysh.orgl.mai@web.de wrote:
There are really two distinct issues around thread safety, worth Firstly, there's the issue you demonstrated, that the way in which Secondly, there's the more obscure possibility of a race condition, The comparison to wrap_op_checker() is apt. The scoping is the same, I agree that a wrap_keyword_plugin() function would be sensible. -zefram |
From zefram@fysh.orgl.mai@web.de via RT wrote:
wrap_keyword_plugin() needs to be marked experimental, to match -zefram |
From @maukeOn Thu, 09 Nov 2017 16:24:59 -0800, zefram@fysh.org wrote:
Thanks for the review. I have pushed a new version of my branch. |
From @maukeOn Thu, 09 Nov 2017 16:05:54 -0800, zefram@fysh.org wrote:
In light of this, my back-compat suggestion becomes: #ifndef wrap_keyword_plugin |
From zefram@fysh.orgl.mai@web.de via RT wrote:
The entry in embed.fnc should be flagged "M" to match the "x" on the -zefram |
From @xsawyerxOn 11/10/2017 03:11 AM, l.mai@web.de via RT wrote:
Thank you for this patch, Lukas! Zefram, while you've reviewed the original patch, can you also review I'll look into merging it in Devel::PPPort. Thanks, guys! |
From @leonerdOn Wed, 08 Nov 2017 16:34:25 -0800
A thought occurs to me on this. I have two keyword-plugin-using modules on CPAN; they are Syntax::Keyword::Try Of those two, the first one can be made safe using this function, but The code currently looks like BOOT: XopENTRY_set(&xop_await, xop_name, "await"); next_keyword_plugin = PL_keyword_plugin; I would need a suitable locking mechanism to lock the entire thing. I begin to wonder if this is a wider problem with BOOT itself; that static boot_once(pTHX) { ... } BOOT: -- leonerd@leonerd.org.uk | https://metacpan.org/author/PEVANS |
From zefram@fysh.orgSawyer X wrote:
The proposed reserve definition of wrap_keyword_plugin() is mostly -zefram |
From zefram@fysh.orgPaul "LeoNerd" Evans wrote:
No, there are parts of BOOT that need to run per interpreter. The main -zefram |
From @cpansproutOn Sun, 12 Nov 2017 16:20:47 -0800, zefram@fysh.org wrote:
Do you think a BOOTONCE section would be an appropriate extension to XS, or is it rare enough that we do not need it? -- Father Chrysostomos |
From @maukeAm 13.11.2017 um 01:16 schrieb Zefram:
MUTEX_LOCK/MUTEX_UNLOCK are NOOPs on unthreaded builds. -- |
From zefram@fysh.orgFather Chrysostomos via RT wrote:
It sounds risky, because of the greatly expanded scope of the critical -zefram |
From @maukeOn Wed, 08 Nov 2017 16:34:24 -0800, mauke- wrote:
The branch has been merged in commit 6cc7638. Should the status of this ticket change to "pending release"? |
From @xsawyerxOn 11/13/2017 08:25 PM, l.mai@web.de via RT wrote:
Yes, but I would like to merge the backport solution to Devel::PPPort. Zefram, can you respond on Lukas' latest comment on |
From zefram@fysh.orgSawyer X wrote:
He's right. His version is fine. -zefram |
Migrated from rt.perl.org#132413 (status was 'open')
Searchable as RT132413$
The text was updated successfully, but these errors were encountered: