Residual cleanups for tied objects in PL/Perl

On 2026-08-16 Su 1:38 PM, Tom Lane wrote:

The attached patch fixes a null pointer dereference case in
hstore_plperl, and an infinite-loop case in plperl itself.
These were left out of the recent plperl security patch because
they are not security matters according to our current rules;
but they are certainly bugs that ought to be fixed.

The test cases I have that reach these bugs require Perl "Tie"
modules that aren't present in common Perl installations,
so I'm not planning on trying to construct regression test
entries for them. But I've attached two SQL scripts that
cause failures without the patch.

Which modules?

One interesting point is that Claude Opus, which constructed
these test cases, seemed to think that we ought to be able
to read out the values assigned to the tied array or hash.
But what the repaired code actually produces is SQL NULLs.
AFAICT we are getting Perl "undef" values out of hv_iternext
or av_fetch, so it's hard to see how we could do any better.

At first glance I'm inclined to agree with Opus.

I think what is going on here is that the test functions are
failing to install enough infrastructure for the tied object
to produce any output, but maybe someone who knows more Perl
than me can correct that guess.

I wonder if it's conflicting with our opcode restrictions. I'll play
around a bit with it.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

On 2026-08-16 Su 1:38 PM, Tom Lane wrote:

The test cases I have that reach these bugs require Perl "Tie"
modules that aren't present in common Perl installations,
so I'm not planning on trying to construct regression test
entries for them. But I've attached two SQL scripts that
cause failures without the patch.

Which modules?

At least Tie::Array isn't present in RHEL10's standard perl
package set --- I had to install "perl-Tie" to get that test
case to run here.

One interesting point is that Claude Opus, which constructed
these test cases, seemed to think that we ought to be able
to read out the values assigned to the tied array or hash.
But what the repaired code actually produces is SQL NULLs.
AFAICT we are getting Perl "undef" values out of hv_iternext
or av_fetch, so it's hard to see how we could do any better.

At first glance I'm inclined to agree with Opus.

If we can make it work, I'm all for that, but I wonder how many
other places have the same issue.

regards, tom lane

вс, 16 авг. 2026 г. в 22:38, Tom Lane <tgl@sss.pgh.pa.us>:One interesting
point is that Claude Opus, which constructed

these test cases, seemed to think that we ought to be able
to read out the values assigned to the tied array or hash.
But what the repaired code actually produces is SQL NULLs.
AFAICT we are getting Perl "undef" values out of hv_iternext
or av_fetch, so it's hard to see how we could do any better.

Hi Tom!

A tied hash does not store the value in HeVAL(). hv_iternext() leaves that
pointer unset. Tom's NULL check therefore always takes the SQL NULL path,
even when FETCH would return a real value. hv_iterval() plus SvGETMAGIC()
is what actually runs FETCH.

The same applies to a tied array. av_len() stops the infinite loop, but
av_fetch() still returns a magic SV whose SvOK() is false until GETMAGIC.
Without that, SETOF also yields NULLs.

The attached patch includes regress tests. They use a small inline TIEHASH
/ TIEARRAY class, not Tie::Hash or Tie::Array.

--
Regards,
Rachitskiy Andrey

Andrey Rachitskiy <pl0h0yp1@gmail.com> writes:

A tied hash does not store the value in HeVAL(). hv_iternext() leaves that
pointer unset. Tom's NULL check therefore always takes the SQL NULL path,
even when FETCH would return a real value. hv_iterval() plus SvGETMAGIC()
is what actually runs FETCH.

The same applies to a tied array. av_len() stops the infinite loop, but
av_fetch() still returns a magic SV whose SvOK() is false until GETMAGIC.
Without that, SETOF also yields NULLs.

