Skip to content

Commit

Permalink
doio: fix shmread() on non-string buffers
Browse files Browse the repository at this point in the history
- If the buffer is a reference, don't leak memory (or abort, on
  debugging perls). The problem is that SvPOK_only() blindly turns off
  some SV flags, but does not decrement any refcounts if ROK was on.
- If the buffer is tied, don't call FETCH. Conceptually, shmread() is a
  bytestring assignment (from a shared memory segment to a scalar
  variable), so it should only STORE. (This is also why most of the code
  can be replaced by sv_setpvn().)

Fixes #22898.
  • Loading branch information
mauke committed Jan 9, 2025
1 parent 3caa23e commit 3a16cb5
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 13 deletions.
13 changes: 2 additions & 11 deletions doio.c
Original file line number Diff line number Diff line change
Expand Up @@ -3421,17 +3421,8 @@ Perl_do_shmio(pTHX_ I32 optype, SV **mark, SV **sp)
return -1;

if (optype == OP_SHMREAD) {
SvGETMAGIC(mstr);
SvUPGRADE(mstr, SVt_PV);
/* suppress warning when reading into undef var (tchrist 3/Mar/00) */
if (! SvOK(mstr))
SvPVCLEAR(mstr);
SvPOK_only(mstr);
char *const mbuf = SvGROW(mstr, (STRLEN)msize+1);

Copy(shm + mpos, mbuf, msize, char);
SvCUR_set(mstr, msize);
*SvEND(mstr) = '\0';
sv_setpvn(mstr, shm + mpos, msize);
SvUTF8_off(mstr);
SvSETMAGIC(mstr);
/* who knows who has been playing with this shared memory? */
SvTAINTED_on(mstr);
Expand Down
8 changes: 8 additions & 0 deletions pod/perldelta.pod
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,14 @@ XXX
L<perlfunc/shmread> and L<perlfunc/shmwrite> are no longer limited to 31-bit
values for their POS and SIZE arguments. [GH #22895]

=item *

L<perlfunc/shmread> is now better behaved if VAR is not a plain string. If VAR
is a tied variable, it calls C<STORE> once; previously, it would also call
C<FETCH>, but without using the result. If VAR is a reference, the referenced
entity has its refcount properly decremented when VAR is turned into a string;
previously, it would leak memory. [GH #22898]

=back

=head1 Known Problems
Expand Down
17 changes: 15 additions & 2 deletions t/io/shm.t
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ if (not defined $key) {
}
}
else {
plan(tests => 28);
plan(tests => 33);
pass('acquired shared mem');
}

Expand Down Expand Up @@ -87,7 +87,7 @@ my ($fetch, $store) = (0, 0);
sub STORE { ++$store; $_[0][0] = $_[1] } }
tie my $ct, 'Counted';
shmread $key, $ct, 0, 1;
is($fetch, 1, "shmread FETCH once");
is($fetch, 0, "shmread FETCH none");
is($store, 1, "shmread STORE once");
($fetch, $store) = (0, 0);
shmwrite $key, $ct, 0, 1;
Expand All @@ -110,6 +110,19 @@ is($store, 0, "shmwrite STORE none");
is($rdbuf, $text, "check we got back the expected (upgraded source)");
}

# GH #22898 - reading into reference is sane
{
my $rdbuf = [];
builtin::weaken(my $wref = $rdbuf);

my $text = 'A';
ok(shmwrite($key, $text, 0, 1), "wrote 'A' to shared segment");
ok(shmread($key, $rdbuf, 0, 1), "read 1 byte into buffer that previously stored a ref");
is(ref($rdbuf), '', "buffer is not a reference anymore");
is($rdbuf, $text, "buffer contains expected string");
is($wref, undef, "no leak: referenced object had refcount decremented");
}

# GH #22895 - 2^31 boundary
SKIP: {
skip("need at least 5GB of memory for this test", 5)
Expand Down

0 comments on commit 3a16cb5

Please sign in to comment.