perl5 icon indicating copy to clipboard operation
perl5 copied to clipboard

Clean up PL_strtab hash with NULL in perl_destruct

Open antirs opened this issue 6 months ago • 11 comments

As it's not required to pre-allocate PL_strtab hash before perl_construct() and PL_strtab is not initialized with NULL by default, SIGSEGV could happen in certain circumstances.

Particularly, when application links libperl and compiled with ASAN. PL_strtab remains unitialized as at start-up it contains garbage and initialization in perl_construct() is skipped because !PL_strtab check is skipped.

See also: c82f4881ef (Allow custom PL_strtab hash in perl_construct.)

Crash log may look like this:

ASAN_OPTIONS=report_globals=0:detect_leaks=1:handle_segv=1:handle_sigbus=1:handle_abort=1 ./fuzz -jobs=1 -runs=1 -seed=666 </dev/null
./fuzz -runs=1 -seed=666 >fuzz-0.log 2>&1
================== Job 0 exited with exit code 1 ============
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 666
INFO: A corpus is not provided, starting from an empty corpus
AddressSanitizer:DEADLYSIGNAL
=================================================================
==22328==ERROR: AddressSanitizer: SEGV on unknown address 0x000000081880 (pc 0x5599c1528155 bp 0x7ffe87a38d70 sp 0x7ffe87a38d00 T0)
==22328==The signal is caused by a READ memory access.
    #0 0x5599c1528155 in S_share_hek_flags /mnt/downloads/temp/_code/PERL/perl5/hv.c:3426:11
    #1 0x5599c15280ce in Perl_share_hek /mnt/downloads/temp/_code/PERL/perl5/hv.c:3399:12
    #2 0x5599c168db46 in Perl_newSVpvn_share /mnt/downloads/temp/_code/PERL/perl5/sv.c:9923:5
    #3 0x5599c14eca43 in S_init_main_stash /mnt/downloads/temp/_code/PERL/perl5/perl.c:4140:20
    #4 0x5599c14e88a0 in S_parse_body /mnt/downloads/temp/_code/PERL/perl5/perl.c:2223:5
    #5 0x5599c14e81b1 in perl_parse /mnt/downloads/temp/_code/PERL/perl5/perl.c:1932:9
...

  • This set of changes does not require a perldelta entry.

antirs avatar May 31 '25 19:05 antirs

Since your pull request is intended to correct a possible segfault, can you supply the output of perl -V for the perl build where you observe the problem? Thanks.

jkeenan avatar May 31 '25 22:05 jkeenan

The output of `perl -V`:
Summary of my perl5 (revision 5 version 41 subversion 14) configuration:
  Derived from: f9d1ecdb4e3a32ee002d062294dc02e4ce5fdab0
  Platform:
    osname=linux
    osvers=6.12.27-amd64
    archname=x86_64-linux
    uname='linux substation 6.12.27-amd64 #1 smp preempt_dynamic debian 6.12.27-1 (2025-05-06) x86_64 gnulinux '
    config_args='-des -Doptimize=-g -Dusedevel -DDEBUGGING=-g -Uusenm -Dprefix=/mnt/downloads/temp/_code/PERL/perl5/.install'
    hint=previous
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='cc'
    ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2'
    optimize='-g'
    cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='14.2.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='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 -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
    perllibs=-lpthread -ldl -lm -lcrypt -lutil -lc
    libc=/lib/x86_64-linux-gnu/libc.so.6
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version='2.41'
  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
    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_LARGE_FILES
    USE_LOCALE
    USE_LOCALE_COLLATE
    USE_LOCALE_CTYPE
    USE_LOCALE_NUMERIC
    USE_LOCALE_TIME
    USE_PERLIO
    USE_PERL_ATOF
  Locally applied patches:
    uncommitted-changes
  Built under linux
  Compiled at Jun  1 2025 08:54:50
  %ENV:
    PERL5LIB="/home/enomem/.perl5/lib/perl5:/home/enomem/.perl5/lib/perl5/x86_64-linux/auto:/home/enomem/.perl5/lib/perl5:/home/enomem/.perl5/lib/perl5/x86_64-linux/auto:/home/enomem/.perl5/lib/perl5:/home/enomem/.perl5/lib/perl5/x86_64-linux/auto"
    PERL_LOCAL_LIB_ROOT="/home/enomem/.perl5:/home/enomem/.perl5:/home/enomem/.perl5"
    PERL_MB_OPT="--install_base "/home/enomem/.perl5""
    PERL_MM_OPT="INSTALL_BASE=/home/enomem/.perl5"
  @INC:
    /home/enomem/.perl5/lib/perl5/x86_64-linux
    /home/enomem/.perl5/lib/perl5
    /home/enomem/.perl5/lib/perl5/x86_64-linux/auto
    /home/enomem/.perl5/lib/perl5/x86_64-linux
    /home/enomem/.perl5/lib/perl5
    /home/enomem/.perl5/lib/perl5/x86_64-linux/auto
    /home/enomem/.perl5/lib/perl5/x86_64-linux
    /home/enomem/.perl5/lib/perl5
    /home/enomem/.perl5/lib/perl5/x86_64-linux/auto
    /mnt/downloads/temp/_code/PERL/perl5/.install/lib/site_perl/5.41.14/x86_64-linux
    /mnt/downloads/temp/_code/PERL/perl5/.install/lib/site_perl/5.41.14
    /mnt/downloads/temp/_code/PERL/perl5/.install/lib/5.41.14/x86_64-linux
    /mnt/downloads/temp/_code/PERL/perl5/.install/lib/5.41.14

