[ovs-dev] [PATCH ovn] checkpatch: Avoid a case of catastrophic backtracking

[Frode Nordahl]

On Thu, Sep 2, 2021 at 4:04 PM Aaron Conole <aconole at redhat.com> wrote:
>
> Hi Frode (et. al.),
>
> Frode Nordahl writes:
>
> > The checkpatch script will hang forever on line 282 of a otherwise
> > valid patch [0].  While the root of the issue is probably
> > somewhere between the regex itself and the Python re
> > implementation, this patch provides an early return to avoid this
> > specific issue.
> >
> > Also fix the docstring for the if_and_for_end_with_bracket_check
> > function.
> >
> > 0: https://patchwork.ozlabs.org/project/ovn/patch/20210819110857.2229769-8-frode.nordahl@canonical.com/
> > Signed-off-by: Frode Nordahl <frode.nordahl at canonical.com>
> > ---
>
> Sorry, I was on PTO when this patch came in and missed it.  I don't
> agree with your proposed fix.  It's a bit too conditional (and I was
> able to build a test-case that breaks it anyway).
>
> I see it's already been applied to OVN, but I plan to propose the
> following for OVS repository (and suggest a similar change to OVN).
>
> The change should be safe because we already validate that we are only
> in a for/if/while block, so the early bail to True should be okay.
>
> WDYT?

No worries, I had to propose something as apparently I was clogging up
the CI, which is not nice :-) Your approach does look more generally
useful indeed and I would have no issue with having mine replaced by
it.

However, it does end up with the opposite verdict on the line in
question, was that intentional? From my reading of the test, it checks
that when there is room for a bracket at the end of a line for a
conditional control block it should be there, does that not mean we
can put it on the next line when there is not room?

-- 
Frode Nordahl

> ---
>  tests/checkpatch.at     | 16 ++++++++++++++++
>  utilities/checkpatch.py | 10 +++++++---
>  2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
> index 0718acd995..f382149163 100755
> --- a/tests/checkpatch.at
> +++ b/tests/checkpatch.at
> @@ -232,6 +232,22 @@ done
>  AT_CLEANUP
>
>
> +AT_SETUP([checkpatch - catastrophic backtracking])
> +dnl Special case this rather than using the above construct because sometimes a
> +dnl warning needs to be generated for line lengths (f.e. when the 'while'
> +dnl keyword is used).
> +try_checkpatch \
> +   "COMMON_PATCH_HEADER
> +    +     if (!b_ctx_in->chassis_rec || !b_ctx_in->br_int || !b_ctx_in->ovs_idl_txn)
> +    " \
> +    "ERROR: Inappropriate bracing around statement
> +    #8 FILE: A.c:1:
> +         if (!b_ctx_in->chassis_rec || !b_ctx_in->br_int || !b_ctx_in->ovs_idl_txn)
> +"
> +
> +AT_CLEANUP
> +
> +
>  AT_SETUP([checkpatch - parenthesized constructs - for])
>  try_checkpatch \
>     "COMMON_PATCH_HEADER
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 699fb4b027..16f46c78e8 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -274,9 +274,13 @@ def if_and_for_end_with_bracket_check(line):
>          if not balanced_parens(line):
>              return True
>
> -        if __regex_ends_with_bracket.search(line) is None and \
> -           __regex_if_macros.match(line) is None:
> -            return False
> +        if __regex_ends_with_bracket.search(line) is None:
> +            if line.endswith("\\") and \
> +               __regex_if_macros.match(line) is not None:
> +                return True
> +            else:
> +                return False
> +
>      if __regex_conditional_else_bracing.match(line) is not None:
>          return False
>      if __regex_conditional_else_bracing2.match(line) is not None:
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev