Re: [PATCH v2] get_maintainer: correctly parse UTF-8 encoded names in files

From: Alvin Šipraga
Date: Mon Dec 18 2023 - 20:26:38 EST

Next message: syzbot: "Re: [syzbot] [net?] [nfc?] KASAN: slab-use-after-free Read in nfc_alloc_send_skb"
Previous message: Alvin Šipraga: "[PATCH v3 0/2] get_maintainer: correctly parse UTF-8 encoded names in files"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

+Shawn, please have a look at the bottom of this mail about your patch
we would like to resend.

On Fri, Dec 15, 2023 at 10:30:52AM -0800, Joe Perches wrote:
> On Fri, 2023-12-15 at 10:30 +0000, Alvin Šipraga wrote:
> > On Thu, Dec 14, 2023 at 07:57:54AM -0800, Joe Perches wrote:
> > > On Thu, 2023-12-14 at 16:06 +0100, Alvin Šipraga wrote:
> > > > @@ -442,7 +443,7 @@ sub maintainers_in_file {
> > > > my $text = do { local($/) ; <$f> };
> > > > close($f);
> > > >
> > > > - my @poss_addr = $text =~ m$[A-Za-zÀ-ÿ\"\' \,\.\+-]*\s*[\,]*\s*[$\<\{]{0,1}[A-Za-z0-9_\.\+-]+\@[A-Za-z0-9\.-]+\.[A-Za-z0-9]+[$\>\}]{0,1}$g;
> > > > + my @poss_addr = $text =~ m$[\p{L}\"\' \,\.\+-]*\s*[\,]*\s*[$\<\{]{0,1}[A-Za-z0-9_\.\+-]+\@[A-Za-z0-9\.-]+\.[A-Za-z0-9]+[$\>\}]{0,1}$g;
> > > > push(@file_emails, clean_file_emails(@poss_addr));
>
> Hi again Alvin.
>
> Separate issue, but on the one .yaml file I tried:
>
> $ ./scripts/get_maintainer.pl Documentation/devicetree/bindings/serial/8250.yaml
> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> (supporter:TTY LAYER AND SERIAL DRIVERS)
> Jiri Slaby <jirislaby@xxxxxxxxxx> (supporter:TTY LAYER AND SERIAL DRIVERS)
> Rob Herring <robh+dt@xxxxxxxxxx> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Conor Dooley <conor+dt@xxxxxxxxxx> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Lubomir Rintel <lkundrak@xxxxx> (in file)
> - <devicetree@xxxxxxxxxxxxxxx> (in file)
> linux-kernel@xxxxxxxxxxxxxxx (open list:TTY LAYER AND SERIAL DRIVERS)
> linux-serial@xxxxxxxxxxxxxxx (open list:TTY LAYER AND SERIAL DRIVERS)
> devicetree@xxxxxxxxxxxxxxx (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
>
> Note the single '-' in the "name" portion of devicetree@xxxxxxxxxxxxxxx
>
> Maybe clean_file_emails needs some better name cleansing code.

OK, I had a look and made a patch to fix this as well. Please see v3
which is on its way to your inbox.

Regarding the .dts* patch, I need some feedback below before sending
anything, so I did not include it in the series.

>
> > > Rather than open _all_ files in utf-8, perhaps the block
> > > that opens a specific file to find maintainers
> > >
> > > sub maintainers_in_file {
> > > my ($file) = @_;
> > >
> > > return if ($file =~ m@\bMAINTAINERS$@);
> > >
> > > if (-f $file && ($email_file_emails || $file =~ /\.yaml$/)) {
> > > open(my $f, '<', $file)
> > > or die "$P: Can't open $file: $!\n";
> > > my $text = do { local($/) ; <$f> };
> > > close($f);
> > > ...
> > >
> > > should change the
> > >
> > > open(my $f...
> > > to
> > > use open qw(:std :encoding(UTF-8));
> > > open(my $f...
> >
> > Yes, this also works for parsing the name in an arbitrary file. But with the
> > change you suggest above, the script then corrupts my name when it is lifted
> > from MAINTAINERS (!?):
> >
> > $ ./scripts/get_maintainer.pl -f drivers/net/dsa/realtek/ | grep alsi
> > "Alvin Å ipraga" <alsi@xxxxxxxxxxxxxxx> (maintainer:REALTEK RTL83xx SMI DSA ROUTER CHIPS)
>
> Curious. Let me see if I can figure out why that happens.
>
>
> > If you are still unconvinced then I will gladly send a v3 patching the two cases
> > we have discussed (read_maintainer_file() and maintainers_in_file()).
>
> No rush.
>
> > > And unrelated and secondarily, perhaps the
> > > $file =~ /\.yaml$/
> > > test should be
> > > $file =~ /\.(?:yaml|dtsi?)$/
> > >
> > > to also find any maintainer address in the dts* files
> > >
> > > https://lore.kernel.org/lkml/20231028174656.GA3310672@bill-the-cat/T/
> >
> > Is this supposed to parse the "Copyright (c) 20xx John Doe <foo@xxxxxxxx>" in
> > the .dts* files?
>
> Yes, just as it would and does for .yaml files.
>
> $ git grep -P -i 'copy.*\<\w+\@\w+\.\w+\>' -- '*.yaml'
> Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml:# Copyright (C) 2019,2020 Lubomir Rintel <lkundrak@xxxxx>
> Documentation/devicetree/bindings/media/marvell,mmp2-ccic.yaml:# Copyright 2019,2020 Lubomir Rintel <lkundrak@xxxxx>
> Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.yaml:# Copyright (C) 2019,2020 Lubomir Rintel <lkundrak@xxxxx>
> Documentation/devicetree/bindings/phy/allwinner,sun50i-h6-usb3-phy.yaml:# Copyright 2019 Ondrej Jirman <megous@xxxxxxxxxx>
> Documentation/devicetree/bindings/phy/marvell,mmp3-hsic-phy.yaml:# Copyright 2019 Lubomir Rintel <lkundrak@xxxxx>
> Documentation/devicetree/bindings/phy/marvell,mmp3-usb-phy.yaml:# Copyright 2019,2020 Lubomir Rintel <lkundrak@xxxxx>
> Documentation/devicetree/bindings/reset/bitmain,bm1880-reset.yaml:# Copyright 2019 Manivannan Sadhasivam <mani@xxxxxxxxxx>
> Documentation/devicetree/bindings/reset/marvell,berlin2-reset.yaml:# Copyright 2015 Antoine Tenart <atenart@xxxxxxxxxx>
> Documentation/devicetree/bindings/reset/qca,ar7100-reset.yaml:# Copyright 2015 Alban Bedel <albeu@xxxxxxx>
> Documentation/devicetree/bindings/serial/8250.yaml:# Copyright 2020 Lubomir Rintel <lkundrak@xxxxx>
> Documentation/devicetree/bindings/spi/marvell,mmp2-ssp.yaml:# Copyright 2019,2020 Lubomir Rintel <lkundrak@xxxxx>
> Documentation/devicetree/bindings/usb/marvell,pxau2o-ehci.yaml:# Copyright 2019,2020 Lubomir Rintel <lkundrak@xxxxx>

Hmm, I ran this over the arch/ directory on all .dts* files and got some
false positives:

1355276466-18295-1-git-send-email-arve@xxxxxxxxxxx

This one comes from a lore link with a Message-ID. So deleting URLs
before parsing should fix it.

aaci_bitclk@12.288M
led@10.0
led@10.1
led@10.2
led@10.3
led@10.4
led@10.5
led@10.6
led@10.7
led@18.0
led@18.1
led@18.2
led@18.3
led@18.4
led@18.5
led@18.6
led@18.7
mxtal@19.2M
timclk@2.4M
uartclk@14.74M
xtal24.576@24.576M

These can also be avoided by strengthening the email parsing regex
to more strictly validate the TLD, which may not begin with a digit.

So something like this:

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index faa96801897a..e253053da967 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -446,7 +446,10 @@ sub maintainers_in_file {
my $text = do { local($/) ; <$f> };
close($f);

- my @poss_addr = $text =~ m$[\p{L}\"\' \,\.\+-]*\s*[\,]*\s*[$\<\{]{0,1}[A-Za-z0-9_\.\+-]+\@[A-Za-z0-9\.-]+\.[A-Za-z0-9]+[$\>\}]{0,1}$g;
+ # Avoid mistaking URLs with Message-IDs as emails
+ $text =~ s/https?:[^\s]+//g;
+
+ my @poss_addr = $text =~ m$[\p{L}\"\' \,\.\+-]*\s*[\,]*\s*[$\<\{]{0,1}[A-Za-z0-9_\.\+-]+\@[A-Za-z0-9\.-]+\.[A-Za-z][A-Za-z0-9]+[$\>\}]{0,1}$g;
push(@file_emails, clean_file_emails(@poss_addr));
}
}

>
> > But sure, I can do a resend of Shawn's original patch
> > separately if you like.
>
> Yes please. Make sure to cc Andrew Morton.

Given that some changes are needed, I want your input on how to send:

1. Should I send the above diff as a patch preceding Shawn's patch?

2. Should I send the above diff as a patch following Shawn's patch? Here
the justification is arguably more clear in the history.

3. Or should I roll it into Shawn's patch? If so, Shawn, may I have your
Signed-off-by?

Kind regards,
Alvin

Next message: syzbot: "Re: [syzbot] [net?] [nfc?] KASAN: slab-use-after-free Read in nfc_alloc_send_skb"
Previous message: Alvin Šipraga: "[PATCH v3 0/2] get_maintainer: correctly parse UTF-8 encoded names in files"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]