search

Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.9k stars 540 forks source link

BBC: 5.41.1 breaks PLICEASE/UUID-FFI-0.11.tar.gz #22380

Open andk opened 2 months ago

andk commented 2 months ago

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 <richardleach@users.noreply.github.com>
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
    .
tonycoz commented 2 months ago

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

plicease commented 2 months ago

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.

tonycoz commented 2 months ago

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.

tonycoz commented 2 months ago

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.

tonycoz commented 2 months ago

Always doing a SvPV_force() breaks tests, and would break at least one example in perlpacktut.pod

andk commented 2 months ago

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

andk commented 2 months ago

Also affected: TMM/HEAT-Crypto-0.07.tar.gz http://www.cpantesters.org/cpan/report/fbd71cd8-3c50-11ef-86c6-156034470c55

Leont commented 2 months ago

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.

andk commented 2 months ago

Also affected: NGLENN/Algorithm-AM-3.12.tar.gz http://www.cpantesters.org/cpan/report/90f70a84-3b21-11ef-897d-51c950bdeb56

jkeenan commented 2 months ago

@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 <rich+perl@hyphen-dash-hyphen.info>
AuthorDate: Fri Dec 9 23:42:32 2022 +0000
Commit:     Richard Leach <rich+perl@hyphen-dash-hyphen.info>
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 commented 2 months ago

@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.)

richardleach commented 2 months ago

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.

tonycoz commented 1 month ago

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.

richardleach commented 1 month ago

Thanks @tonycoz. I can work on patches for the other failures next week.

richardleach commented 3 weeks ago

I've opened https://rt.cpan.org/Ticket/Display.html?id=154991 for Dash::Leak.

richardleach commented 3 weeks ago

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.

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.