search

Perl / perl5

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

Hash::Util test will break Test::More if it fails #20929

Open demerphq opened 1 year ago

demerphq commented 1 year ago

Module Hash::Util aka ext/Hash-Util/lib/Hash/Util.pm Test::More aka cpan/Test-Simple/lib/Test2/Formatter/TAP.pm (I have filed https://github.com/Test-More/test-more/issues/907 for this part of the bug)

Description ext/Hash-Util/t/Util.t contains the following code:

lock_keys(%ENV);
eval { () = $ENV{I_DONT_EXIST} };
like(
    $@,
    qr/^Attempt to access disallowed key 'I_DONT_EXIST' in a restricted hash/,
    'locked %ENV'
);
unlock_keys(%ENV); # Test::Builder cannot print test failures otherwise

If this test fails then it will break Test::More (it says Test::Builder, but i think that is wrong these days) with a message like this:

Attempt to access disallowed key 'HARNESS_ACTIVE' in a restricted hash at lib/Test2/Formatter/TAP.pm line 121.
A context appears to have been destroyed without first calling release().
Based on $@ it does not look like an exception was thrown (this is not always
a reliable test)

This is a problem because the global error variables ($!, $@, and $?) will
not be restored. In addition some release callbacks will not work properly from
inside a DESTROY method.

Here are the context creation details, just in case a tool forgot to call
release():
  File: ext/Hash-Util/t/Util.t
  Line: 204
  Tool: Test::More::like

It is debatable whether this is a failure in Test::More or Hash::Util. Arguably both. I noticed this while working on https://github.com/Perl/perl5/pull/20928 and i solved it by replacing the code with the following:

{
    # Note we can't call like() while %ENV is locked, or we will get an explosion
    # because of: Attempt to access unknown key 'HARNESS_ACTIVE' in a restricted hash
    #             at lib/Test2/Formatter/TAP.pm line 121.
    # So we copy the error, and then do the like check on the copy.
    lock_keys(%ENV);
    eval { () = $ENV{I_DONT_EXIST} };
    my $error = $@;
    unlock_keys(%ENV);

    like(
        $error,
        qr/^Attempt to access unknown key 'I_DONT_EXIST' in a restricted hash/,
        'locked %ENV'
    );
}

However this includes a change to the error message from locked hashes, so it can't be applied directly.

I will also file a bug with Test::More as this one is a bit debatable. Is expecting Test::More to deal with a locked %ENV reasonable? Should Test::More copy %ENV super early before any code can lock it? Should Hash::Util be changed as I did in my PR?

Steps to Reproduce

$ perl -MTest::More -MHash::Util=lock_hash -e'lock_hash(%ENV); is(1,2)'
Attempt to access disallowed key 'HARNESS_ACTIVE' in a restricted hash at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121.
A context appears to have been destroyed without first calling release().
Based on $@ it does not look like an exception was thrown (this is not always
a reliable test)

This is a problem because the global error variables ($!, $@, and $?) will
not be restored. In addition some release callbacks will not work properly from
inside a DESTROY method.

Here are the context creation details, just in case a tool forgot to call
release():
  File: -e
  Line: 1
  Tool: Test::More::is

Here is a trace to the code that caused the context to be destroyed, this could
be an exit(), a goto, or simply the end of a scope:
Context destroyed at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121.
    eval {...} called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121
    Test::Builder::ok(Test::Builder=HASH(0x561e206dbac8), "", undef) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/Builder.pm line 982
    Test::Builder::cmp_ok(Test::Builder=HASH(0x561e206dbac8), 1, "eq", 2, undef) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/Builder.pm line 820
    Test::Builder::is_eq(Test::Builder=HASH(0x561e206dbac8), 1, 2) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/More.pm line 405
    Test::More::is(1, 2) called at -e line 1

Cleaning up the CONTEXT stack...
A context appears to have been destroyed without first calling release().
Based on $@ it does not look like an exception was thrown (this is not always
a reliable test)

This is a problem because the global error variables ($!, $@, and $?) will
not be restored. In addition some release callbacks will not work properly from
inside a DESTROY method.

Here are the context creation details, just in case a tool forgot to call
release():
  File: -e
  Line: 1
  Tool: Test::More::is

Here is a trace to the code that caused the context to be destroyed, this could
be an exit(), a goto, or simply the end of a scope:
Context destroyed at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121.
    eval {...} called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121
    Test::Builder::cmp_ok(Test::Builder=HASH(0x561e206dbac8), 1, "eq", 2, undef) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/Builder.pm line 820
    Test::Builder::is_eq(Test::Builder=HASH(0x561e206dbac8), 1, 2) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/More.pm line 405
    Test::More::is(1, 2) called at -e line 1

Cleaning up the CONTEXT stack...
A context appears to have been destroyed without first calling release().
Based on $@ it does not look like an exception was thrown (this is not always
a reliable test)

This is a problem because the global error variables ($!, $@, and $?) will
not be restored. In addition some release callbacks will not work properly from
inside a DESTROY method.

Here are the context creation details, just in case a tool forgot to call
release():
  File: -e
  Line: 1
  Tool: Test::More::is

Here is a trace to the code that caused the context to be destroyed, this could
be an exit(), a goto, or simply the end of a scope:
Context destroyed at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121.
    eval {...} called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121
    Test::Builder::is_eq(Test::Builder=HASH(0x561e206dbac8), 1, 2) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/More.pm line 405
    Test::More::is(1, 2) called at -e line 1

Cleaning up the CONTEXT stack...
Attempt to access disallowed key 'HARNESS_ACTIVE' in a restricted hash at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121.
END failed--call queue aborted.

Expected behavior The test failing shouldn't completely break Test::More. :-)

Perl configuration This isnt really relevant, but

./perl -Ilib -V
Summary of my perl5 (revision 5 version 37 subversion 10) configuration:
  Commit id: 351ceb16a6ce2fb54d5a88bd12017b9e4f617466
  Platform:
    osname=linux
    osvers=5.14.0-1058-oem
    archname=x86_64-linux-thread-multi
    uname='linux oncidium 5.14.0-1058-oem #66-ubuntu smp fri feb 10 09:46:18 utc 2023 x86_64 x86_64 x86_64 gnulinux '
    config_args='-Dusethreads -Doptimize=-g -d -Dusedevel -Dcc=ccache gcc -Dld=gcc -DDEBUGGING'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=define
    usemultiplicity=define
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='gcc'
    ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
    optimize='-g'
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='9.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='double'
    nvsize=8
    Off_t='off_t'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='gcc'
    ldflags =' -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib/x86_64-linux-gnu /usr/lib /usr/lib64
    libs=-lpthread -ldl -lm -lcrypt -lutil -lc
    perllibs=-lpthread -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.31.so
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version='2.31'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-E'
    cccdlflags='-fPIC'
    lddlflags='-shared -g -L/usr/local/lib -fstack-protector-strong'

Characteristics of this binary (from libperl): 
  Compile-time options:
    DEBUGGING
    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_TRACK_MEMPOOL
    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_PERLIO
    USE_PERL_ATOF
    USE_REENTRANT_API
    USE_THREAD_SAFE_LOCALE
  Built under linux
  Compiled at Mar 13 2023 11:34:50
  %ENV:
    PERLBREW_CONFIGURE_FLAGS="-de -Dcc=ccache\ gcc -Dld=gcc"
    PERLBREW_HOME="/home/yorton/.perlbrew"
    PERLBREW_MANPATH="/home/yorton/perl5/perlbrew/perls/perl-5.34.1/man"
    PERLBREW_PATH="/home/yorton/perl5/perlbrew/bin:/home/yorton/perl5/perlbrew/perls/perl-5.34.1/bin"
    PERLBREW_PERL="perl-5.34.1"
    PERLBREW_ROOT="/home/yorton/perl5/perlbrew"
    PERLBREW_SHELLRC_VERSION="0.88"
    PERLBREW_VERSION="0.88"
  @INC:
    lib
    /usr/local/lib/perl5/site_perl/5.37.10/x86_64-linux-thread-multi
    /usr/local/lib/perl5/site_perl/5.37.10
    /usr/local/lib/perl5/5.37.10/x86_64-linux-thread-multi
    /usr/local/lib/perl5/5.37.10
perl-enjoyer commented 1 year ago

Is it important that the lock_keys test operate on %ENV? Could it pick a hash with fewer consequences?

demerphq commented 1 year ago

Is it important that the lock_keys test operate on %ENV?

Yes it is important that we test that lock_keys can operate on %ENV. It is a magic hash, and people might want to lock it.

Could it pick a hash with fewer consequences?

Most of our tests are against "normal hashes", but that doesn't tell us that the locking mechanism works on a magic hash like %ENV.