Hmm. I confirm that adding SvGETMAGIC() makes the tied-array test
work (didn't try the hash case). But to my previous point, where
else would we need to call it, if we're going to try to support
such cases? Also, what exactly are the preconditions for that
function? Looking at the perl header files here, it looks like
sv being nonnull is required and sufficient ... but your patch
is inconsistent about checking that.

regards, tom lane

On 2026-08-16 Su 3:04 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2026-08-16 Su 1:38 PM, Tom Lane wrote:

The test cases I have that reach these bugs require Perl "Tie"
modules that aren't present in common Perl installations,
so I'm not planning on trying to construct regression test
entries for them. But I've attached two SQL scripts that
cause failures without the patch.

Which modules?

At least Tie::Array isn't present in RHEL10's standard perl
package set --- I had to install "perl-Tie" to get that test
case to run here.

That seems odd, given that we use Tie::Hash and Tie::StdHash in
src/pl/plperl/plc_trusted.pl

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

пн, 17 авг. 2026 г. в 01:26, Tom Lane <tgl@sss.pgh.pa.us>:

But to my previous point, where
else would we need to call it, if we're going to try to support
such cases? Also, what exactly are the preconditions for that
function?

If we want a tied hash or array to convert like an ordinary one, GETMAGIC
is still needed before SvOK() / SvROK() in plperl_sv_to_datum() and
jsonb_plperl's SV_to_JsonbValue(). plperl_build_tuple_result() still
uses HeVAL() and will crash on a tied hash the same way hstore_plperl
did. That loop also calls hek2cstr(), which uses FREETMPS, so
hek2cstr() has to run before hv_iterval().

The trigger path does not return a hash. After "MODIFY",
plperl_modify_tuple() walks $_TD->{new} with HeVAL(). PL/Perl already
filled that hash from the tuple, so it is not tied. A crash would
need the trigger to tie that hash itself, for example

tie %{$_TD->{new}}, 'SomeTie';
$_TD->{new}{v} = 'from_tie';
return 'MODIFY';

Looking at the perl header files here, it looks like

sv being nonnull is required and sufficient ... but your patch
is inconsistent about checking that.

Yes. Non-NULL is required and sufficient.

The extra NULL checks on the hstore side are not needed after
hv_iterval(). I left the old HeVAL() guards in because the point was
to show that FETCH results are available, and why a NULL check on
HeVAL() always yields SQL NULL. The if (svp) in the SETOF loop is
only because av_fetch() returns NULL for a missing element.

That seems odd, given that we use Tie::Hash and Tie::StdHash in
src/pl/plperl/plc_trusted.pl

Trusted plperl replaces require with pp_require_safe, which only
succeeds if the module is already in %INC. Otherwise it dies with
"Unable to load Tie/Array.pm into plperl". tie itself is allowed.
plperlu can load the modules as usual.

Proof:
```
\set ON_ERROR_STOP off

CREATE EXTENSION IF NOT EXISTS plperl;
CREATE EXTENSION IF NOT EXISTS plperlu;

\echo === 1) plperl: use Tie::Array ===
CREATE OR REPLACE FUNCTION try_use_tie_array() RETURNS text
LANGUAGE plperl AS $$
use Tie::Array;
return 'loaded';
$$;
SELECT try_use_tie_array();

\echo === 3) plperl: tie with inline class (no use) ===
CREATE OR REPLACE FUNCTION try_tie_inline() RETURNS text
LANGUAGE plperl AS $$
{
package PLPerlProofTie;
sub TIEARRAY { bless [], $_[0] }
sub STORE { $_[0][$_[1]] = $_[2] }
sub FETCH { $_[0][$_[1]] }
sub FETCHSIZE { scalar @{$_[0]} }
}
my @a;
tie @a, 'PLPerlProofTie';
$a[0] = 'ok';
return $a[0];
$$;
SELECT try_tie_inline();

\echo === 4) plperlu: use Tie::Array ===
CREATE OR REPLACE FUNCTION try_use_tie_array_u() RETURNS text
LANGUAGE plperlu AS $$
use Tie::Array;
return 'loaded';
$$;
SELECT try_use_tie_array_u();
```

пн, 17 авг. 2026 г. в 02:40, Andrey Rachitskiy <pl0h0yp1@gmail.com>:

If we want a tied hash or array to convert like an ordinary one, GETMAGIC
is still needed before SvOK() / SvROK() in plperl_sv_to_datum() and
jsonb_plperl's SV_to_JsonbValue(). plperl_build_tuple_result() still
uses HeVAL() and will crash on a tied hash the same way hstore_plperl
did. That loop also calls hek2cstr(), which uses FREETMPS, so
hek2cstr() has to run before hv_iterval().

The first patch tries to close every Perl-to-SQL path that still
ignored FETCH on tied values.

The trigger path does not return a hash. After "MODIFY",
plperl_modify_tuple() walks $_TD->{new}. That hash is normally filled
by PL/Perl from the tuple, so it is not tied. The loop is still fixed,
so the HeVAL() bug is not left behind if a trigger replaces that hash
with a tied one.

The second patch is regress only. Separate files cover the cases the
code patch touches: SETOF text, scalar text, composite, integer[], a
BEFORE INSERT trigger that swaps in a tied $_TD->{new}, hstore, and
jsonb. The classes are small inline TIEHASH / TIEARRAY / TIESCALAR
helpers, not Tie::Hash or Tie::Array.

--
Regards,
Rachitskiy Andrey

пн, 17 авг. 2026 г. в 03:24, Andrey Rachitskiy <pl0h0yp1@gmail.com>:

The first patch tries to close every Perl-to-SQL path that still
ignored FETCH on tied values.

I have attached v2.

The previous version missed a few GETMAGIC call sites:
get_perl_array_ref(), the trigger return value, $_TD->{new} before
the SvOK() / SvROK() checks, and the start of return_next(). A tied
scalar whose FETCH returns an arrayref or a hashref still looked like
undef.

That also made the SETOF walker cleaner. The loop only walks with
av_len() and calls return_next(). FETCH lives in return_next() and
plperl_sv_to_datum(), so the loop does not run GETMAGIC on each
element itself.

The regress patch covers those cases. tied_scalar_setof and
tied_scalar_int_array wrap an arrayref in a tied scalar. tied_modify
uses tied scalars for $_TD->{new} and for the "MODIFY" return.

--
Regards,
Rachitskiy Andrey

Andrey Rachitskiy <pl0h0yp1@gmail.com> writes:

I have attached v2.

v2 looks like a bunch of very random changes. I would like to see
more consistency in the code, ie exactly one pattern for iterating
through hashes and exactly one pattern for iterating through arrays.
We'd not have had the original infinite-loop problem in the first
place if plperl_func_handler() hadn't seen fit to use av_fetch() in a
way randomly different from every other usage. We still have multiple
patterns for iterating through hashes: why do a couple of places use
hv_iternextsv when the others don't? And why do we use hek2cstr
in some places and not others?

(oh, and for extra credit: why is there a second call of hv_iterinit
in plperl_build_tuple_result and plperl_modify_tuple?)

I think we'd be well advised to clean up that inconsistency, and then
inject tied-object handling in the same way in each loop.

regards, tom lane

I spent some time digging through the Perl documentation and source
code, and I'm starting to have doubts about this idea of "fix it
by applying SvGETMAGIC to array/hash entries". I can't see anything
in the documentation recommending such an approach, and I don't see
any of their own code doing so either. Pretty much all their calls
to SvGETMAGIC are in functions that have been passed a scalar value
from elsewhere. So while this may work today, I'm unconvinced that
it's idiomatic or future-proof. Can you point at any production code
that is doing it this way?

regards, tom lane

пн, 17 авг. 2026 г. в 21:49, Tom Lane <tgl@sss.pgh.pa.us>:

I spent some time digging through the Perl documentation and source
code, and I'm starting to have doubts about this idea of "fix it
by applying SvGETMAGIC to array/hash entries". I can't see anything
in the documentation recommending such an approach, and I don't see
any of their own code doing so either. Pretty much all their calls
to SvGETMAGIC are in functions that have been passed a scalar value
from elsewhere. So while this may work today, I'm unconvinced that
it's idiomatic or future-proof. Can you point at any production code
that is doing it this way?

https://perldoc.perl.org/perlguts - generic C code must call the

SvGETMAGIC() macro to invoke any 'get' magic if they use an SV obtained
from external sources in functions that don't handle magic. See perlapi for
a description of these functions.
https://perldoc.perl.org/perlapi#SvGETMAGIC - This is an idiomatic way.
I can search for the production code, but there are many xs modules on
metacpan and everyone uses this approach, as it is described in the
documentation. As an example, let's use the Encode module
https://github.com/Perl/perl5/blob/c31ca2013f287840fcddf498ead9602666569966/cpan/Encode/Encode
.xs#L349

пн, 17 авг. 2026 г. в 22:21, Andrey Rachitskiy <pl0h0yp1@gmail.com>:

As an example, let's use the Encode module
https://github.com/Perl/perl5/blob/c31ca2013f287840fcddf498ead9602666569966/cpan/Encode/Encode
.xs#L349

sorry, here is the correct link - `
https://github.com/Perl/perl5/blob/c31ca2013f287840fcddf498ead9602666569966/cpan/Encode/Encode.xs#L1107`

Andrey Rachitskiy <pl0h0yp1@gmail.com> writes:

sorry, here is the correct link - `
https://github.com/Perl/perl5/blob/c31ca2013f287840fcddf498ead9602666569966/cpan/Encode/Encode.xs#L1107`

Yeah, this is an example of the pattern I was talking about: functions
calling SvGETMAGIC on random SV*'s that are passed to them. What
I do not see is SvGETMAGIC being called on values obtained from
hv_iternext or av_fetch. Maybe all the internal usages in Perl are
working with hashes/arrays that they know aren't tied, but that
seems unlikely.

regards, tom lane

Looking a bit more at the issue of standardizing our
perl-array-scanning code, I noticed that av_len returns SSize_t
not int, creating an overflow risk since we aren't consistent
about assigning the result to something wider than int. This
would only result in truncating the output not anything more
exciting, but it's still bad. Also, it seems from looking at
the Perl sources that they prefer to use av_count() to compute
the loop limit. That's the same as av_len()+1 (except it returns
Size_t), but I think that's a better habit to be in since it
removes the risk of forgetting to add 1. So here is a patch
that just cleans up those issues.

