Mailing List Archive

This is a bug report for perl from zefram@fysh.org,
generated with the help of perlbug 1.43 running under perl 5.37.10.


-----------------------------------------------------------------
<!--[Please describe your issue here]-->

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\.

<!--[Please do not change anything below this line]-->
<!------------------------------------------------------------------- -->


---
**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 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
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
```