# [PATCH] send-email: fix uninitialized var warning for $smtp_auth 6 messages from 2015-09-18 to 2015-09-21. Participants: Brian Norris, Eric Sunshine, Junio C Hamano. Thread: https://gitlist.dev/t/40383 ## Brian Norris, 2015-09-18 22:12 Subject: [PATCH] send-email: fix uninitialized var warning for $smtp_auth Message-ID: <1442614370-39880-1-git-send-email-computersforpeace@gmail.com> URL: https://gitlist.dev/e/1442614370-39880-1-git-send-email-computersforpeace%40gmail.com ``` On the latest version of git-send-email, I see this error just before running SMTP auth (I didn't provide any --smtp-auth= parameter): Use of uninitialized value $smtp_auth in pattern match (m//) at /usr/local/google/home/briannorris/git/git/git-send-email.perl line 1139. Signed-off-by: Brian Norris Cc: Jan Viktorin Cc: Eric Sunshine Cc: Junio C Hamano --- git-send-email.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-send-email.perl b/git-send-email.perl index c5a3f766f7fd..e3ff44b4d0cd 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1136,7 +1136,7 @@ sub smtp_auth_maybe { # Check mechanism naming as defined in: # https://tools.ietf.org/html/rfc4422#page-8 - if ($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { + if ($smtp_auth && $smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { die "invalid smtp auth: '${smtp_auth}'"; } -- 2.6.0.rc0.131.gf624c3d ``` ## Eric Sunshine, 2015-09-18 22:25 Subject: Re: [PATCH] send-email: fix uninitialized var warning for $smtp_auth Message-ID: <20150918222524.GA22410@flurp.local> URL: https://gitlist.dev/e/20150918222524.GA22410%40flurp.local In-Reply-To: <1442614370-39880-1-git-send-email-computersforpeace@gmail.com> ``` On Fri, Sep 18, 2015 at 03:12:50PM -0700, Brian Norris wrote: > On the latest version of git-send-email, I see this error just before > running SMTP auth (I didn't provide any --smtp-auth= parameter): > > Use of uninitialized value $smtp_auth in pattern match (m//) at /usr/local/google/home/briannorris/git/git/git-send-email.perl line 1139. > > Signed-off-by: Brian Norris > --- > git-send-email.perl | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git-send-email.perl b/git-send-email.perl > index c5a3f766f7fd..e3ff44b4d0cd 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -1136,7 +1136,7 @@ sub smtp_auth_maybe { > > # Check mechanism naming as defined in: > # https://tools.ietf.org/html/rfc4422#page-8 > - if ($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { > + if ($smtp_auth && $smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { > die "invalid smtp auth: '${smtp_auth}'"; > } Thanks, makes sense. I wonder if moving the check to the point where $smtp_auth is actually used (despite the noisier diff) would be cleaner, like this: --- 8< --- diff --git a/git-send-email.perl b/git-send-email.perl index c5a3f76..2a5ceda 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1134,12 +1134,6 @@ sub smtp_auth_maybe { Authen::SASL->import(qw(Perl)); }; - # Check mechanism naming as defined in: - # https://tools.ietf.org/html/rfc4422#page-8 - if ($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { - die "invalid smtp auth: '${smtp_auth}'"; - } - # TODO: Authentication may fail not because credentials were # invalid but due to other reasons, in which we should not # reject credentials. @@ -1154,6 +1148,12 @@ sub smtp_auth_maybe { my $cred = shift; if ($smtp_auth) { + # Check mechanism naming as defined in: + # https://tools.ietf.org/html/rfc4422#page-8 + if ($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { + die "invalid smtp auth: '${smtp_auth}'"; + } + my $sasl = Authen::SASL->new( mechanism => $smtp_auth, callback => { --- 8< --- ``` ## Brian Norris, 2015-09-18 22:42 Subject: Re: [PATCH] send-email: fix uninitialized var warning for $smtp_auth Message-ID: <20150918224242.GA45466@google.com> URL: https://gitlist.dev/e/20150918224242.GA45466%40google.com In-Reply-To: <20150918222524.GA22410@flurp.local> ``` On Fri, Sep 18, 2015 at 06:25:24PM -0400, Eric Sunshine wrote: > On Fri, Sep 18, 2015 at 03:12:50PM -0700, Brian Norris wrote: > > --- a/git-send-email.perl > > +++ b/git-send-email.perl > > @@ -1136,7 +1136,7 @@ sub smtp_auth_maybe { > > > > # Check mechanism naming as defined in: > > # https://tools.ietf.org/html/rfc4422#page-8 > > - if ($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { > > + if ($smtp_auth && $smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { > > die "invalid smtp auth: '${smtp_auth}'"; > > } > > Thanks, makes sense. I wonder if moving the check to the point where > $smtp_auth is actually used (despite the noisier diff) would be cleaner, > like this: > > --- 8< --- > diff --git a/git-send-email.perl b/git-send-email.perl > index c5a3f76..2a5ceda 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -1134,12 +1134,6 @@ sub smtp_auth_maybe { > Authen::SASL->import(qw(Perl)); > }; > > - # Check mechanism naming as defined in: > - # https://tools.ietf.org/html/rfc4422#page-8 > - if ($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { > - die "invalid smtp auth: '${smtp_auth}'"; > - } > - > # TODO: Authentication may fail not because credentials were > # invalid but due to other reasons, in which we should not > # reject credentials. > @@ -1154,6 +1148,12 @@ sub smtp_auth_maybe { > my $cred = shift; > > if ($smtp_auth) { > + # Check mechanism naming as defined in: > + # https://tools.ietf.org/html/rfc4422#page-8 > + if ($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { > + die "invalid smtp auth: '${smtp_auth}'"; > + } > + > my $sasl = Authen::SASL->new( > mechanism => $smtp_auth, > callback => { > --- 8< --- By moving the patch into the sub-subroutine (is this a lambda? I'm not too familiar with my perl), I think you change the order of the checks. So, previously, initial password auth would happen after the $smtp_auth format check. With your patch, it comes first. My patch: $ git send-email --smtp-auth="@" ... ... Send this email? ([y]es|[n]o|[q]uit|[a]ll): y invalid smtp auth: '@' at ./git-send-email.perl line 1140. Your patch: $ git send-email --smtp-auth="@" ... ... Send this email? ([y]es|[n]o|[q]uit|[a]ll): y Password for xxxx: invalid smtp auth: '@' at ./git-send-email.perl line 1155. Seems like the former is a little better, so you don't have to waste time with your password too many times. Regards, Brian ``` ## Eric Sunshine, 2015-09-18 22:47 Subject: Re: [PATCH] send-email: fix uninitialized var warning for $smtp_auth Message-ID: URL: https://gitlist.dev/e/CAPig%2BcR%2BW0Kt2y%2BvspS5dF6FHU%3DXLWZXJR46r7Rv4JP4gNiKTA%40mail.gmail.com In-Reply-To: <20150918224242.GA45466@google.com> ``` On Fri, Sep 18, 2015 at 6:42 PM, Brian Norris wrote: > On Fri, Sep 18, 2015 at 06:25:24PM -0400, Eric Sunshine wrote: >> Thanks, makes sense. I wonder if moving the check to the point where >> $smtp_auth is actually used (despite the noisier diff) would be cleaner, > > By moving the patch into the sub-subroutine (is this a lambda? I'm not > too familiar with my perl), I think you change the order of the checks. > So, previously, initial password auth would happen after the > $smtp_auth format check. With your patch, it comes first. > > My patch: > > $ git send-email --smtp-auth="@" ... > ... > Send this email? ([y]es|[n]o|[q]uit|[a]ll): y > invalid smtp auth: '@' at ./git-send-email.perl line 1140. > > Your patch: > > $ git send-email --smtp-auth="@" ... > ... > Send this email? ([y]es|[n]o|[q]uit|[a]ll): y > Password for xxxx: > invalid smtp auth: '@' at ./git-send-email.perl line 1155. > > Seems like the former is a little better, so you don't have to waste > time with your password too many times. Makes sense (again). Thanks for the explanation. ``` ## Brian Norris, 2015-09-18 22:50 Subject: Re: [PATCH] send-email: fix uninitialized var warning for $smtp_auth Message-ID: <20150918225009.GA46959@google.com> URL: https://gitlist.dev/e/20150918225009.GA46959%40google.com In-Reply-To: ``` On Fri, Sep 18, 2015 at 06:47:20PM -0400, Eric Sunshine wrote: > Makes sense (again). Thanks for the explanation. No problem. Thanks for the review. ``` ## Junio C Hamano, 2015-09-21 17:52 Subject: Re: [PATCH] send-email: fix uninitialized var warning for $smtp_auth Message-ID: URL: https://gitlist.dev/e/xmqqr3lracd7.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1442614370-39880-1-git-send-email-computersforpeace@gmail.com> ``` Brian Norris writes: > On the latest version of git-send-email, I see this error just before > running SMTP auth (I didn't provide any --smtp-auth= parameter): > > Use of uninitialized value $smtp_auth in pattern match (m//) at /usr/local/google/home/briannorris/git/git/git-send-email.perl line 1139. > > Signed-off-by: Brian Norris > Cc: Jan Viktorin > Cc: Eric Sunshine > Cc: Junio C Hamano > --- > git-send-email.perl | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git-send-email.perl b/git-send-email.perl > index c5a3f766f7fd..e3ff44b4d0cd 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -1136,7 +1136,7 @@ sub smtp_auth_maybe { > > # Check mechanism naming as defined in: > # https://tools.ietf.org/html/rfc4422#page-8 > - if ($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { > + if ($smtp_auth && $smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) { > die "invalid smtp auth: '${smtp_auth}'"; > } Thanks. ```