Re: [edk2-devel] [Patch] BaseTools/Scripts/PatchCheck: Address false error conditions

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Laszlo,

This patch has been pushed.

    https://github.com/tianocore/edk2/pull/297

I also entered a new BZ to relax CVE pattern check.

    https://bugzilla.tianocore.org/show_bug.cgi?id=2462

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> Sent: Monday, January 13, 2020 9:32 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>
> Subject: Re: [edk2-devel] [Patch]
> BaseTools/Scripts/PatchCheck: Address false error
> conditions
> 
> Hello Mike,
> 
> thanks a lot for this patch!
> 
> Comments below:
> 
> On 01/10/20 23:53, Michael D Kinney wrote:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=2406
> >
> > * Always print subject line after the git commit id
> to make
> >   it easier to know the context of warnings or
> errors.
> > * Allow UTF-8 characters in subject line
> > * Error if subject line length > 75 without CVE-xxx-
> xxxxx present
> > * Error if subject line length > 92 with CVE-xxxx-
> xxxxx present
> > * If body line length is > 75, then print warning
> instead of error.
> >
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> > ---
> >  BaseTools/Scripts/PatchCheck.py | 57
> +++++++++++++++++++++++++++++----
> >  1 file changed, 51 insertions(+), 6 deletions(-)
> >
> > diff --git a/BaseTools/Scripts/PatchCheck.py
> b/BaseTools/Scripts/PatchCheck.py
> > index 9668025798..ff4572bb7c 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -1,7 +1,7 @@
> >  ## @file
> >  #  Check a patch for various format issues
> >  #
> > -#  Copyright (c) 2015 - 2019, Intel Corporation. All
> rights reserved.<BR>
> > +#  Copyright (c) 2015 - 2020, Intel Corporation. All
> rights reserved.<BR>
> >  #
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> >  #
> > @@ -35,6 +35,8 @@ class CommitMessageCheck:
> >          self.subject = subject
> >          self.msg = message
> >
> > +        print (subject)
> > +
> >          self.check_contributed_under()
> >          self.check_signed_off_by()
> >          self.check_misc_signatures()
> > @@ -179,6 +181,8 @@ class CommitMessageCheck:
> >          for sig in self.sig_types:
> >              self.find_signatures(sig)
> >
> > +    cve_re = re.compile('CVE-[0-9]{4}-[0-9]{5}[^0-
> 9]')
> > +
> 
> (1) I suggest we permit CVE IDs with 4 arbitrary digits
> too. That is:
> 
>   CVE-[0-9]{4}-[0-9]{4,5}[^0-9]
>                     ^^^^^
> 
> Because of:
> 
> 
> https://cve.mitre.org/cve/identifiers/syntaxchange.html
> #new
> 
> For example, "CVE-2020-0001" is an existent CVE:
> 
>   https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-
> 2020-0001
> 
> and edk2 could get a CVE assigned for which only 4
> "arbitrary digits"
> were necessary. (In that case, the CNA would not pad
> that sequence to 5
> digits.)
> 
> Now, I wouldn't like to overcomplicate this, therefore
> I'm not
> requesting that in such cases, we also lower the
> inclusive limit from 92
> to 91 characters, in the subject line.
> 
> (I think that this suggested update to the script/patch
> does not require
> a reposting. Another comment below.)
> 
> 
> >      def check_overall_format(self):
> >          lines = self.msg.splitlines()
> >
> > @@ -196,9 +200,26 @@ class CommitMessageCheck:
> >              self.error('Empty commit message!')
> >              return
> >
> > -        if count >= 1 and len(lines[0].rstrip()) >=
> 72:
> > -            self.error('First line of commit message
> (subject line) ' +
> > -                       'is too long.')
> > +        if count >= 1 and re.search(self.cve_re,
> lines[0]):
> > +            #
> > +            # If CVE-xxxx-xxxxx is present in
> subject line, then limit length of
> > +            # subject line to 92 characters
> > +            #
> > +            if len(lines[0].rstrip()) >= 93:
> > +                self.error(
> > +                    'First line of commit message
> (subject line) is too long (%d >= 93).' %
> > +                    (len(lines[0].rstrip()))
> > +                    )
> > +        else:
> > +            #
> > +            # If CVE-xxxx-xxxxx is not present in
> subject line, then limit
> > +            # length of subject line to 75
> characters
> > +            #
> > +            if len(lines[0].rstrip()) >= 76:
> > +                self.error(
> > +                    'First line of commit message
> (subject line) is too long (%d >= 76).' %
> > +                    (len(lines[0].rstrip()))
> > +                    )
> >
> >          if count >= 1 and len(lines[0].strip()) ==
> 0:
> >              self.error('First line of commit message
> (subject line) ' +
> > @@ -212,7 +233,14 @@ class CommitMessageCheck:
> >              if (len(lines[i]) >= 76 and
> >                  len(lines[i].split()) > 1 and
> >                  not lines[i].startswith('git-svn-
> id:')):
> > -                self.error('Line %d of commit
> message is too long.' % (i + 1))
> > +                #
> > +                # Print a warning if body line is
> longer than 75 characters
> > +                #
> > +                print(
> > +                    'WARNING - Line %d of commit
> message is too long (%d >= 76).' %
> > +                    (i + 1, len(lines[i]))
> > +                    )
> > +                print(lines[i])
> >
> >          last_sig_line = None
> >          for i in range(count - 1, 0, -1):
> > @@ -503,8 +531,25 @@ class CheckOnePatch:
> >          else:
> >              self.stat = mo.group('stat')
> >              self.commit_msg =
> mo.group('commit_message')
> > +        #
> > +        # Parse subject line from email header.  The
> subject line may be
> > +        # composed of multiple parts with different
> encodings.  Decode and
> > +        # combine all the parts to produce a single
> string with the contents of
> > +        # the decoded subject line.
> > +        #
> > +        parts =
> email.header.decode_header(pmail.get('subject'))
> > +        subject = ''
> > +        for (part, encoding) in parts:
> > +            if encoding:
> > +                part = part.decode(encoding)
> > +            else:
> > +                try:
> > +                    part = part.decode()
> > +                except:
> > +                    pass
> > +            subject = subject + part
> >
> > -        self.commit_subject =
> pmail['subject'].replace('\r\n', '')
> > +        self.commit_subject =
> subject.replace('\r\n', '')
> >          self.commit_subject =
> self.commit_subject.replace('\n', '')
> >          self.commit_subject =
> self.subject_prefix_re.sub('', self.commit_subject, 1)
> >
> >
> 
> So I've applied this patch locally, such that the most
> recent prior
> patch for "BaseTools/Scripts/PatchCheck.py" has been
> commit 2649a735b249
> ("BaseTools/PatchCheck.py: Ignore CR and LF characters
> in subject
> length", 2020-01-09). And I've attempted to test it
> against two patch
> sets I have pending on the list:
> 
> [edk2-devel] [PATCH 0/2]
> UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S)
> downloads
> 
> [edk2-devel] [PATCH]
> UefiCpuPkg/PiSmmCpuDxeSmm: fix 2M->4K page splitting
> regression for PDEs
> 
> (2) The update seems to break python2 compatibility:
> 
> > Traceback (most recent call last):
> >   File "BaseTools/Scripts/PatchCheck.py", line 688,
> in <module>
> >     sys.exit(PatchCheckApp().retval)
> >   File "BaseTools/Scripts/PatchCheck.py", line 648,
> in __init__
> >     self.process_one_arg(patch)
> >   File "BaseTools/Scripts/PatchCheck.py", line 665,
> in process_one_arg
> >     self.ok &= CheckOneArg(arg, self.count).ok
> >   File "BaseTools/Scripts/PatchCheck.py", line 632,
> in __init__
> >     checker = CheckGitCommits(param, max_count)
> >   File "BaseTools/Scripts/PatchCheck.py", line 576,
> in __init__
> >     self.ok &= CheckOnePatch(commit, patch).ok
> >   File "BaseTools/Scripts/PatchCheck.py", line 451,
> in __init__
> >     self.find_patch_pieces()
> >   File "BaseTools/Scripts/PatchCheck.py", line 540,
> in find_patch_pieces
> >     parts =
> email.header.decode_header(pmail.get('subject'))
> > AttributeError: 'module' object has no attribute
> 'header'
> 
> However, the patch works very well with python3.
> 
> I've also checked a local one-off commit with a bunch
> of UTF-8 sequences
> in the subject line. (In Hungarian we have the
> nonsensical phrase
> "árvíztűrő tükörfúrógép ÁRVÍZTŰRŐ TÜKÖRFÚRÓGÉP" for
> easily testing all
> the Hungarian diacritical marks, which are part of
> latin2. Verbatim, it
> means "flood tolerant mirror drill" :) )
> 
> In order to unblock some pending patches wrt. the edk2
> CI system, I'd
> suggest pushing this patch at once. For that:
> 
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> Then, the regex improvement for (1) could be a
> separate, future patch.
> (Normally I'd propose implementing that small change
> right before
> pushing, but then my Tested-by would not be valid any
> longer.
> "Reviewed-by" can survive small changes, but "Tested-
> by" can't.)
> 
> Similarly, python2 support (2) (*if* we care -- not
> saying we should
> necessarily care) could be added as a subsequent patch.
> 
> In summary I'd be thankful if the patch could be pushed
> as-is, with my
> Tested-by (and with an R-b from a BaseTools maintainer,
> of course).
> 
> Thank you!
> Laszlo
> 
> 
>