antirs avatar Jun 01 '25 06:06 antirs

@tonycoz @iabyn @Leont @leonerd or anyone else who understands this – can you opine? If possible we want to ship this in this cycle.

ap avatar Jun 06 '25 15:06 ap

For non-MULTIPLICITY builds PL_strtab is a statically allocated global, without an initializer it should be initialized to NULL by the C runtime/compiler/linker etc (ie. as required by the C language)

I suspect the real fix here is to zero PL_strtab when it is released in perl_destruct.

tonycoz avatar Jun 10 '25 00:06 tonycoz

Crash happens when perl is compiled with libFuzzer in a program like this:

fuzz.c
#include "EXTERN.h"
#include "perl.h"
#include "XSUB.h"

#include "ppport.h"

#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

static PerlInterpreter *my_perl;


int FuzzMe(const char *data, size_t size) {
    int exitstatus, i;
    char *cmd[] = {"", "-e 0", NULL};
    my_perl = perl_alloc();
    perl_construct(my_perl);
    PL_perl_destruct_level = 1;
    if (!perl_parse(my_perl, NULL, 2, cmd, (char **)NULL)) {
        perl_run(my_perl);
    }

    exitstatus = perl_destruct(my_perl);
    perl_free(my_perl);
    return 0;
}

extern int LLVMFuzzerTestOneInput(const char *data, size_t size) {
    FuzzMe(data, size);
    return 0;
}

Compiled with

compilation commands
clang -ggdb -I. -fwrapv -g -DDEBUGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2 -DDEBUGGING -g   -DVERSION=\"0.01\" -DXS_VERSION=\"0.01\" -fPIC -I/mnt/downloads/temp/_code/PERL/perl5/.install/lib/5.41.14/x86_64-linux/CORE   -c -o fuzz.o fuzz.c
clang -ggdb -I. -fwrapv -g -DDEBUGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2 -DDEBUGGING -g   -DVERSION=\"0.01\" -DXS_VERSION=\"0.01\" -fPIC -I/mnt/downloads/temp/_code/PERL/perl5/.install/lib/5.41.14/x86_64-linux/CORE -fsanitize=address,fuzzer -fprofile-instr-generate -fcoverage-mapping -o fuzz fuzz.o -L /mnt/downloads/temp/_code/PERL/perl5/ -lperl -lcrypt

When pre-initialized with NULL, there is no crash. Without ASAN it runs normally.

antirs avatar Jun 10 '25 09:06 antirs

If possible we want to ship this in this cycle.

Emphasis added and marking ticket "Release Blocker."

jkeenan avatar Jun 10 '25 22:06 jkeenan

When pre-initialized with NULL, there is no crash. Without ASAN it runs normally.

Yes, you're running the interpreter multiple times.

The first time around PL_strtab is NULL, so perl_construct() creates a HV, and then perl_destruct() frees it, but doesn't clear the global PL_strtab variable.

