perl5
perl5 copied to clipboard
Perl
BBC: 5.41.1 breaks PLICEASE/UUID-FFI and others
Description
with v5.41.0-13-g06e421c559 a test started to fail for PLICEASE/UUID-FFI-0.11.tar.gz
Sample fail report: http://www.cpantesters.org/cpan/report/10ab0748-3a79-11ef-9583-ab3925c06771
Bisect says:
06e421c559c63975f29c35ba3588a0e6b0c75eca is the first bad commit
commit 06e421c559c63975f29c35ba3588a0e6b0c75eca
Author: Richard Leach <[email protected]>
Date: Fri Dec 9 23:42:32 2022 +0000
S_fold_constants: remove early SvREADONLY(sv) to allow SvIsCOW(sv)
Standard CONST PVs have the IsCOW flag set, meaning that COW can
be used when assigning the CONST to a variable, rather than making
a copy of the buffer. CONST PVs arising from constant folding have
been lacking this flag, leading to unnecessary copying of PV buffers.
This seems to have occurred because a common branch in S_fold_constants
marks SVs as READONLY before the new CONST OP is created. When the OP
is created, the Perl_ck_svconst() check function is called - this is
the same as when a standard CONST OP is created. If the SV is not
already marked as READONLY, the check function will try to set IsCOW
if it is safe to do so, then in either case will make sure that the
READONLY flag is set.
This commit therefore removes the SvREADONLY(sv) statement from
S_fold_constants(), allowing Perl_ck_svconst() to set the IsCOW
and READONLY flags itself. Minor test updates are also included.
ext/Devel-Peek/t/Peek.t | 4 ++--
op.c | 4 +++-
t/op/undef.t | 4 +++-
3 files changed, 8 insertions(+), 4 deletions(-)
bisect found first bad commit
@plicease , you may want to watch this issue.
Steps to Reproduce
cpan -i PLICEASE/UUID-FFI-0.11.tar.gz
Expected behavior
Should compile and test OK
Perl configuration
# perl -V output goes here
Summary of my perl5 (revision 5 version 41 subversion 1) configuration:
Commit id: e9f8aee2bc0fcdce0a13746848aad38c23073ef7
Platform:
osname=linux
osvers=6.5.0-35-generic
archname=x86_64-linux-thread-multi-ld
uname='linux k93jammy 6.5.0-35-generic #35~22.04.1-ubuntu smp preempt_dynamic tue may 7 09:00:52 utc 2 x86_64 x86_64 x86_64 gnulinux '
config_args='-Dprefix=/home/sand/src/perl/repoperls/installed-perls/host/k93jammy/v5.41.1/322f -Dmyhostname=k93jammy -Dinstallusrbinperl=n -Uversiononly -Dusedevel -des -Ui_db -Dlibswanted=cl pthread socket inet nsl gdbm dbm malloc dl ld sun m crypt sec util c cposix posix ucb BSD gdbm_compat -Duseithreads -Duselongdouble -DEBUGGING=-g'
hint=recommended
useposix=true
d_sigaction=define
useithreads=define
usemultiplicity=define
use64bitint=define
use64bitall=define
uselongdouble=define
usemymalloc=n
default_inc_excludes_dot=define
Compiler:
cc='cc'
ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
optimize='-O2 -g'
cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
ccversion=''
gccversion='11.4.0'
gccosandvers=''
intsize=4
longsize=8
ptrsize=8
doublesize=8
byteorder=12345678
doublekind=3
d_longlong=define
longlongsize=8
d_longdbl=define
longdblsize=16
longdblkind=3
ivtype='long'
ivsize=8
nvtype='long double'
nvsize=16
Off_t='off_t'
lseeksize=8
alignbytes=16
prototype=define
Linker and Libraries:
ld='cc'
ldflags =' -fstack-protector-strong -L/usr/local/lib'
libpth=/usr/local/lib /usr/lib/x86_64-linux-gnu /usr/lib /usr/lib64
libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
libc=/lib/x86_64-linux-gnu/libc.so.6
so=so
useshrplib=false
libperl=libperl.a
gnulibc_version='2.35'
Dynamic Linking:
dlsrc=dl_dlopen.xs
dlext=so
d_dlsymun=undef
ccdlflags='-Wl,-E'
cccdlflags='-fPIC'
lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector-strong'
Characteristics of this binary (from libperl):
Compile-time options:
HAS_LONG_DOUBLE
HAS_STRTOLD
HAS_TIMES
MULTIPLICITY
PERLIO_LAYERS
PERL_COPY_ON_WRITE
PERL_DONT_CREATE_GVSV
PERL_HASH_FUNC_SIPHASH13
PERL_HASH_USE_SBOX32
PERL_MALLOC_WRAP
PERL_OP_PARENT
PERL_PRESERVE_IVUV
PERL_USE_DEVEL
PERL_USE_SAFE_PUTENV
USE_64_BIT_ALL
USE_64_BIT_INT
USE_ITHREADS
USE_LARGE_FILES
USE_LOCALE
USE_LOCALE_COLLATE
USE_LOCALE_CTYPE
USE_LOCALE_NUMERIC
USE_LOCALE_TIME
USE_LONG_DOUBLE
USE_PERLIO
USE_PERL_ATOF
USE_REENTRANT_API
USE_THREAD_SAFE_LOCALE
Built under linux
Compiled at Jul 2 2024 19:27:33
%ENV:
PERL="/tmp/basesmoker-reloperl-Fbf2/bin/perl"
PERL5LIB="/tmp/loop_over_bdir-2248-KZJRDI/Sysync-0.38-0/blib/arch:/tmp/loop_over_bdir-2248-KZJRDI/Sysync-0.38-0/blib/lib"
PERL5OPT=""
PERL5_CPANPLUS_IS_RUNNING="2256"
PERL5_CPAN_IS_RUNNING="2256"
PERL_CANARY_STABILITY_NOPROMPT="1"
PERL_MM_USE_DEFAULT="1"
PERL_USE_UNSAFE_INC="1"
@INC:
/tmp/loop_over_bdir-2248-KZJRDI/Sysync-0.38-0/blib/arch
/tmp/loop_over_bdir-2248-KZJRDI/Sysync-0.38-0/blib/lib
/home/sand/src/perl/repoperls/installed-perls/host/k93jammy/v5.41.1/322f/lib/site_perl/5.41.1/x86_64-linux-thread-multi-ld
/home/sand/src/perl/repoperls/installed-perls/host/k93jammy/v5.41.1/322f/lib/site_perl/5.41.1
/home/sand/src/perl/repoperls/installed-perls/host/k93jammy/v5.41.1/322f/lib/5.41.1/x86_64-linux-thread-multi-ld
/home/sand/src/perl/repoperls/installed-perls/host/k93jammy/v5.41.1/322f/lib/5.41.1
.
From what I can tell this happens because we now do copy-on-write for more values.
In this case $data at https://metacpan.org/release/PLICEASE/UUID-FFI-0.11/source/lib/UUID/FFI.pm#L118
The tests that fail here sort the uuids, then call as_hex() on each uuid, as_hex() takes a pointer to the buffer of $data, but since that is now CoW it's the same buffer each time, so the map ends up overwriting that buffer as it works its way through the list, finally producing a list of all the same hex UUID.
I expect the real fix here is for FFI::Platypus to implement some sort of string buffer parameter type and un-CoW the buffer with SvPV_force().
Another approach that could be done by core would be to make pack "P" SvPV_force() the buffer, making P less[1] of a dangerous mess. This would slow down pack "P" a little, or increase memory usage (depending on the size of the PV for the SV), but it would make pack "P" safer, and perhaps more like what people expect.
[1] less, but still dangerous
I am happy to put the fix in either pack "P" or in FFI::Platypus. There is already a FFI::Platypus::Buffer module which does this same thing and the scalar_to_pointer function could be pretty easily reimplemented as XS if necessary, and UUID::FFI should tbh be using that instead of rolling its own unpack/pack. My intuition is that pack "P" would be better being safer even if it does cost a little more.
One problem with always doing a force normal/SvPV_force() is that it throws for a readonly SV, which may break existing code that uses pack "P" without needing a writeable pointer.
Another issue is if the SV has set magic, that set magic is never called when using the mechanism that UUID::FFI uses. I don't see any change that can be done for that in perl.
Always doing a SvPV_force() breaks tests, and would break at least one example in perlpacktut.pod
I just discovered another module affected by this change: MONS/Dash-Leak-0.06.tar.gz
Sample fail report: http://www.cpantesters.org/cpan/report/1dd7ca24-3f14-11ef-ab0f-0e74c8b2e081
Also affected: TMM/HEAT-Crypto-0.07.tar.gz http://www.cpantesters.org/cpan/report/fbd71cd8-3c50-11ef-86c6-156034470c55
Also affected: TMM/HEAT-Crypto-0.07.tar.gz http://www.cpantesters.org/cpan/report/fbd71cd8-3c50-11ef-86c6-156034470c55
That just needs a single SvTHINKFIRST to be fixed.
Also affected: NGLENN/Algorithm-AM-3.12.tar.gz http://www.cpantesters.org/cpan/report/90f70a84-3b21-11ef-897d-51c950bdeb56
@richardleach, the breakage being discussed in this ticket has been attributed to one of your commits in the last dev cycle.
commit 06e421c559c63975f29c35ba3588a0e6b0c75eca
Author: Richard Leach <[email protected]>
AuthorDate: Fri Dec 9 23:42:32 2022 +0000
Commit: Richard Leach <[email protected]>
CommitDate: Tue Jun 11 21:20:02 2024 +0100
S_fold_constants: remove early SvREADONLY(sv) to allow SvIsCOW(sv)
Would you care to comment? Thanks.
@richardleach, the breakage being discussed in this ticket has been attributed to one of your commits in the last dev cycle.
I've been away from my dev VM for the past ~10 days and won't be properly back at it for another 10. I can look into these at that point.
From what's been mentioned so far, it doesn't sound like a technical bug has been introduced into core, rather that the previous no-SvIsCOW behaviour has been relied upon. (Although I've not had time to sit down and get my head around the implications for pack, nor SvPV_force() breaking tests, as per @tonycoz's comments above.)
- UUID::FFI - see discussion higher up the ticket.
- HEAT::Crypto - @Leont mentions that the fix seems straightforward.
- Dash::Leak - at a quick glance, it looks like a constant-folded buffer of 'x's is repeatedly created and then deliberately leaked over 30 iterations. Previously, that presumably would have leaked 30 buffers of 'x's, but now leaks 1 (or maybe 0, I dunno yet). It should be possible to submit a PR rewriting that test.
- Algorithm::AM - not sure. At first glance, looks like
AM.xsisn't prepared for COWed buffers, so that's where I'd go looking.
Noted that there are quick 'n' dirty pure-perl ways to defeat COW in the UUID::FFI and Dash::Leak cases by lightly permuting the buffer to un-COW it. For example:
my $data = ("x" x 35)."w"; $data++;
That's obviously introducing an extra operation and obfuscating the code, so isn't ideal, but might help in some cases.
That's obviously introducing an extra operation and obfuscating the code, so isn't ideal, but might help in some cases.
Someone might come along and optimize those two steps into a constant assignment, it's not like $data is being passed anywhere that might make it magical.
I'll work on a patch to SvPV_force() non-READONLY SVs, but that's only safer, not safe.
That will fix the mechanism in the specific use by UUID::FFI, but it's not a general fix, since modification through that pointer doesn't call set magic either, so if a tied SV is passed in the changes will evaporate on the next fetch of the SV.
It won't help any of the other failures here.
nor SvPV_force() breaking tests, as per @tonycoz's comments above.
Some tests pass in constant strings which SvPV_force() reasonably throws on, so I'll make it only force for non-READONLY.
Thanks @tonycoz. I can work on patches for the other failures next week.
I've opened https://rt.cpan.org/Ticket/Display.html?id=154991 for Dash::Leak.
Also affected: TMM/HEAT-Crypto-0.07.tar.gz http://www.cpantesters.org/cpan/report/fbd71cd8-3c50-11ef-86c6-156034470c55
That just needs a single
SvTHINKFIRSTto be fixed.
I tried modifying *S_get_key_buffer() to do SV_CHECK_THINKFIRST(var); immediately prior to the BYTE* buff = (BYTE*) SvPV(var, len); line.
This looks to fix the 'decrypt shared' test failure. It does not fix the 'verify' failure. Will dig further.
Also affected: TMM/HEAT-Crypto-0.07.tar.gz http://www.cpantesters.org/cpan/report/fbd71cd8-3c50-11ef-86c6-156034470c55
That just needs a single
SvTHINKFIRSTto be fixed.
@Leont, could you open a bug ticket at https://github.com/tomushkin/heat-perl/issues about this? (I don't understand the problem well enough to do so myself.) Thanks.
I tried modifying S_get_key_buffer() to do SV_CHECK_THINKFIRST(var); immediately prior to the BYTE buff = (BYTE*) SvPV(var, len); line.
This looks to fix the 'decrypt shared' test failure. It does not fix the 'verify' failure. Will dig further.
Yeah, there is something weird going on there.
Interestingly it will also fail on older perls if you add the SV_CHECK_THINKFIRST, so I'm pretty sure the module is wrong.
Also affected: TMM/HEAT-Crypto-0.07.tar.gz http://www.cpantesters.org/cpan/report/fbd71cd8-3c50-11ef-86c6-156034470c55
That just needs a single
SvTHINKFIRSTto be fixed.@Leont, could you open a bug ticket at https://github.com/tomushkin/heat-perl/issues about this? (I don't understand the problem well enough to do so myself.) Thanks.
I managed to fix it. PR at https://github.com/tomushkin/heat-perl/pull/1
Also affected: NGLENN/Algorithm-AM-3.12.tar.gz http://www.cpantesters.org/cpan/report/90f70a84-3b21-11ef-897d-51c950bdeb56
I've got this module's tests passing locally, but mostly through keyboard mashing:
index 264ffbe..c4d7559 100644
--- a/Algorithm-AM-3.12/AM.xs
+++ b/Algorithm-AM-3.12_fixed_xs_maybe/AM.xs
@@ -1038,6 +1038,7 @@ _fillandcount(...)
tempsv = *hv_fetch(context_to_class, HeKEY(he), NUM_LATTICES * sizeof(AM_SHORT), 0);
this_class = (AM_SHORT) SvUVX(tempsv);
if (this_class) {
+SV_CHECK_THINKFIRST(sum[this_class]);
AM_LONG *s = (AM_LONG *) SvPVX(sum[this_class]);
for (i = 0; i < 7; ++i) {
*(s + i) += gangcount[i];
@@ -1048,6 +1049,7 @@ _fillandcount(...)
while (SvIOK(dataitem)) {
IV datanum = SvIVX(dataitem);
IV ocnum = SvIVX(classes[datanum]);
+SV_CHECK_THINKFIRST(sum[ocnum]);
AM_LONG *s = (AM_LONG *) SvPVX(sum[ocnum]);
for (i = 0; i < 7; ++i) {
*(s + i) += p[i];
I'll hopefully get some time at the weekend to consider if these are the appropriate places to un-cow and work up a PR.
garfieldnate/Algorithm-AM#63 now submitted.
A new CPAN version of Algorithm-AM has been released, per https://metacpan.org/dist/Algorithm-AM.
I have gotten PASSes versus perl-5.41.4 on Linux and FreeBSD. I tried to submit CPANtesters reports but the metabase is not receiving reports (see https://www.nntp.perl.org/group/perl.cpan.testers.discuss/2024/10/msg4648.html).
@richardleach, could you please review this issue and see whether (a) there's anything that still has to be fixed in blead; or (b) whether there still are upstream CPAN distros for which a patch has not been prepared? Thanks.
My understanding is that these two are fixed:
- UUID::FFI - Tony pushed https://github.com/Perl/perl5/pull/22407
- Algorithm::AM - patch supplied and a new version was released to CPAN.
Patches were supplied for these two, but they haven't been applied nor has there been any discussion on the tickets:
- HEAT::Crypto - Leon supplied https://github.com/tomushkin/heat-perl/pull/1
- Dash::Leak - I supplied patch
I am not aware of any further CPAN breakages nor any bugs in bleed related to this change.