initfields CV visibility causes mayhem

[demerphq]

Filed on behalf of Zefram from 5.37.10_6615_1679509295@barba.rous.org

The field-initialising code of a perlclass(1) class is wrapped up in
a CV that's only intended to be used from inside the
automatically-generated constructors of that class and its subclasses.
But it is visible to Perl code, and, being a first-class CV, it is
possible to to call it in other situations. Like this:

$ perl -Mfeature=current_sub -Mexperimental=class -lwe 'class Foo { field $ifsub = __SUB__; method ifsub { $ifsub } } $f = Foo->new; $ifsub = $f->ifsub; $ifsub->($f)'
zsh: segmentation fault  perl -Mfeature=current_sub -Mexperimental=class -lwe

The subroutine is expecting a bare HV of parameters as a second
argument, and crashes if it doesn't get one. This crash arises
specifically in the OPpINITFIELDS part of pp_methstart.

Although it's assuming that there will be a second argument, it is
actually robust to the argument not being of the expected kind. So
it's possible to *successfully* call the subroutine by supplying a
dummy second argument. This has the interesting effect that it makes it
possible to reinitialise the fields of an already-constructed object.
This doesn't just reset the fields' values, it replaces the
variables:

$ perl -Mfeature=current_sub -Mexperimental=class -lwe 'class Foo { field $ifsub = __SUB__; field $a; method ifsub { $ifsub } method aref { \$a } } $f = Foo->new; $ifsub = $f->ifsub; $ar0 = $f->aref; $ifsub->($f, 0); $ar1 = $f->aref; print $ar0; print $ar1'
SCALAR(0x558b635bc5d8)
SCALAR(0x558b635926e8)

I think the intent with this class system is that the fields of an
initialised object should never be replaced. Whether or not replacement
should be permitted in general, doing it this way has the
definitely-unwanted effect of leaking the old field variable:

$ perl -Mfeature=current_sub -Mexperimental=class -MDevel::Peek=Dump -lwe 'class Foo { field $ifsub = __SUB__; field $a; method ifsub { $ifsub } method aref { \$a } } $f = Foo->new; $ifsub = $f->ifsub; $ar0 = $f->aref; $ifsub->($f, 0); $ar1 = $f->aref; $f = undef; Dump($ar0); Dump($ar1)'
SV = IV(0x55fc19b69d38) at 0x55fc19b69d48
  REFCNT = 1
  FLAGS = (ROK)
  RV = 0x55fc19b69460
  SV = NULL(0x0) at 0x55fc19b69460
    REFCNT = 2
    FLAGS = ()
SV = IV(0x55fc19c9bf70) at 0x55fc19c9bf80
  REFCNT = 1
  FLAGS = (ROK)
  RV = 0x55fc19b3f6e8
  SV = NULL(0x0) at 0x55fc19b3f6e8
    REFCNT = 1
    FLAGS = ()

There's more than one way to fix this. It looks like the visibility of
the initfields CV was unanticipated, but it's difficult to avoid for a
real CV. (It can also be seen via the debugger interface.) One
approach to fixing this would be to make the initfields optree not part
of a real CV, so that there's nothing for Perl code to grab.
__SUB__ could yield the constructor subroutine or undef. Trying to
call ops outside the context of a regular CV strikes me as hairy,
though, and liable to open up other unwanted effects.

I think it's probably better to uphold the CVness and make it actually
work. This requires a couple of tweaks, nothing major. Obviously
pp_methstart would have to be robust to the lack of a second argument,
either croaking or treating that situation the same as an argument of
the wrong type. It could also be made to play nicely with Perl callers
by accepting the parameter hash wrapped in reference, so that Perl code
can supply parameters. It can still accept a bare hash from the
constructor XSUB.

I think that field reinitialisation should be prevented for
fully-constructed objects. To do this, the constructor code should
have some kind of finalisation step that marks the object as complete
after initialising the fields. This doesn't actually have to be a new
step, it could just be an existing part of initialisation happening
later. This issue intersects with a related problem about the order of
construction operations: the object with fields uninitialised is
*also* visible (via debugging of the initfields call), and calling
methods on it causes havoc.