The second time around PL_strtab points at the now freed HV, and so isn't initialized to point to a new HV, and the interpreter proceeds to use the bad pointer and dies horribly.

If I test with code like:

int RunIt(const char *data, size_t size) {
    int exitstatus, i;
    char *cmd[] = {"", "-e 0", NULL};
    my_perl = perl_alloc();
    perl_construct(my_perl);
    PL_perl_destruct_level = 1;
    if (!perl_parse(my_perl, NULL, 2, cmd, (char **)NULL)) {
        perl_run(my_perl);
    }

    exitstatus = perl_destruct(my_perl);
    perl_free(my_perl);
    return 0;
}

int main(int argc, char **argv) {
    RunIt("one", 3);
    RunIt("two", 3);
}

I get the same type of crash you did without ASAN or a fuzzer:

==206276== Invalid read of size 8
==206276==    at 0x1B7110: S_share_hek_flags (hv.c:3407)
==206276==    by 0x1B70D3: Perl_share_hek (hv.c:3399)
==206276==    by 0x42AD9A: Perl_newSVpvn_share (sv.c:9923)
==206276==    by 0x14D2E0: S_init_main_stash (perl.c:4140)
==206276==    by 0x145B3C: S_parse_body (perl.c:2223)
==206276==    by 0x145075: perl_parse (perl.c:1932)
==206276==    by 0x13F0AF: RunIt (embedtwice.c:20)
==206276==    by 0x13F135: main (embedtwice.c:31)
==206276==  Address 0x4b783c8 is 1,512 bytes inside a block of size 4,080 free'd
==206276==    at 0x484417B: free (vg_replace_malloc.c:872)
==206276==    by 0x4A4C15: Perl_safesysfree (util.c:433)
==206276==    by 0x3E7115: Perl_sv_free_arenas (sv.c:713)
==206276==    by 0x143F33: perl_destruct (perl.c:1533)
==206276==    by 0x13F0D1: RunIt (embedtwice.c:24)
==206276==    by 0x13F121: main (embedtwice.c:30)
==206276==  Block was alloc'd at
==206276==    at 0x48417B4: malloc (vg_replace_malloc.c:381)
==206276==    by 0x4A48B9: Perl_safesysmalloc (util.c:176)
==206276==    by 0x3E51F8: Perl_more_sv (sv.c:277)
==206276==    by 0x3E292E: Perl_new_sv (sv_inline.h:85)
==206276==    by 0x3E2A19: Perl_newSV_type (sv_inline.h:378)
==206276==    by 0x42A3B7: Perl_newSVpvn (sv.c:9792)
==206276==    by 0x141C4D: perl_construct (perl.c:261)
==206276==    by 0x13F082: RunIt (embedtwice.c:18)
==206276==    by 0x13F121: main (embedtwice.c:30)

(I get a Segmentation fault without valgrind too.)

If I clear PL_strtab after releasing it in perl_destruct() it no longer crashes.

Adding an initializer to the strtab as you did means that if the caller sets PL_strtab to a custom HV as licensed by c82f488, PL_strtab will be cleared by S_init_interp() before the later code in perl_construct() gets a chance to use it. This will also leak the custom HV.

tonycoz avatar Jun 11 '25 04:06 tonycoz

It could be we don't want to support pre-setting PL_strtab as c82f488 enables, if we want that we can just revert c82f488 and PL_strtab will be set unconditionally.

With this PR the if (!PL_strtab) { is always true.

tonycoz avatar Jun 11 '25 05:06 tonycoz

Finally, I've got it. Now commit message is unrelated, as initial fix in PR was about initializing PL_strtab, but now it's for cleaning.

antirs avatar Jun 11 '25 06:06 antirs

This has been around for much longer than just this release. That doesn’t meant we wouldn’t very much like to ship a fix in this release… once it becomes clear what the right fix is.

ap avatar Jun 12 '25 15:06 ap

I think the current change is ok, though using github to merge blead (I assume from the email address and history) broke the authors porting test.

@antirs, please rebase on blead and squash your commits, this should remove the merge and make it ready to apply.

tonycoz avatar Jun 12 '25 22:06 tonycoz