# [PATCH] send-email: don't call methods on undefined values 4 messages from 2013-09-08 to 2013-09-13. Participants: brian m. carlson, Junio C Hamano. Thread: https://gitlist.dev/t/34882 ## brian m. carlson, 2013-09-08 20:54 Subject: [PATCH] send-email: don't call methods on undefined values Message-ID: <1378673674-97212-1-git-send-email-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/1378673674-97212-1-git-send-email-sandals%40crustytoothpaste.net ``` If SSL verification is enabled in git send-email, we could attempt to call a method on an undefined value if the verification failed, since $smtp would end up being undef. Look up the error string in a way that will produce a helpful error message and not cause further errors. Signed-off-by: brian m. carlson --- 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 2162478..3782c3b 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1234,7 +1234,7 @@ X-Mailer: git-send-email $gitversion if ($smtp->code == 220) { $smtp = Net::SMTP::SSL->start_SSL($smtp, ssl_verify_params()) - or die "STARTTLS failed! ".$smtp->message; + or die "STARTTLS failed! ".IO::Socket::SSL::errstr(); $smtp_encryption = ''; # Send EHLO again to receive fresh # supported commands -- 1.8.4.rc3 ``` ## Junio C Hamano, 2013-09-09 16:45 Subject: Re: [PATCH] send-email: don't call methods on undefined values Message-ID: URL: https://gitlist.dev/e/xmqqli363pwp.fsf%40gitster.dls.corp.google.com In-Reply-To: <1378673674-97212-1-git-send-email-sandals@crustytoothpaste.net> ``` "brian m. carlson" writes: > If SSL verification is enabled in git send-email, we could attempt to call a > method on an undefined value if the verification failed, since $smtp would end > up being undef. Look up the error string in a way that will produce a helpful > error message and not cause further errors. > > Signed-off-by: brian m. carlson > --- > 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 2162478..3782c3b 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -1234,7 +1234,7 @@ X-Mailer: git-send-email $gitversion > if ($smtp->code == 220) { > $smtp = Net::SMTP::SSL->start_SSL($smtp, > ssl_verify_params()) > - or die "STARTTLS failed! ".$smtp->message; > + or die "STARTTLS failed! ".IO::Socket::SSL::errstr(); I agree that $smtp->message may be bogus at this point, but could "require IO::Socket::SSL" have failed on us in ssl_verify_params? In that degraded mode, we do not do SSL peer verification, but otherwise we would still attempt to talk with the peer, and in such a case, IO::Socket::SSL would not be available to us at this point, no? > $smtp_encryption = ''; > # Send EHLO again to receive fresh > # supported commands ``` ## brian m. carlson, 2013-09-09 22:49 Subject: Re: [PATCH] send-email: don't call methods on undefined values Message-ID: <20130909224950.GA101065@vauxhall.crustytoothpaste.net> URL: https://gitlist.dev/e/20130909224950.GA101065%40vauxhall.crustytoothpaste.net In-Reply-To: ``` On Mon, Sep 09, 2013 at 09:45:10AM -0700, Junio C Hamano wrote: > "brian m. carlson" writes: > > --- a/git-send-email.perl > > +++ b/git-send-email.perl > > @@ -1234,7 +1234,7 @@ X-Mailer: git-send-email $gitversion > > if ($smtp->code == 220) { > > $smtp = Net::SMTP::SSL->start_SSL($smtp, > > ssl_verify_params()) > > - or die "STARTTLS failed! ".$smtp->message; > > + or die "STARTTLS failed! ".IO::Socket::SSL::errstr(); > > I agree that $smtp->message may be bogus at this point, but could > "require IO::Socket::SSL" have failed on us in ssl_verify_params? > In that degraded mode, we do not do SSL peer verification, but > otherwise we would still attempt to talk with the peer, and in such > a case, IO::Socket::SSL would not be available to us at this point, > no? Since Net::SMTP::SSL uses IO::Socket::SSL (in fact, it is an IO::Socket::SSL), we can be guaranteed that it is, in fact, available at this point. I guess strictly we don't need that require in IO::Socket::SSL since we'll already be guaranteed that it exists by the require of Net::SMTP::SSL. I tried using Net::SMTP::SSL::errstr() instead, but that didn't seem to produce useful output. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 ``` ## Junio C Hamano, 2013-09-13 06:09 Subject: Re: [PATCH] send-email: don't call methods on undefined values Message-ID: URL: https://gitlist.dev/e/xmqqd2odl0ba.fsf%40gitster.dls.corp.google.com In-Reply-To: <20130909224950.GA101065@vauxhall.crustytoothpaste.net> ``` "brian m. carlson" writes: > On Mon, Sep 09, 2013 at 09:45:10AM -0700, Junio C Hamano wrote: >> "brian m. carlson" writes: >> > --- a/git-send-email.perl >> > +++ b/git-send-email.perl >> > @@ -1234,7 +1234,7 @@ X-Mailer: git-send-email $gitversion >> > if ($smtp->code == 220) { >> > $smtp = Net::SMTP::SSL->start_SSL($smtp, >> > ssl_verify_params()) >> > - or die "STARTTLS failed! ".$smtp->message; >> > + or die "STARTTLS failed! ".IO::Socket::SSL::errstr(); >> >> I agree that $smtp->message may be bogus at this point, but could >> "require IO::Socket::SSL" have failed on us in ssl_verify_params? >> In that degraded mode, we do not do SSL peer verification, but >> otherwise we would still attempt to talk with the peer, and in such >> a case, IO::Socket::SSL would not be available to us at this point, >> no? > > Since Net::SMTP::SSL uses IO::Socket::SSL (in fact, it is an > IO::Socket::SSL), we can be guaranteed that it is, in fact, available at > this point. I guess strictly we don't need that require in > IO::Socket::SSL since we'll already be guaranteed that it exists by the > require of Net::SMTP::SSL. That "require" came from around $gmane/230533, which was about making sure the update to pacify newer Net::SMTP::SSL does not break folks with older packages, I think. I just didn't/don't know the history of Net::SMTP::SSL, and that was where my comments came from. As long as "Net::SMTP::SSL uses IO::Socket::SSL" has been true since the ancient past, I agree that that 'require' of the latter does not need to be guarded by an 'eval'; at that point, we are already in the Net::SMTP::SSL codepath. And your patch I replied to should be the right thing to do. Thanks for clarifying. ```