| Message ID | 20210820142742.1431762-1-frode.nordahl@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] checkpatch: Avoid a case of catastrophic backtracking | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Hi Frode, This looks good to me, so Acked-by: Mark Michelson <mmichels@redhat.com> I have one small nit below, but I don't think you need to submit a v3 of the patch. I think whoever merges this can fix this in the process: On 8/20/21 10:27 AM, Frode Nordahl wrote: > 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@canonical.com> > --- > utilities/checkpatch.py | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > index 9e8d17653..a18a26c08 100755 > --- a/utilities/checkpatch.py > +++ b/utilities/checkpatch.py > @@ -34,6 +34,7 @@ colors = False > spellcheck_comments = False > quiet = False > spell_check_dict = None > +MAX_LINE_LEN = 79 > > > def open_spell_check_dict(): > @@ -247,7 +248,7 @@ def if_and_for_whitespace_checks(line): > > > def if_and_for_end_with_bracket_check(line): > - """Return TRUE if there is not a bracket at the end of an if, for, while > + """Return TRUE if there is a bracket at the end of an if, for, while > block which fits on a single line ie: 'if (foo)'""" > > def balanced_parens(line): > @@ -264,6 +265,11 @@ def if_and_for_end_with_bracket_check(line): > if not balanced_parens(line): > return True > > + # Early return to avoid potential catastrophic backtracking in the > + # __regex_if_macros regex > + if len(line) == MAX_LINE_LEN - 1 and line[-1] == ')': > + return True > + > if __regex_ends_with_bracket.search(line) is None and \ > __regex_if_macros.match(line) is None: > return False > @@ -282,7 +288,7 @@ def pointer_whitespace_check(line): > > def line_length_check(line): > """Return TRUE if the line length is too long""" > - if len(line) > 79: > + if len(line) > MAX_LINE_LEN: > print_warning("Line is %d characters long (recommended limit is 79)" > % len(line)) This warning should use MAX_LINE_LEN instead of the hard-coded 79. > return True >
On Fri, Aug 27, 2021 at 12:18 PM Mark Michelson <mmichels@redhat.com> wrote: > > Hi Frode, > > This looks good to me, so > > Acked-by: Mark Michelson <mmichels@redhat.com> Acked-by: Michael Santana <msantana@redhat.com> > > I have one small nit below, but I don't think you need to submit a v3 of > the patch. I think whoever merges this can fix this in the process: > > On 8/20/21 10:27 AM, Frode Nordahl wrote: > > 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@canonical.com> > > --- > > utilities/checkpatch.py | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > > index 9e8d17653..a18a26c08 100755 > > --- a/utilities/checkpatch.py > > +++ b/utilities/checkpatch.py > > @@ -34,6 +34,7 @@ colors = False > > spellcheck_comments = False > > quiet = False > > spell_check_dict = None > > +MAX_LINE_LEN = 79 > > > > > > def open_spell_check_dict(): > > @@ -247,7 +248,7 @@ def if_and_for_whitespace_checks(line): > > > > > > def if_and_for_end_with_bracket_check(line): > > - """Return TRUE if there is not a bracket at the end of an if, for, while > > + """Return TRUE if there is a bracket at the end of an if, for, while > > block which fits on a single line ie: 'if (foo)'""" > > > > def balanced_parens(line): > > @@ -264,6 +265,11 @@ def if_and_for_end_with_bracket_check(line): > > if not balanced_parens(line): > > return True > > > > + # Early return to avoid potential catastrophic backtracking in the > > + # __regex_if_macros regex > > + if len(line) == MAX_LINE_LEN - 1 and line[-1] == ')': > > + return True > > + > > if __regex_ends_with_bracket.search(line) is None and \ > > __regex_if_macros.match(line) is None: > > return False > > @@ -282,7 +288,7 @@ def pointer_whitespace_check(line): > > > > def line_length_check(line): > > """Return TRUE if the line length is too long""" > > - if len(line) > 79: > > + if len(line) > MAX_LINE_LEN: > > print_warning("Line is %d characters long (recommended limit is 79)" > > % len(line)) > > This warning should use MAX_LINE_LEN instead of the hard-coded 79. > > > return True > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Sun, Aug 29, 2021 at 10:56 PM Michael Santana <msantana@redhat.com> wrote: > > On Fri, Aug 27, 2021 at 12:18 PM Mark Michelson <mmichels@redhat.com> wrote: > > > > Hi Frode, > > > > This looks good to me, so > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > Acked-by: Michael Santana <msantana@redhat.com> > > > > I have one small nit below, but I don't think you need to submit a v3 of > > the patch. I think whoever merges this can fix this in the process: > > > > On 8/20/21 10:27 AM, Frode Nordahl wrote: > > > 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@canonical.com> > > > --- > > > utilities/checkpatch.py | 10 ++++++++-- > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > > > index 9e8d17653..a18a26c08 100755 > > > --- a/utilities/checkpatch.py > > > +++ b/utilities/checkpatch.py > > > @@ -34,6 +34,7 @@ colors = False > > > spellcheck_comments = False > > > quiet = False > > > spell_check_dict = None > > > +MAX_LINE_LEN = 79 > > > > > > > > > def open_spell_check_dict(): > > > @@ -247,7 +248,7 @@ def if_and_for_whitespace_checks(line): > > > > > > > > > def if_and_for_end_with_bracket_check(line): > > > - """Return TRUE if there is not a bracket at the end of an if, for, while > > > + """Return TRUE if there is a bracket at the end of an if, for, while > > > block which fits on a single line ie: 'if (foo)'""" > > > > > > def balanced_parens(line): > > > @@ -264,6 +265,11 @@ def if_and_for_end_with_bracket_check(line): > > > if not balanced_parens(line): > > > return True > > > > > > + # Early return to avoid potential catastrophic backtracking in the > > > + # __regex_if_macros regex > > > + if len(line) == MAX_LINE_LEN - 1 and line[-1] == ')': > > > + return True > > > + > > > if __regex_ends_with_bracket.search(line) is None and \ > > > __regex_if_macros.match(line) is None: > > > return False > > > @@ -282,7 +288,7 @@ def pointer_whitespace_check(line): > > > > > > def line_length_check(line): > > > """Return TRUE if the line length is too long""" > > > - if len(line) > 79: > > > + if len(line) > MAX_LINE_LEN: > > > print_warning("Line is %d characters long (recommended limit is 79)" > > > % len(line)) > > > > This warning should use MAX_LINE_LEN instead of the hard-coded 79. Thanks for the patch. I addressed Mark's comments and applied the patch to the main branch. Numan > > > > > return True > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index 9e8d17653..a18a26c08 100755 --- a/utilities/checkpatch.py +++ b/utilities/checkpatch.py @@ -34,6 +34,7 @@ colors = False spellcheck_comments = False quiet = False spell_check_dict = None +MAX_LINE_LEN = 79 def open_spell_check_dict(): @@ -247,7 +248,7 @@ def if_and_for_whitespace_checks(line): def if_and_for_end_with_bracket_check(line): - """Return TRUE if there is not a bracket at the end of an if, for, while + """Return TRUE if there is a bracket at the end of an if, for, while block which fits on a single line ie: 'if (foo)'""" def balanced_parens(line): @@ -264,6 +265,11 @@ def if_and_for_end_with_bracket_check(line): if not balanced_parens(line): return True + # Early return to avoid potential catastrophic backtracking in the + # __regex_if_macros regex + if len(line) == MAX_LINE_LEN - 1 and line[-1] == ')': + return True + if __regex_ends_with_bracket.search(line) is None and \ __regex_if_macros.match(line) is None: return False @@ -282,7 +288,7 @@ def pointer_whitespace_check(line): def line_length_check(line): """Return TRUE if the line length is too long""" - if len(line) > 79: + if len(line) > MAX_LINE_LEN: print_warning("Line is %d characters long (recommended limit is 79)" % len(line)) return True
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@canonical.com> --- utilities/checkpatch.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)