* [PATCH] send-email: don't call methods on undefined values @ 2013-09-08 20:54 brian m. carlson 2013-09-09 16:45 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: brian m. carlson @ 2013-09-08 20:54 UTC (permalink / raw) To: git; +Cc: artagnon, mst, gitster 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 <sandals@crustytoothpaste.net> --- 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] send-email: don't call methods on undefined values 2013-09-08 20:54 [PATCH] send-email: don't call methods on undefined values brian m. carlson @ 2013-09-09 16:45 ` Junio C Hamano 2013-09-09 22:49 ` brian m. carlson 0 siblings, 1 reply; 4+ messages in thread From: Junio C Hamano @ 2013-09-09 16:45 UTC (permalink / raw) To: brian m. carlson; +Cc: git, artagnon, mst "brian m. carlson" <sandals@crustytoothpaste.net> 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 <sandals@crustytoothpaste.net> > --- > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] send-email: don't call methods on undefined values 2013-09-09 16:45 ` Junio C Hamano @ 2013-09-09 22:49 ` brian m. carlson 2013-09-13 6:09 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: brian m. carlson @ 2013-09-09 22:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, artagnon, mst [-- Attachment #1: Type: text/plain, Size: 1469 bytes --] On Mon, Sep 09, 2013 at 09:45:10AM -0700, Junio C Hamano wrote: > "brian m. carlson" <sandals@crustytoothpaste.net> 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] send-email: don't call methods on undefined values 2013-09-09 22:49 ` brian m. carlson @ 2013-09-13 6:09 ` Junio C Hamano 0 siblings, 0 replies; 4+ messages in thread From: Junio C Hamano @ 2013-09-13 6:09 UTC (permalink / raw) To: brian m. carlson; +Cc: git, artagnon, mst "brian m. carlson" <sandals@crustytoothpaste.net> writes: > On Mon, Sep 09, 2013 at 09:45:10AM -0700, Junio C Hamano wrote: >> "brian m. carlson" <sandals@crustytoothpaste.net> 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-13 6:09 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-08 20:54 [PATCH] send-email: don't call methods on undefined values brian m. carlson 2013-09-09 16:45 ` Junio C Hamano 2013-09-09 22:49 ` brian m. carlson 2013-09-13 6:09 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).