Skip to content

Avoid referencing uninitialized memory in PerlIOScalar_write #24063

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
t-a-k wants to merge 3 commits into
base: blead
Choose a base branch
from
+18 −4
Open

Avoid referencing uninitialized memory in PerlIOScalar_write #24063

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ea1aa9
PerlIOScalar_write: initialize target scalar if undefined
t-a-k Jan 7, 2026
863c99c
PerlIOScalar_write: avoid SvCUR() on an undefined scalar
t-a-k Jan 17, 2026
47026e3
t/io/scalar.t: Changed BAIL_OUT to die
t-a-k Jan 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion perlio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1259,7 +1259,13 @@ PerlIOScalar_write(pTHX_ PerlIO * f, const void *vbuf, Size_t count)
char *dst;
SvGETMAGIC(sv);
if (!SvROK(sv)) sv_force_normal(sv);
if (SvOK(sv)) SvPV_force_nomg_nolen(sv);
if (SvOK(sv))
SvPV_force_nomg_nolen(sv);
else
/* SvPV_force_nomg_nolen would initialize PV even if !SvOK(sv),
but in such case it will emit "Use of uninitialized value"
warning, which is not expected here. */
SvPVCLEAR(sv);
Copy link
Contributor

@tonycoz tonycoz Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation is strange, the else should be at the same level as the if to match our common usage, see the if/else starting line 1274 for example.

I think the conditional sv_force_normal() above your change is now redundant, since all paths now either force_normal (SvPV_force*()) or set an explicit value.

Copy link
Contributor Author

@t-a-k t-a-k Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation is strange,

This seems to be due to GitHub's diff rendering. The if (SvOK(sv)) line is indented with a single tab character (this came from the original source before this change), while other added lines are indented without tab characters (according to .editorconfig).
.editorconfig also specifies tab_width = 8 and the else line is indented with 8 spaces, so these lines should appear to be aligned.

Copy link
Contributor

@Leont Leont Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would seem that is my fault. Before ec57fb9 this part of the code was in a separate module that hadn't been updated to the "new" core style standard, and I didn't notice I should have fixed the indentation in that process.

I would suggest we follow up with a commit that fixes all the tabs to spaces.

if (SvUTF8(sv) && !sv_utf8_downgrade(sv, TRUE)) {
ck_warner(packWARN(WARN_UTF8), code_point_warning);
SETERRNO(EINVAL, SS_IVCHAN);
Expand Down
6 changes: 6 additions & 0 deletions pod/perldelta.pod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,12 @@ used to be parsed incorrectly.

=item *

When a scalar variable used as the target of a filehandle is C<undef>'ed
while being output, garbage bytes would be left in that scalar.
[L<GH #24008|https://github.com/Perl/perl5/issues/24008>]

=item *

XXX

=back
Expand Down
10 changes: 9 additions & 1 deletion t/io/scalar.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use strict;
use Fcntl qw(SEEK_SET SEEK_CUR SEEK_END); # Not 0, 1, 2 everywhere.
use Errno qw(EACCES);

plan(128);
plan(129);

my $fh;
my $var = "aaa\n";
Expand Down Expand Up @@ -528,3 +528,11 @@ SKIP:
select((select($fh), ++$|)[0]);
ok(!(print $fh "x"), "write to a large offset");
}

{ # GH #24008
open my $fh, '>', \my $str or BAIL_OUT $!;
Copy link
Contributor

@jkeenan jkeenan Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised to see that this suggests using BAIL_OUT. According to perldoc Test::More, BAIL_OUT

        Indicates to the harness that things are going so badly all testing
        should terminate. This includes the running of any additional test
        scripts.

        This is typically used when testing cannot continue such as a
        critical module failing to compile or a necessary external utility
        not being available such as a database connection failing.

        The test will exit with 255.

Why wouldn't a simple die suffice here?

Copy link
Contributor

@Leont Leont Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Copy link
Contributor

@khwilliamson khwilliamson Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t-a-k I think we are waiting for changing to die, and then this p.r. can be merged

Copy link
Contributor Author

@t-a-k t-a-k Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delayed response. I will make a commit shortly.

I think that BAIL_OUT is provided because die would change the behavior due to some internal state, such as $SIG{__DIE__}, $! and enclosing eval, so it might not be able to make the test fail reliably.

print $fh "xxxxx";
undef $str;
print $fh "y";
is($str, "\0\0\0\0\0y", "write to undef'ed variable");
}
Loading