| Message ID | 20210820050821.3390062-1-frode.nordahl@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] 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 |
On Fri, Aug 20, 2021 at 1:09 AM Frode Nordahl <frode.nordahl@canonical.com> 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. Thank you for catching this. We hit a snag last week where our CI got stuck on checkpatch.py for a couple of days without anyone noticing. I didn't look into it but I hope this is the cause and this patch fixes it. > > 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..7c316267f 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 FALSE if there is not a bracket at the end of an if, for, while Nit-picking: double negatives can be confusing s/FALSE/TRUE/ s/not // > 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 > + Why not >= ? > 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 > -- > 2.32.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Aug 20, 2021 at 1:43 PM Michael Santana <msantana@redhat.com> wrote: > > On Fri, Aug 20, 2021 at 1:09 AM Frode Nordahl > <frode.nordahl@canonical.com> 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. > Thank you for catching this. We hit a snag last week where our CI got > stuck on checkpatch.py for a couple of days without anyone noticing. I > didn't look into it but I hope this is the cause and this patch fixes > it. You're very welcome, and apologies if I unintentionally caused any issues for the CI. > > > > 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..7c316267f 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 FALSE if there is not a bracket at the end of an if, for, while > Nit-picking: double negatives can be confusing > s/FALSE/TRUE/ > s/not // Ah, thank you for clearing that up, I did stare at that docstring for a while trying to figure out what was meant. > > 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 > > + > Why not >= ? The premise of the check is that if you have room for a bracket on a line with a statement, it should be there. From that I gather that if there is no room you're allowed to put the bracket on the next line, which is the case when you use exactly MAX_LINE_LEN -1 characters for indentation + statement. The catastrophic backtrack appears to occur exactly in this situation when it's not a macro. If the length of the line is >= MAX_LINE_LEN it should be flagged as an error by the line_length_check function.
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index 9e8d17653..7c316267f 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 FALSE if there is not 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(-)