Thread race in dist/IO/IO.xs

[p5pRT]

Migrated from rt.perl.org#125685 (status was 'open')

Searchable as RT125685$

[p5pRT]

From william@25thandClement.com

Created by william@25thandClement.com

This is a bug report for perl from william@​25thandClement.com,
generated with the help of perlbug 1.40 running under perl 5.20.1.

-----------------------------------------------------------------
The _create_getline_subs routine in dist/IO/IO.xs manipulates PL_check in a
thread-unsafe manner. It's called when loading IO​::Handle, at line 437 of
dist/IO/lib/IO/Handle.pm.

In our case this manifested as Perl interpreters in other threads randomly
throwing warnings about undefined constants. This appears to be a regression
starting with 5.16 and still exists in 5.22 and 5.23.1. Apache mod_perl
seems to be immune because they clone their interpreters, and presumably
IO​::Handle is always loaded into the original.

Our workaround was to preload IO​::Handle when initializing interpreters
while holding a global writer lock, and use a reader lock when executing
Perl code.

I'm not sure what a proper fix is. Simply using wrap_op_checker, or
otherwise taking a mutex from _create_getline_subs, is insufficient because
the interposition is intended to be temporary. I don't understand what the
function is trying to achieve, either.

Perl Info

Flags:
    category=library
    severity=high
    module=IO::Handle

Site configuration information for perl 5.20.1:

Configured by root at Thu Jan  1  0:00:00 UTC 1970.

Summary of my perl5 (revision 5 version 20 subversion 1) configuration:
   
  Platform:
    osname=openbsd, osvers=5.7, archname=amd64-openbsd
    uname='openbsd'
    config_args='-dsE -Dopenbsd_distribution=defined -Dccflags=-DNO_LOCALE_NUMERIC -DNO_LOCALE_COLLATE -Dmksymlinks'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-DNO_LOCALE_NUMERIC -DNO_LOCALE_COLLATE -fno-strict-aliasing -fno-delete-null-pointer-checks -pipe -fstack-protector -I/usr/local/include',
    optimize='-O2',
    cppflags='-DNO_LOCALE_NUMERIC -DNO_LOCALE_COLLATE -fno-strict-aliasing -fno-delete-null-pointer-checks -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.2.1 20070719 ', gccosandvers='openbsd5.7'
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags ='-Wl,-E  -fstack-protector -L/usr/local/lib'
    libpth=/usr/lib /usr/lib
    libs=-lm -lutil -lc
    perllibs=-lm -lutil -lc
    libc=/usr/lib/libc.a, so=so, useshrplib=true, libperl=libperl.so.17.0
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-R/usr/libdata/perl5/amd64-openbsd/5.20.1/CORE'
    cccdlflags='-DPIC -fPIC ', lddlflags='-shared -fPIC  -fstack-protector -L/usr/local/lib'



@INC for perl 5.20.1:
    /usr/local/libdata/perl5/site_perl/amd64-openbsd
    /usr/libdata/perl5/site_perl/amd64-openbsd
    /usr/local/libdata/perl5/site_perl
    /usr/libdata/perl5/site_perl
    /usr/libdata/perl5/amd64-openbsd/5.20.1
    /usr/local/libdata/perl5/amd64-openbsd/5.20.1
    /usr/libdata/perl5
    /usr/local/libdata/perl5
    .


Environment for perl 5.20.1:
    HOME=/home/william
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin:/usr/games:.
    PERL_BADLANG (unset)
    SHELL=/bin/ksh

[p5pRT]

From @jkeenan

On Fri, 24 Jul 2015 23​:11​:32 GMT, william@​25thandClement.com wrote​:

This is a bug report for perl from william@​25thandClement.com,
generated with the help of perlbug 1.40 running under perl 5.20.1.

-----------------------------------------------------------------
The _create_getline_subs routine in dist/IO/IO.xs manipulates PL_check
in a
thread-unsafe manner. It's called when loading IO​::Handle, at line 437
of
dist/IO/lib/IO/Handle.pm.

