fix a collection of other I32 bugs #21012
Conversation
tonycoz
added
the
defer-next-dev
tonycoz
force-pushed
the
20917-other-i32-bugs
branch
from
c7f61ce to
00e4038
Compare
|
squashed the "squash me" commit and rebased |
tonycoz
removed
the
defer-next-dev
|
@tonycoz, to properly test this pull request, should we be building a perl with any particular configuration arguments in order to get meaningful results? (This p.r. has 2 approvals, but probably has not yet been tested other than GH actions.) |
|
To test this properly you need to set PERL_TEST_MEMORY set in the environment to 8 or more, and have that amount of available memory (in GB). They only make sense for 64-bit platforms, ie. where ptrsize is 8 (or more if that exists.) |
|
IN the perlreapi.pod commit you say "though I suspect it could use some, some members mentioned no longer exist,", which might be the case as this is a copy of the actual structure. Ill take a look, but i guess your patch will need to be applied first. Actually, even better maybe would be to drop the perlreapi.pod patch and let me take care of it. |
tonycoz
force-pushed
the
20917-other-i32-bugs
branch
from
00e4038 to
77c06b8
Compare
I've removed it. I also added another fix for a bug that came up while I was testing the rebased code. Unfortunately I expect we have a number of similar bugs, I think it means we need to add This is a big job: and it takes more than just adding a bunch of casts. |
Ok, I have just pushed #21241 as promised. |
Discovered while scanning for stack issues, S_scomplement which implements string complement for pp_complement and pp_scomplement used an I32 to keep the working length, for strings between 2GB and 4GB this resulted in a noop.
This deliberately dropped out since repeatcpy() took an I32 length of source string parameter, but the code in repeatcpy() is sufficiently robust we can simply make it a SSize_t. Removed the error for strings over I32 len, the left over check should reasonably handle integer overflow beyond the capacity of a SSize_t.
If I understand the code clen is the length of a constant replacement string, ie. s/.../THIS/. I don't expect this would ever go over 2GB, but aim for correctness. I made this a SSize_t cast instead of completely removing the cast since some analysers/compilers reasonably complain about such comparisons.
I wasn't able to make this misbehave, since with a COW enabled perl a COW copy of the input SV is created when the regexp is matched above, and this entire block is skipped down to "have_a_cow:"
This was casting the offsets to I32 when restoring them, but these values can be outside the positive range of an I32, which caused problems in pp_substcont.
I wasn't able to trigger this code on a COW built perl, but we can match against strings over 2GB, so this value could become too large for an I32 on a 64-bit platform.
The size parameters for msgsnd() and msgrcv() are size_t, and the return value of msgrcv() is ssize_t, adjust perl to match. Unfortunately Linux appears to limit the size of the message queue to well under 2GB, so I wasn't able to write a failing test for this.
This would call get magic on the buffer SV, even though it's output only. It failed to call set magic on the buffer SV.
While the code checks that size is non-negative, that check was done after using that size with SvGROW(), so a negative size caused a panic, rather than the error return value the code appeared to be written for.
These could in theory overflow a 32-bit signed integer (which is undefined behaviour)
While we can't test this easily from perl (as the comment mentions, the pos() is cleared in utf8::decode()), testing under the debugger revealed the pos value being truncated here, so use the correct type.
This was new to me, I hadn't encountered this failure while working on the original "other I32 bugs". The original test here was failing with an "Out of memory" error since the long hash key length was overflowing the I32. Once that was fixed the test was failing purely due to the invalid code, once that was fixed the test passed so I removed the TODO.
tonycoz
force-pushed
the
20917-other-i32-bugs
branch
from
77c06b8 to
2f9154b
Compare
|
Pushed as merge commit 03f6a79 |
4 participants
These commits fix a few of the other I32 bugs I suspected when I was reviewing the code for the #20990 work.
I have a few more I haven't investigated yet.