To fix both of those problems, I suggest that the object shouldn't be
blessed until field initialisation is complete. This means that the
uninitialised object doesn't look like a constructed object from the
outside, and wouldn't be accepted as an invocant by regular methods.
Then part of the invocant acceptability test needs to be inverted for
the OPpINITFIELDS case: it needs to reject a blessed invocant, and
only accept an *unblessed* SVt_PVOBJ. Either pp_methstart or
pp_initfield would have to check that ObjectMAXFIELD is big enough,
rather than pp_methstart just asserting that it is.

This suggestion still leaves open the possibility of a
not-fully-constructed object having field initialisation run multiple
times. (In fact, it opens up more possibilities for that, because it
would make it possible to run field initialisers for multiple
independent classes on the same SVt_PVOBJ.) I think it's OK for
fields to be reinitialised in an incomplete object, but to avoid
leaking pp_initfield needs to handle refcounting when replacing a
field. Alternatively pp_initfield could check that the field value is
null, croaking if it's not, to refuse to reinitialise.

I can also imagine convoluted ways in which an object could end up
becoming fully constructed while its initfields CV is running, which
would mean that pp_methstart checking that the invocant isn't complete
isn't enough. pp_initfield should also check in some form, either by
croaking on a non-null field value, or by a separate check of the
object's overall state.

This stuff intersects with another thing that I was going to suggest for
reasons not resulting from actual bugs. Not going into the full
reasoning here, I was going to suggest that after field initialisation
is complete the SVt_PVOBJ should have SvREADONLY set, reflecting the
intent that its field variables should never change from then on. The
SvREADONLY flag is another thing that could be used for the purposes
described above, to discern whether an object is initialisable. I
think it makes most sense to both bless and set SvREADONLY after field
initialisation, and to have pp_initfield check SvREADONLY.

I'd be happy to put together a patch implementing some version of the
above.

---

Flags

• category=core
• severity=medium

---

Perl configuration

Site configuration information for perl 5.37.10:

Configured by zefram at Tue Mar 21 14:35:23 GMT 2023.

Summary of my perl5 (revision 5 version 37 subversion 10) configuration:

  Platform:
    osname=linux
    osvers=5.10.0-21-amd64
    archname=x86_64-linux-thread-multi
    uname='linux [barba.rous.org](http://barba.rous.org/) 5.10.0-21-amd64 #1 smp debian 5.10.162-1 (2023-01-21) x86_64 gnulinux '
    config_args='-des -Dprefix=/home/zefram/usr/perl/perl_install/perl-5.37.10-i64-f52 -Duselargefiles -Dusethreads -Uafs -Ud_csh -Uusesfio -Uusenm -Duseshrplib -Dusedevel -Uversiononly -Ui_db'
    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='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 -D_FORTIFY_SOURCE=2'
    optimize='-O2'
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='10.2.1 20210110'
    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 /lib/x86_64-linux-gnu /lib
    libs=-lpthread -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
    perllibs=-lpthread -ldl -lm -lcrypt -lutil -lc
    libc=[libc-2.31.so](http://libc-2.31.so/)
    so=so
    useshrplib=true
    libperl=libperl.so
    gnulibc_version='2.31'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-E -Wl,-rpath,/home/zefram/usr/perl/perl_install/perl-5.37.10-i64-f52/lib/5.37.10/x86_64-linux-thread-multi/CORE'
    cccdlflags='-fPIC'
    lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'


---
@INC for perl 5.37.10:
    /home/zefram/usr/perl/perl_install/perl-5.37.10-i64-f52/lib/site_perl/5.37.10/x86_64-linux-thread-multi
    /home/zefram/usr/perl/perl_install/perl-5.37.10-i64-f52/lib/site_perl/5.37.10
    /home/zefram/usr/perl/perl_install/perl-5.37.10-i64-f52/lib/5.37.10/x86_64-linux-thread-multi
    /home/zefram/usr/perl/perl_install/perl-5.37.10-i64-f52/lib/5.37.10

---
Environment for perl 5.37.10:
    HOME=/home/zefram
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/zefram/usr/perl/perl_install/perl-5.37.10-i64-f52/bin:/home/zefram/usr/perl/util:/home/zefram/pub/x86_64-unknown-linux-gnu/bin:/home/zefram/pub/common/bin:/usr/bin:/bin:/usr/local/bin:/usr/games
    PERL_BADLANG (unset)
    SHELL=/usr/bin/zsh