Skip to content

Conversation

@mauke
Copy link
Contributor

@mauke mauke commented Aug 27, 2025

Fixes a Coverity issue:

>>>     function_return: Function Perl_delimcpy_no_escape(tmpbuf, tmpbuf + 4096UL, s, bufend, 58, &len) modifies its argument, assigning 2147483647 to len.
3553            s = delimcpy_no_escape(tmpbuf, tmpbuf + sizeof tmpbuf, s, bufend,
3554                                   ':', &len);
>>>     CID 583353: (#1 of 1): Overflowed constant (INTEGER_OVERFLOW)
>>>     overflow_const: Expression len + 1, where len is known to be equal to 2147483647, overflows the type of len + 1, which is type int.
3558            if (len + 1 + strlen(scriptname) + MAX_EXT_LEN >= sizeof tmpbuf)
3559                continue;       /* don't search dir with too-long name */

If there is not enough available space in tmpbuf, delimcpy_no_escape sets len to I32_MAX, but the following code does not check for this. (I believe this case is reachable simply by setting PATH to a huge string.)

Avoid the potential overflow by rewriting

A + B >= C

as

A >= C - B

(Also, make 'len' unsigned (specifically, size_t) to match the type of sizeof/strlen() and avoid warnings about comparisons between signed and unsigned integers.)


  • This set of changes does not require a perldelta entry.

Fixes a Coverity issue:

    >>>     function_return: Function Perl_delimcpy_no_escape(tmpbuf, tmpbuf + 4096UL, s, bufend, 58, &len) modifies its argument, assigning 2147483647 to len.
    3553            s = delimcpy_no_escape(tmpbuf, tmpbuf + sizeof tmpbuf, s, bufend,
    3554                                   ':', &len);
    >>>     CID 583353: (Perl#1 of 1): Overflowed constant (INTEGER_OVERFLOW)
    >>>     overflow_const: Expression len + 1, where len is known to be equal to 2147483647, overflows the type of len + 1, which is type int.
    3558            if (len + 1 + strlen(scriptname) + MAX_EXT_LEN >= sizeof tmpbuf)
    3559                continue;       /* don't search dir with too-long name */

If there is not enough available space in tmpbuf, delimcpy_no_escape
sets len to I32_MAX, but the following code does not check for this. (I
believe this case is reachable simply by setting PATH to a huge string.)

Avoid the potential overflow by rewriting

    A + B >= C

as

    A >= C - B

(Also, make 'len' unsigned (specifically, size_t) to match the type of
sizeof/strlen() and avoid warnings about comparisons between signed and
unsigned integers.)
@jkeenan
Copy link
Contributor

jkeenan commented Aug 27, 2025

If there is not enough available space in tmpbuf, delimcpy_no_escape sets len to I32_MAX, but the following code does not check for this. (I believe this case is reachable simply by setting PATH to a huge string.)

Would it be feasible to write a regression test for this?

@mauke
Copy link
Contributor Author

mauke commented Aug 27, 2025

Would it be feasible to write a regression test for this?

I don't think so. You can get delimcpy_no_escape to return I32_MAX, but in practice, len + 1 + strlen(scriptname) + MAX_EXT_LEN is likely to produce a huge unsigned number, which (being greater than sizeof tmpbuf) triggers the continue statement (and no harm done). The len + 1 part is technically undefined behavior, but I don't know how to make it manifest in an observable manner.

Copy link
Contributor

@tonycoz tonycoz left a comment

Choose a reason for hiding this comment

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

I'll admit this makes me want a new UI for delimcpy_no_escape(), though I'm not sure what that would be.

@mauke mauke merged commit 91a2ef9 into Perl:blead Aug 28, 2025
34 checks passed
@mauke mauke deleted the fix-coverity-583353-find-script branch August 28, 2025 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants