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.kin...@intel.com>; > devel@edk2.groups.io > Cc: Feng, Bob C <bob.c.f...@intel.com>; Gao, Liming > <liming....@intel.com>; Justen, Jordan L > <jordan.l.jus...@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.f...@intel.com> > > Cc: Liming Gao <liming....@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Jordan Justen <jordan.l.jus...@intel.com> > > Signed-off-by: Michael D Kinney > <michael.d.kin...@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 <ler...@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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53198): https://edk2.groups.io/g/devel/message/53198 Mute This Topic: https://groups.io/mt/69610631/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-