Fix some integer overflow issues (and a porting/diag.t bug) #23597
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before this patch ================= The porting/diag.t test tries to make sure every error message emitted by perl is documented in pod/perldiag.pod. The way it does this is as follows: 1. Collect list of diagnostic functions by taking all known functions whose name contains warn, err, die, croak, or deprecate. Add Perl_mess and anything that starts with PERL_DIAG_. Add the special (v)FAIL(2,3,4) macros used in regcomp.c. 2. For every input file, scan it line by line. 3. If the line starts with "#", skip it (so we don't find macro definitions like "#define croak(...) ..." themselves). 4. If the line contains the name of a diagnostic function, collect the line. 5. If the function name is followed by a simple argument list (basically "(...)" with at most one level of nested parentheses within), stop. Otherwise keep collecting lines until we find a line that ends with ");", which we take to be the end of the statement. 6. Join all collected lines into one blob of text and rescan it for the diagnostic message. 7. Read the next line and repeat (step 3). There are some issues with this approach. For example, step 5 may end up eating large chunks of the input file until a ");" line is encountered. If we're in a header that "#define"s many macros in a row, all of them will be swallowed here. Also, while "#define" lines themselves are skipped, the body of the macro may not be: If the "#define" line ends with a backslash, the rest of the macro definition (placed on the following lines) will be scanned as normal. Finally, there is a bug in the "simple argument list" check in step 5: It doesn't handle a completely empty argument list (as in "croak_memory_wrap()", for example), so it starts collecting lines until it finds the next ");". After this patch ================ Extend step 3: If the line is a definition of one of the regcomp error macros (e.g. "#define FAIL2(...)"), skip the entire definition, including backslash continuation lines. These macros are just wrappers around croak; they don't emit any particular diagnostics of their own. Extend step 5: Handle empty argument lists so "croak_memory_wrap()" does not start a runaway scan for ");". Extend step 5: When collecting lines, don't just stop at ");", but also at (or rather just before) lines starting with "#", so we don't blindly swallow preprocessor directives. If an error message is not found in perldiag, report the source location where it is emitted with the number of the first line of its collected chunk (i.e. the location of the diagnostic function name), not (as before) the last line of the chunk (i.e. the location of the next ");", which may be a long ways off). This exposes one new message in regcomp_internal.h, which has been added to perldiag.
Coverity says:
CID 584099: Integer handling issues (INTEGER_OVERFLOW)
Expression "newlen + 8UL", where "newlen" is known to be equal to 18446744073709551615, overflows the type of "newlen + 8UL", which is type "unsigned long".
(Referring to (n) + PTRSIZE - 1 where n = newlen and PTRSIZE = 8UL.)
Crudely avoid the issue by checking n for overflow beforehand and dying
with a "panic: memory wrap" error if so.
Coverity says:
CID 584102: Insecure data handling (INTEGER_OVERFLOW)
The cast of "S_sequence_num(my_perl, ((XPVCV *)({...; p_;}))->xcv_start_u.xcv_start)" to a signed type could result in a negative number.
Avoid the issue by taking the UV returned by sequence_num and printing
it directly (without going through IV conversion).
Coverity says:
CID 584101: Integer handling issues (INTEGER_OVERFLOW)
Expression "status", where "Perl_SvIV(my_perl, out)" is known to be equal to 0, overflows the type of "status", which is type "int".
What it really means is that doing 'int status = SvIV(...)' may overflow
(since IV can be (and on 64-bit systems usually is) wider than int).
This patch doesn't fix that issue. However, it avoids yet another type
in the mix: S_run_user_filter() is declared as returning I32, not int,
and elsewhere assigns filter_read(...) (also an I32) to 'result'.
So instead of worrying about (overflowing) conversions between IV, int,
and I32, make 'result' an I32 and only worry about (overflowing)
conversions between IV and I32.
Coverity says:
CID 584092: Integer handling issues (INTEGER_OVERFLOW)
Expression "result", where "Perl_SvIV(my_perl, *my_perl->Istack_sp)" is known to be equal to 0, overflows the type of "result", which is type "I32".
The sort comparison function returns IV (a Perl integer), but all the
sorting routines in this file want to work with I32. Instead of
converting (and possibly truncating) the value directly, normalize the
result to -1/0/1, which fits in any integer type.
Coverity says:
CID 584095: Integer handling issues (INTEGER_OVERFLOW)
Expression "duration", where "(IV)Perl_SvIV(my_perl, *sp--)" is known to be equal to 0, overflows the type of "duration", which is type "I32 const".
There are two dodgy type conversions in this function: from IV (POPi) to
I32 (duration), and from I32 (duration) to unsigned int (sleep argument,
by cast). Avoid the one Coverity complains about by making 'duration' an
IV.
Contributor
Author
|
If no one screams within a day or two, I intend on merging this PR shortly. |
khwilliamson
approved these changes
Sep 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Coverity reports potential integer overflow issues. Some of them are relatively straightforward to handle.
The change to
porting/diag.tis needed because otherwise it would mishandle thecroak_memory_wrap()call added here.