Skip Menu |

This queue is for tickets about the Scalar-List-Utils CPAN distribution.

Report information
The Basics
Id: 80646
Status: resolved
Priority: 0/
Queue: Scalar-List-Utils

People
Owner: Nobody in particular
Requestors: dolmen [...] cpan.org
yyang [...] proofpoint.com
Cc: ribasushi [...] leporine.io
AdminCc:

Bug Information
Severity: Critical
Broken in: 1.25
Fixed in: 1.27



Subject: Recursive MULTICALL prematurely freed CV
Test script to make List::Util segfault: use List::Util 'first'; sub foo { if($comparison) { return 1; } else { local $comparison = 1; first \&foo, 1,2,3; } } for(1,2){ foo() } This is Perl Bug #78070, fixed in Perl 5.13.6: •The XS multicall API no longer causes subroutines to lose reference counts if called via the multicall interface from within those very subroutines. This affects modules like List::Util. Calling one of its functions with an active subroutine as the first argument could cause a crash So we can backport it here: --- multicall.h~ 2012-11-05 10:41:35.487993000 -0800 +++ multicall.h 2012-11-05 10:41:58.861218000 -0800 @@ -154,8 +154,8 @@ #define POP_MULTICALL \ STMT_START { \ - CvDEPTH(multicall_cv)--; \ - LEAVESUB(multicall_cv); \ + if (! --CvDEPTH(multicall_cv)) \ + LEAVESUB(multicall_cv); \ POPBLOCK(cx,PL_curpm); \ POPSTACK; \ CATCH_SET(multicall_oldcatch); \
Curious. Using that unit test I can confirm it breaks on e.g. 5.12.4, but the patch doesn't fix it. It breaks even with that patch in place. Moreover it doesn't appear that the included "multicall.h" file is actually used by default. I've tried just inserting silly syntax breakage into it, and that doesn't break the build. -- Paul Evans
On Wed Nov 07 15:40:22 2012, PEVANS wrote: Show quoted text
> Curious. > > Using that unit test I can confirm it breaks on e.g. 5.12.4, but the > patch doesn't fix it. It breaks even with that patch in place. > > Moreover it doesn't appear that the included "multicall.h" file is > actually used by default. I've tried just inserting silly syntax > breakage into it, and that doesn't break the build.
multicall.h is used only on 5.8, which didn’t provide the multicall API. The bug that this patch fixes for 5.8 is present in perl’s own multicall API in perl 5.10 and 5.12. Tweaking the reference count right before using the macro might be the best fix for those two versions.
Show quoted text
> multicall.h is used only on 5.8, which didn’t provide the multicall > API. > > The bug that this patch fixes for 5.8 is present in perl’s own > multicall API in perl 5.10 and > 5.12. > > Tweaking the reference count right before using the macro might be the > best fix for those > two versions.
Hi, Just reviving this again. I'm at a loss to understand exactly what is required here. Can you suggest what if any changes to code actually in Scalar-List-Utils needs to be done? -- Paul Evans
Don't have any suggestions, just chiming in with the severity of impact - the test fails and/or segfaults on all perls < 5.14, this includes even 5.12.5 :(
On Sun Dec 16 14:07:34 2012, PEVANS wrote: Show quoted text
> > multicall.h is used only on 5.8, which didn’t provide the multicall > > API. > > > > The bug that this patch fixes for 5.8 is present in perl’s own > > multicall API in perl 5.10 and > > 5.12. > > > > Tweaking the reference count right before using the macro might be the > > best fix for those > > two versions.
> > Hi, > > Just reviving this again. I'm at a loss to understand exactly what is > required here. Can you suggest what if any changes to code actually in > Scalar-List-Utils needs to be done?
Something like this (sorry, completely untested): #if PERL_VERSION == 10 || PERL_VERSION == 12 if (CvDEPTH(multicall_cv) > 1) SvREFCNT_inc_simple_void_NN(multicall_cv); #endif POP_MULTICALL; And the patch to multicall.h for 5.8.
This now looks to be patched at least for 5.8.9, 5.10 and 5.12. -- Paul Evans
On Thu Dec 27 10:16:11 2012, PEVANS wrote: Show quoted text
> This now looks to be patched at least for 5.8.9, 5.10 and 5.12.
Thank you very much. Confirmed to compile/pass tests on all of this, except for 5.6.2. The fix for that seems easy-ish, but I will leave patching to the more knowledgeable. rabbit@Thesaurus:~$ perlbrew list 5.10.0_2 5.10.0_dbg1 5.10.0_tr 5.10.1 5.10.1_2 5.10.1_dbg1 5.10.1_no_thr 5.12.0_dbg1 5.12.1 5.12.2 5.12.3 5.12.4 5.12.4_dbg1 5.12.5 5.12.5_nthr 5.14.1_thr 5.14.1_thr_mb_dbg 5.14.2_dbg_thr_mb 5.14.2_thr_cln 5.14.3 5.14.3_nthr 5.15.8 5.15.9 5.16.0 * 5.16.2.1 5.16.2_nthr 5.17.6 5.6.2 5.8.1 5.8.1_dbg_mb 5.8.2 5.8.3 5.8.3_cln 5.8.3_dbg_thr 5.8.3_tr 5.8.4 5.8.5 5.8.5_clean_cattest 5.8.6 5.8.7 5.8.8 5.8.8_cpanmeta 5.8.8_dbg 5.8.9 5.8.9_dbg1 5.8.9_f
On Sat Dec 29 01:01:48 2012, RIBASUSHI wrote: Show quoted text
> Thank you very much. Confirmed to compile/pass tests on all of this, > except for 5.6.2. The fix for that seems easy-ish, but I will leave > patching to the more knowledgeable.
Cool. That's nice to know. I don't think personally I care much as back as 5.6 - certainly, I won't put in any effort up-front now to fix it. If someone else wants to submit a patch I'll happily accept it, but for now I shall consider this one fixed. -- Paul Evans