re_op_compile: Coverity ASSIGN_WHERE_COMPARE_MEANT fix

[richardleach]

Coverity CID 316368 raises the concern that in the following comparison:
(OP(first) == OPEN && (sawopen = 1)
the intention might have been to compare sawopen, not assign to it.

The intention genuinely is to assign to sawopen, but since there are two
assignments in this fashion followed by two assignments in the subsequent
block, we can do a small refactor to consolidate the assignments in a way
that will hopefully keep Coverity happy.

[iabyn]

Well this isn't just a refactor, it's a change in behaviour. Previously, sawlookahead was set if two conditions were true: OP(first) == IFMATCH && !first->flags; now it just needs the first condition. I have no idea whether it matters, but if it doesn't, I'd prefer this as two commits - a pure refactor, followed by removing of the extra test which (allegedly) isn't needed.

[richardleach]

Previously, sawlookahead was set if two conditions were true: OP(first) == IFMATCH && !first->flags; now it just needs the first condition.

I was thinking that, w.r.t sawlookahead, the while block would only be entered if either:

• (OP(first) == IFMATCH && !first->flags) is true
  or I suppose:
• (PL_regkind[OP(first)] == CURLY && ARG1(first) > 0) is true and it happens to simultaneously be the case (I don't know if it's possible) that (OP(first) == IFMATCH && first->flags)

I could do something around that second case if so.

[richardleach]

Alternatively, would structuring it like this be acceptable?

        while (1) {
            if (OP(first) == OPEN)
                sawopen = 1;
            /* for now we can't handle lookbehind IFMATCH*/
            else if (OP(first) == IFMATCH && !first->flags)
                sawlookahead = 1;
            else if (OP(first) == PLUS) {
                sawplus = 1;
                goto op_first_plus;
            } else if (OP(first) == MINMOD)
                sawminmod = 1;
            else if (
                   /* An OR of *one* alternative - should not happen now. */
                   (OP(first) == BRANCH && OP(first_next) != BRANCH)   ||
                   /* An {n,m} with n>0 */
                   (PL_regkind[OP(first)] == CURLY && ARG1(first) > 0) ||
                   (OP(first) == NOTHING && PL_regkind[OP(first_next)] != END )
            ) {
                /* Don't break */
            } else {
                break;
            }
                /*
                 * the only op that could be a regnode is PLUS, all the rest
                 * will be regnode_1 or regnode_2.
                 *
                 * (yves doesn't think this is true)
                 */
                first += regarglen[OP(first)];
              op_first_plus:
                first = NEXTOPER(first);
                first_next= regnext(first);            
        }

[iabyn]

On Sun, May 01, 2022 at 04:14:05PM -0700, Richard Leach wrote: Alternatively, would structuring it like this be acceptable?

Perhaps. But I think for non-buggy code it's probably best to just let sleeping dogs lie as far as possible. Can Coverity be satisfied by using double parens, as in: while ((OP(first) == OPEN && ((sawopen = 1)) ) || ...

…

-- You're only as old as you look.

[richardleach]

Can Coverity be satisfied by using double parens

I'll set up a Coverity project and find out.

[richardleach]

I haven't gotten around to setting up a Coverity project, nor am I likely to do so imminently. Almost identical other Coverity warnings have been marked as false positives in the project, so perhaps the most expedient thing to do here is do the same?

[demerphq]

On Wed, 27 Apr 2022, 07:16 Richard Leach, ***@***.***> wrote: Coverity CID 316368 raises the concern that in the following comparison: (OP(first) == OPEN && (sawopen = 1) the intention might have been to compare sawopen, not assign to it.

I think this might just need double parens on the expr. Afaik that is an established way of telling the compiler the assignment is intended. I'm guessing that the && is confusing things. Also this style is very heavily used in our regexp code, and refactoring it can be quite difficult and often results in much harder to read code. The intention genuinely is to assign to sawopen, but since there are two

…

assignments in this fashion followed by two assignments in the subsequent block, we can do a small refactor to consolidate the assignments in a way that will hopefully keep Coverity happy. ------------------------------ You can view, comment on, or merge this pull request online at: #19672 Commit Summary - 3b44327 <3b44327> re_op_compile: Coverity ASSIGN_WHERE_COMPARE_MEANT fix File Changes (1 file <https://github.com/Perl/perl5/pull/19672/files>) - *M* regcomp.c <https://github.com/Perl/perl5/pull/19672/files#diff-dc8c62daf844c4b19510dfce777053a52ac4821852ef9818e3de8912dd3c4edb> (19) Patch Links: - https://github.com/Perl/perl5/pull/19672.patch - https://github.com/Perl/perl5/pull/19672.diff — Reply to this email directly, view it on GitHub <#19672>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZ5R44CC5ZU6PWFLCSE73VHB2OFANCNFSM5UNLDM5A> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[demerphq]

Oh @iabyn already said that. My bad for replying before reading the full thread. Not enough coffee this morning. Sorry @richardleach .

[richardleach]

I've updated the PR with just additional parens as recommended.