Feature 'class' Segmentation fault in DESTROY phase for improperly constructed instance

[jbarrett]

Module: core feature 'class'

Description

Referencing a required :param class member in DESTROY, which had not been correctly initialised during instantiation, triggered a Segmentation fault.

FAO @leonerd, @ilmari - following up on IRC conversation. Many thanks!

Steps to Reproduce

This script is the most whittled-down example I could produce which exhibits the behaviour:

#!/usr/bin/env perl

use v5.40;
use experimental qw/ class /;

class Foo {

    field $dependency :param :reader;

    DESTROY {
        shift->dependency->cleanup;
    }
}

# Ooops, forgot to supply the dependency
Foo->new;

Expected behavior
Additional error:

• Can't call method "cleanup" on an undefined value

Perl configuration

$ perl -V
Summary of my perl5 (revision 5 version 40 subversion 0) configuration:

  Platform:
    osname=linux
    osvers=6.6.23
    archname=x86_64-linux
    uname='linux hollister.home.fuzzix.org 6.6.23 #1 smp preempt_dynamic wed mar 27 13:34:48 cdt 2024 x86_64 intel(r) core(tm) i5-7200u cpu @ 2.50ghz genuineintel gnulinux '
    config_args='-de -Dprefix=/home/fuzzix/perl5/perlbrew/perls/perl-5.40.0 -Aeval:scriptdir=/home/fuzzix/perl5/perlbrew/perls/perl-5.40.0/bin'
    hint=recommended
    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='-O2'
    cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='13.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 /lib64 /usr/lib64 /lib /usr/local/lib64
    libs=-lpthread -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
    perllibs=-lpthread -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.39.so
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version='2.39'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-E'
    cccdlflags='-fPIC'
    lddlflags='-shared -O2 -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_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
  Built under linux
  Compiled at Jun  9 2024 22:36:02
  %ENV:
    PERLBREW_HOME="/home/fuzzix/.perlbrew"
    PERLBREW_MANPATH="/home/fuzzix/perl5/perlbrew/perls/perl-5.40.0/man"
    PERLBREW_PATH="/home/fuzzix/perl5/perlbrew/bin:/home/fuzzix/perl5/perlbrew/perls/perl-5.40.0/bin"
    PERLBREW_PERL="perl-5.40.0"
    PERLBREW_ROOT="/home/fuzzix/perl5/perlbrew"
    PERLBREW_SHELLRC_VERSION="0.98"
    PERLBREW_VERSION="0.98"
  @INC:
    /home/fuzzix/perl5/perlbrew/perls/perl-5.40.0/lib/site_perl/5.40.0/x86_64-linux
    /home/fuzzix/perl5/perlbrew/perls/perl-5.40.0/lib/site_perl/5.40.0
    /home/fuzzix/perl5/perlbrew/perls/perl-5.40.0/lib/5.40.0/x86_64-linux
    /home/fuzzix/perl5/perlbrew/perls/perl-5.40.0/lib/5.40.0

[mauke]

I do get the Required parameter 'dependency' is missing for "Foo" constructor error, but then DESTROY is called anyway and segfaults. I think the issue is that the destructor is run at all for objects whose constructor hasn't finished successfully.

[jbarrett]

Good spot, I've amended the report.

@ilmari also pointed out, which might be useful for those looking for this type of lifecycle management, that spelling it method DESTROY will allow access to class members directly.

[leonerd]

Building a debug perl yields this:

$ ./perl -Ilib gh22278.pl 
Required parameter 'dependency' is missing for "Foo" constructor at gh22278.pl line 14.
perl: class.c:299: Perl_pp_methstart: Assertion `fieldp[fieldix]' failed.
Aborted (core dumped)

Indeed the issue is that the object isn't fully-constructed at this point, so some of its SV pointers in the fields array are still NULL.

There's two potential approaches to fixing this:

1. pp_methstart can be more defensive and skip over fields whose pointers are still NULL. This would incur a (very) slight performance cost at every method entry point, having to check all those fields, though I suspect it's small measurement noise at that point, or
2. Instance construction could be split into two phases. The first phase would populate every scalar/array/hash field with a fresh but empty container variable, before actually running any of the initialisation code. That would involve walking the entire ancestry twice during construction.

Overall I feel that option 1 is probably better here, as it involves far smaller code change. A single extra NULL pointer comparison is unlikely to make a huge impact as compared to making all constructors quite a bit slower.

[mauke]

But why is the destructor called at all?

[leonerd]

But why is the destructor called at all?

Perl calls sub DESTROY when it throws away an object instance. There's not a lot we can do about that here.

Certainly we could create (and encourage folks to use) a separate DESTRUCT phaser that is a phaser, not a method, and ensure each one only gets invoked after that particular part of the constructor has completed successfully. In most cases that would be what you wanted to use. But aside from far deeper changes elsewhere, we can't do much about DESTROY so we have to be prepared to handle it.

[leonerd]

It seems the wider topic of how destructors ought to work is still a thing that needs discussing, though I don't think this bug is the place to do it. I'd suggest we begin a separate discussion about that to look at the wider topic overall.