In our case this manifested as Perl interpreters in other threads
randomly
throwing warnings about undefined constants. This appears to be a
regression
starting with 5.16 and still exists in 5.22 and 5.23.1. Apache
mod_perl
seems to be immune because they clone their interpreters, and
presumably
IO​::Handle is always loaded into the original.

Our workaround was to preload IO​::Handle when initializing
interpreters
while holding a global writer lock, and use a reader lock when
executing
Perl code.

I'm not sure what a proper fix is. Simply using wrap_op_checker, or
otherwise taking a mutex from _create_getline_subs, is insufficient
because
the interposition is intended to be temporary. I don't understand what
the
function is trying to achieve, either.

1. Would it be possible to supply a small program demonstrating this problem?

2. Given the line you're pointing to in the source code and the point at which the problem first emerged, I suspect the problem began with​:

#####
commit 986a805
Author​: Father Chrysostomos <sprout@​cpan.org>
Date​: Sat Sep 17 12​:09​:22 2011 -0700

  Make IO​::Handle​::getline(s) respect the open pragma
...
#####

But we'd need a reproducible failure to investigate further.

3. Can you supply the output of 'perl -V' from the system where you observed this problem? (The one supplied with the original post came from a non-threaded perl, which I suspect was not where the problem occured.)

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @dur-randir

On Thu, 05 Jan 2017 12​:24​:26 -0800, jkeenan wrote​:

1. Would it be possible to supply a small program demonstrating this
problem?
use threads;

while (1) {
  push @​foo, threads->create(sub {
  require IO​::Handle;
  });

  $_->detach for(splice @​foo);
}

This should output nothing, but is really noisy instead.

[p5pRT]

From @jkeenan

On Thu, 05 Jan 2017 21​:23​:50 GMT, randir wrote​:

On Thu, 05 Jan 2017 12​:24​:26 -0800, jkeenan wrote​:

1. Would it be possible to supply a small program demonstrating this
problem?
use threads;

while (1) {
push @​foo, threads->create(sub {
require IO​::Handle;
});

$\_\->detach for\(splice @​foo\);

}

This should output nothing, but is really noisy instead.

Thanks. I can confirm that 986a805 is the problematic commit.

#####
$ ./perl -v

This is perl 5, version 15, subversion 2 (v5.15.2-464-g7d167fe) built for x86_64-linux-thread-multi

$ ./perl -Ilib -c ~/learn/perl/p5p/125685-threads.pl
/home/jkeenan/learn/perl/p5p/125685-threads.pl syntax OK

