Avoid referencing uninitialized memory in PerlIOScalar_write#24063
Avoid referencing uninitialized memory in PerlIOScalar_write#24063tonycoz merged 1 commit intoPerl:bleadfrom
Conversation
| } | ||
|
|
||
| { # GH #24008 | ||
| open my $fh, '>', \my $str or BAIL_OUT $!; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@t-a-k I think we are waiting for changing to die, and then this p.r. can be merged
There was a problem hiding this comment.
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.
|
I've pushed a commit to change If these are OK, I will squash them to a single commit. |
| if ((PerlIOBase(f)->flags) & PERLIO_F_APPEND) { | ||
| dst = SvGROW(sv, SvCUR(sv) + count + 1); | ||
| dst = SvGROW(sv, cur + count + 1); | ||
| offset = SvCUR(sv); |
There was a problem hiding this comment.
Could this now be offset = cur; ?
There was a problem hiding this comment.
Could this now be
offset = cur;?
I think this has its pros and cons. Referencing cur after SvGROW will make this variable live across the function call, which will impose more register pressure and might worsen performance. (I assume that SvCUR is always valid after SvGROW, and re-fetching SvCUR is not so complex operation.)
There was a problem hiding this comment.
You could move the assignment before the SvGROW() to avoid that.
Though I doubt the extra memory access of the extra SvCUR() will make any measurable difference.
There was a problem hiding this comment.
You could move the assignment before the SvGROW() to avoid that.
There will be trade-offs here; doing so will extend the lifetime of offset instead.
Anyway, I think that it would be better to keep the changes minimal to fix the issue (#24008) and other improvements like this should be a separate PR.
In this case, I also think the code around here could be simplified so that I'm going to create another PR later.
There was a problem hiding this comment.
(I assume that
SvCURis always valid afterSvGROW,
This was my misunderstanding. SvGROW actually will not touch SvCUR so SvCUR might be invalid even after SvGROW. I'll submit another PR to fix this.
|
Thank you @mauke, I will further squash my commits. |
|
Sorry, I closed by mistake. |
Once a scalar variable is undef'ed, its SvCUR might remain the previous value. Using SvCUR() blindly might bypass the code that world normally clear out the leading bytes and result as garbage (uninitialized) bytes when the target scalar once held a non-empty string and then undef'ed. So only call SvCUR() when the SV is defined. t/io/scalar.t: added a test for this. Thanks to the comment from @jkeenan for improving the test.
7 participants
PerlIOScalar_writeused to blindly useSvCUR()for SVs that is not SvPOK nor SvPOKp and bypasses the code that world normally clear out the leading bytes. This would result as garbage (uninitialized) bytes when the target scalar once held a non-empty string and thenundef'ed.This will fix #24008.