I'm inclined to push this, and separately push a patch to
clean up the inconsistent hash accessing logic, and then deal
with magic objects on top of that.

regards, tom lane

пн, 17 авг. 2026 г. в 23:08, Tom Lane <tgl@sss.pgh.pa.us>:

Looking a bit more at the issue of standardizing our
perl-array-scanning code, I noticed that av_len returns SSize_t
not int, creating an overflow risk since we aren't consistent
about assigning the result to something wider than int. This
would only result in truncating the output not anything more
exciting, but it's still bad. Also, it seems from looking at
the Perl sources that they prefer to use av_count() to compute
the loop limit. That's the same as av_len()+1 (except it returns
Size_t), but I think that's a better habit to be in since it
removes the risk of forgetting to add 1. So here is a patch
that just cleans up those issues.

The patch looks good to me.

Apparently av_fetch works as follows:
av_fetch returns SV** pointing to the real array element (for regular
arrays) OR mortal SV (for tied arrays)
- for regular arrays, the returned SV is NOT mortal — it lives in the
array
- for tied arrays, the returned SV MAY be mortal (temporary) because
it's created on the fly as a proxy
Calling SvGETMAGIC is critical because it:
- for regular SV: does nothing
- for magical SV: calls the FETCH method and turns the temporary "proxy"
into a real value.

--
Regards,
Rachitskiy Andrey

On 2026-08-16 Su 5:10 PM, Andrew Dunstan wrote:

On 2026-08-16 Su 3:04 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2026-08-16 Su 1:38 PM, Tom Lane wrote:

The test cases I have that reach these bugs require Perl "Tie"
modules that aren't present in common Perl installations,
so I'm not planning on trying to construct regression test
entries for them.  But I've attached two SQL scripts that
cause failures without the patch.

Which modules?

At least Tie::Array isn't present in RHEL10's standard perl
package set --- I had to install "perl-Tie" to get that test
case to run here.

That seems odd, given that we use Tie::Hash and Tie::StdHash in
src/pl/plperl/plc_trusted.pl

Answering myself, it appears that those are part of RHEL10's perl-libs
package, while Tie::Array and Tie::StdArray are part of the perl-Tie
package. That seems at first glance like a really awful packaging
decision, something not entirely without precedent.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

Answering myself, it appears that those are part of RHEL10's perl-libs
package, while Tie::Array and Tie::StdArray are part of the perl-Tie
package. That seems at first glance like a really awful packaging
decision, something not entirely without precedent.

Poking around, it looks like they may have done that because
parent.pm ("use parent" support) requires Tie::StdHash, and they
wanted that in the base installation. It does seem pretty strange
though. There is not much in perl-Tie:

$ rpm -ql perl-Tie
/usr/share/man/man3/Tie::Array.3pm.gz
/usr/share/man/man3/Tie::Handle.3pm.gz
/usr/share/man/man3/Tie::Scalar.3pm.gz
/usr/share/man/man3/Tie::StdHandle.3pm.gz
/usr/share/man/man3/Tie::SubstrHash.3pm.gz
/usr/share/perl5/Tie
/usr/share/perl5/Tie/Array.pm
/usr/share/perl5/Tie/Handle.pm
/usr/share/perl5/Tie/Scalar.pm
/usr/share/perl5/Tie/StdHandle.pm
/usr/share/perl5/Tie/SubstrHash.pm

I see the same division in RHEL9. Don't have a live RHEL8
installation to check anymore.

regards, tom lane

Andrey Rachitskiy <pl0h0yp1@gmail.com> writes:

The patch looks good to me.

Pushed that bit, then.

regards, tom lane

Hi, Tom!

Here's a patch to try to clean that up. I think it is wrong that
the extension modules don't use hek2cstr, so I made them do so.
(But we probably shouldn't back-patch that: it's a user-visible
behavioral change and we've not gotten actual field complaints AFAIR.)

Agreed
path looks good for me

(oh, and for extra credit: why is there a second call of hv_iterinit
in plperl_build_tuple_result and plperl_modify_tuple?)

That seems to be just cargo-culted, so I removed those calls.

hv_iterinit is good practice to clean up the iterator state, but here it's
redundant and misleading.

--
Regards,
Rachitskiy Andrey