$ ./perl -Ilib ~/learn/perl/p5p/125685-threads.pl
[# no output]
^C

#####
$ ./perl -v

This is perl 5, version 15, subversion 2 (v5.15.2-465-g986a805) built for x86_64-linux-thread-multi

$ ./perl -Ilib -c ~/learn/perl/p5p/125685-threads.pl
/home/jkeenan/learn/perl/p5p/125685-threads.pl syntax OK

$ ./perl -Ilib ~/learn/perl/p5p/125685-threads.pl
[# very noisy output]
^C
#####

Father C​: any thoughts?

--
James E Keenan (jkeenan@​cpan.org)

[p5pRT]

From @hvds

On Thu, 05 Jan 2017 14​:22​:30 -0800, jkeenan wrote​:

I can confirm that 986a805
is the problematic commit.

I took a quick look through, and the manipulation of PL_check[] looked thread-unsafe; checking a bit further I find in perlvars.h​:
  For thread safety, modules should not write directly to this array.
  Instead, use the function L</wrap_op_checker>.

I don't see mention of a function to remove the inserted checker again though, I guess that's rather harder to do safely.

Hugo

[p5pRT]

From @niner

Hi,
When embedding perl in a multi threaded program, loading IO​::Handle
concurrently in multiple threads (each with their own interpreter) will cause
compilation errors in other threads and in unrelated modules. The errors all
look like missing symbols that should be there, like undefined subroutines or
methods or imported subs not there causing errors like this​:
String found where operator expected at /usr/lib/perl5/5.28.1/SelectSaver.pm
line 42, near "croak 'usage​: SelectSaver->new( [FILEHANDLE] )'" (Do you need
to predeclare croak?) syntax error at /usr/lib/perl5/5.28.1/SelectSaver.pm
line 42, near "croak 'usage​: SelectSaver->new( [FILEHANDLE] )'"

The errors can be reproduced using the attached C program which I compiled
with​: gcc -Wall testthreads.c -Wl,-E -Wl,-rpath,/usr/lib/perl5/5.28.1/x86_64-
linux-thread-multi/CORE -L/usr/local/lib64 -fstack-protector-strong -L/
usr/lib/perl5/5.28.1/x86_64-linux-thread-multi/CORE -lperl -lm -ldl -lcrypt -
lpthread -D_REENTRANT -D_GNU_SOURCE -DPERL_USE_SAFE_PUTENV -fwrapv -fno-
strict-aliasing -pipe -fstack-protector-strong -D_LARGEFILE_SOURCE -
D_FILE_OFFSET_BITS=64 -I/usr/lib/perl5/5.28.1/x86_64-linux-thread-multi/CORE
-o testthreads -fPIC -g testthreads.c
--
Stefan

[p5pRT]

From @niner

#include <EXTERN.h>
#include <perl.h>
#include <XSUB.h>
#include <pthread.h>
#include <unistd.h>

static void xs_init (pTHX);
EXTERN_C void boot_DynaLoader (pTHX_ CV* cv);
EXTERN_C void boot_Socket (pTHX_ CV* cv);

EXTERN_C void xs_init(pTHX) {
    char *file = __FILE__;
    newXS("DynaLoader::boot_DynaLoader", boot_DynaLoader, file);
}

PerlInterpreter *p5_init_perl(int argc, char *argv[]) {
    PerlInterpreter *my_perl = perl_alloc();
    PERL_SET_CONTEXT(my_perl);
    PL_perl_destruct_level = 1;
    perl_construct( my_perl );
    perl_parse(my_perl, xs_init, argc, argv, NULL);
    PL_exit_flags |= PERL_EXIT_DESTRUCT_END;
    perl_run(my_perl);

    return my_perl;
}

void p5_destruct_perl(PerlInterpreter *my_perl) {
    PERL_SET_CONTEXT(my_perl);
    PL_perl_destruct_level = 1;

    POPSTACK_TO(PL_mainstack);
    dounwind(-1);
    LEAVE_SCOPE(0);

    perl_destruct(my_perl);
    perl_free(my_perl);
}

void p5_eval_pv(PerlInterpreter *my_perl, const char* p, I32 croak_on_error) {
    PERL_SET_CONTEXT(my_perl);
    {
        dSP;
        ENTER;
        SAVETMPS;
        PUSHMARK(SP);

        eval_pv(p, croak_on_error);

        SPAGAIN;
        PUTBACK;
        FREETMPS;
        LEAVE;
    }
}

typedef struct {
    int argc;
    char **argv;
} args;

void *thread_code(void *data) {
    PerlInterpreter *my_perl = p5_init_perl(((args*)data)->argc, ((args*)data)->argv);
    p5_eval_pv(my_perl, "use IO::Handle; 1", 1);
    p5_destruct_perl(my_perl);
    return NULL;
}

#define thread_count 60

int main(int argc, char *argv[]) {
    args a = {argc, argv};
    PERL_SYS_INIT(&argc, &argv);

    pthread_t thread_id[thread_count];

    for(int i = 0; i < thread_count; i++) {
        int err = pthread_create(&thread_id[i], NULL, thread_code, &a);
        if (err != 0) abort();
    }

    for(int i = 0; i < thread_count; i++) {
        void *result;
        int err = pthread_join(thread_id[i], &result);
        if (err != 0) abort();
    }

    PERL_SYS_TERM();

    return 0;
}

[p5pRT]

From @niner

Removing this code from IO/Handle.pm makes the issue go away​:

# Special XS wrapper to make them inherit lexical hints from the caller.
_create_getline_subs( <<'END' ) or die $@​;
sub getline {
  @​_ == 1 or croak 'usage​: $io->getline()';
  my $this = shift;
  return scalar <$this>;
}

sub getlines {
  @​_ == 1 or croak 'usage​: $io->getlines()';
  wantarray or
  croak 'Can\'t call $io->getlines in a scalar context, use $io->getline';
  my $this = shift;
  return <$this>;
}
1; # return true for error checking
END

*gets = \&getline; # deprecated

[p5pRT]

From [Unknown Contact. See original ticket]

Removing this code from IO/Handle.pm makes the issue go away​:

# Special XS wrapper to make them inherit lexical hints from the caller.
_create_getline_subs( <<'END' ) or die $@​;
sub getline {
  @​_ == 1 or croak 'usage​: $io->getline()';
  my $this = shift;
  return scalar <$this>;
}

sub getlines {
  @​_ == 1 or croak 'usage​: $io->getlines()';
  wantarray or
  croak 'Can\'t call $io->getlines in a scalar context, use $io->getline';
  my $this = shift;
  return <$this>;
}
1; # return true for error checking
END

*gets = \&getline; # deprecated

[p5pRT]

From @niner

The issue is actually a regression caused by https://perl5.git.perl.org/perl.git/commit/986a805c4b2

_create_getline_subs is not thread safe as it assigns to PL_check directly​:
PL_check[OP_LINESEQ] = io_ck_lineseq;
https://perl5.git.perl.org/perl.git/blob/HEAD:/dist/IO/IO.xs#l565

According to perlapi(1) "PL_check" is global to an entire process and must not be written to directly. Instead "wrap_op_checker" should be used.

But still, since PL_check is global that would affect concurrent compilation and I don't think io_ck_lineseq is prepared for that. It looks like it's really tailored for compilation of getline and getlines as it does not check in any way if it should apply its modifications to the OP tree.

[p5pRT]

From [Unknown Contact. See original ticket]

The issue is actually a regression caused by https://perl5.git.perl.org/perl.git/commit/986a805c4b2

_create_getline_subs is not thread safe as it assigns to PL_check directly​:
PL_check[OP_LINESEQ] = io_ck_lineseq;
https://perl5.git.perl.org/perl.git/blob/HEAD:/dist/IO/IO.xs#l565

According to perlapi(1) "PL_check" is global to an entire process and must not be written to directly. Instead "wrap_op_checker" should be used.

But still, since PL_check is global that would affect concurrent compilation and I don't think io_ck_lineseq is prepared for that. It looks like it's really tailored for compilation of getline and getlines as it does not check in any way if it should apply its modifications to the OP tree.

[p5pRT]

From @jkeenan

On Fri, 20 Sep 2019 16​:09​:16 GMT, ss@​atikon.com wrote​:

Hi,
When embedding perl in a multi threaded program, loading IO​::Handle
concurrently in multiple threads (each with their own interpreter)
will cause
compilation errors in other threads and in unrelated modules. The
errors all
look like missing symbols that should be there, like undefined
subroutines or
methods or imported subs not there causing errors like this​:
String found where operator expected at
/usr/lib/perl5/5.28.1/SelectSaver.pm
line 42, near "croak 'usage​: SelectSaver->new( [FILEHANDLE] )'" (Do
you need
to predeclare croak?) syntax error at
/usr/lib/perl5/5.28.1/SelectSaver.pm
line 42, near "croak 'usage​: SelectSaver->new( [FILEHANDLE] )'"

The errors can be reproduced using the attached C program which I
compiled
with​: gcc -Wall testthreads.c -Wl,-E -Wl,-
rpath,/usr/lib/perl5/5.28.1/x86_64-
linux-thread-multi/CORE -L/usr/local/lib64 -fstack-protector-strong
-L/
usr/lib/perl5/5.28.1/x86_64-linux-thread-multi/CORE -lperl -lm -ldl
-lcrypt -
lpthread -D_REENTRANT -D_GNU_SOURCE -DPERL_USE_SAFE_PUTENV -fwrapv
-fno-
strict-aliasing -pipe -fstack-protector-strong -D_LARGEFILE_SOURCE -
D_FILE_OFFSET_BITS=64 -I/usr/lib/perl5/5.28.1/x86_64-linux-thread-
multi/CORE
-o testthreads -fPIC -g testthreads.c

I built a threaded perl at blead and then tried to compile your test program. The compilation failed.

#####
$ gcc -Wall 134444-testthreads.c -Wl,-E -Wl,-rpath,/home/jkeenan/testing/threaded_blead/lib/perl5/5.31.5/x86_64-linux-thread-multi/CORE \

-L/usr/local/lib64 -fstack-protector-strong \
-L/home/jkeenan/testing/threaded_blead/lib/perl5/5.31.5/x86_64-linux-thread-multi/CORE -lperl -lm -ldl \
-lcrypt -lpthread -D_REENTRANT -D_GNU_SOURCE -DPERL_USE_SAFE_PUTENV -fwrapv -fno-strict-aliasing \
-pipe -fstack-protector-strong -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 \
-I/home/jkeenan/testing/threaded_blead/lib/perl5/5.31.5/x86_64-linux-thread-multi/CORE \
-o testthreads -fPIC -g 134444-testthreads.c
/tmp/ccCfdXuT.o​: In function `p5_init_perl'​:
/home/jkeenan/learn/perl/p5p/134444-testthreads.c​:16​: multiple definition of `p5_init_perl'
/tmp/ccQrNdU4.o​:/home/jkeenan/learn/perl/p5p/134444-testthreads.c​:16​: first defined here
/tmp/ccCfdXuT.o​: In function `p5_destruct_perl'​:
/home/jkeenan/learn/perl/p5p/134444-testthreads.c​:28​: multiple definition of `p5_destruct_perl'
/tmp/ccQrNdU4.o​:/home/jkeenan/learn/perl/p5p/134444-testthreads.c​:28​: first defined here
/tmp/ccCfdXuT.o​: In function `p5_eval_pv'​:
/home/jkeenan/learn/perl/p5p/134444-testthreads.c​:40​: multiple definition of `p5_eval_pv'
/tmp/ccQrNdU4.o​:/home/jkeenan/learn/perl/p5p/134444-testthreads.c​:40​: first defined here
/tmp/ccCfdXuT.o​: In function `thread_code'​:
/home/jkeenan/learn/perl/p5p/134444-testthreads.c​:62​: multiple definition of `thread_code'
/tmp/ccQrNdU4.o​:/home/jkeenan/learn/perl/p5p/134444-testthreads.c​:62​: first defined here
/tmp/ccCfdXuT.o​: In function `main'​:
/home/jkeenan/learn/perl/p5p/134444-testthreads.c​:71​: multiple definition of `main'
/tmp/ccQrNdU4.o​:/home/jkeenan/learn/perl/p5p/134444-testthreads.c​:71​: first defined here
collect2​: error​: ld returned 1 exit status
#####

Can you spot the problem?

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @niner

On Sun, 22 Sep 2019 08​:42​:00 -0700, jkeenan wrote​:

$ gcc -Wall 134444-testthreads.c -Wl,-E -Wl,-
...
-o testthreads -fPIC -g 134444-testthreads.c

Can you spot the problem?

Indeed! You gave it the name of the source file twice.

I have since written a patch that seems to fix my problem. It moves PL_check to the interpreter struct which should avoid any threading related issues altogether. It's attached.