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

[Aaron Conole]

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?

---
 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