Skip to content

Commit

Permalink
pp_pack: SvPV_force() pack(p/P, ...) arguments when writable
Browse files Browse the repository at this point in the history
pack("p"/"P") is generally pretty awful, but we document that you
can pack("P") and then write to the pointer supplied, so make that
safer by ensuring the PV isn't shared with other SVs.

Since _force() throws on a READONLY SV, and you shouldn't be writing
to one, only _force() for a writable SV.

This is still broken when magic is involved, but we did document it
as usable.

Fixes the UUID::FFI part of #22380

Some other modules (and UUID::FFI itself, really) will need XS
fixes.
  • Loading branch information
tonycoz committed Jul 15, 2024
1 parent 9cc725b commit 70cffae
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 3 deletions.
6 changes: 5 additions & 1 deletion ext/XS-APItest/APItest.xs
Original file line number Diff line number Diff line change
Expand Up @@ -4941,7 +4941,11 @@ rc_add(sv1, sv2)
rpp_replace_2_1(r);
return;


void
modify_pv(IV pi, IV sz)
PPCODE:
/* used by op/pack.t when testing pack "p" */
memset(INT2PTR(char *, pi), 'y', sz);

MODULE = XS::APItest PACKAGE = XS::APItest::AUTOLOADtest

Expand Down
2 changes: 1 addition & 1 deletion pp_pack.c
Original file line number Diff line number Diff line change
Expand Up @@ -3076,7 +3076,7 @@ S_pack_rec(pTHX_ SV *cat, tempsym_t* symptr, SV **beglist, SV **endlist )
Perl_ck_warner(aTHX_ packWARN(WARN_PACK),
"Attempt to pack pointer to temporary value");
}
if (SvPOK(fromstr) || SvNIOK(fromstr))
if (SvREADONLY(fromstr))
aptr = SvPV_nomg_const_nolen(fromstr);
else
aptr = SvPV_force_flags_nolen(fromstr, 0);
Expand Down
16 changes: 15 additions & 1 deletion t/op/pack.t
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ BEGIN {
set_up_inc(qw '../lib ../cpan/Math-BigInt/lib');
}

plan tests => 14722;
plan tests => 14724;

use strict;
use warnings qw(FATAL all);
Expand Down Expand Up @@ -343,6 +343,20 @@ sub foo { my $a = "a"; return $a . $a++ . $a++ }
like($warning, qr/temporary val/);
}

SKIP:
{
# based on https://github.com/Perl/perl5/issues/22380
eval { require XS::APItest; 1 }
or skip "Cannot load XS::APItest", 2;
# writable "P"
my $orig = "x" x 36;
my $data = $orig;
my $ptr = unpack 'j', pack 'P', $data;
XS::APItest::modify_pv($ptr, length $data);
is($data, "y" x 36, "check \$data was modified");
is($orig, "x" x 36, "check \$orig wasn't modified");
}

# undef should give null pointer
like(pack("p", undef), qr/^\0+$/);
like(pack("p<", undef), qr/^\0+$/);
Expand Down

0 comments on commit 70cffae

Please sign in to comment.