New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some OpenSSL issues #4875
Some OpenSSL issues #4875
Conversation
ssl_close uses this boolean to know if SSL_shutdown should be called. It turns out OpenSSL auto-shutdowns on failure, so if the call to SSL_connect fails, it will complain about "shutdown while in init", trampling the original error.
/rebuild |
Okay, @pks-t, I started to rebuild this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks! Waiting for CI, as the first run failed
@@ -373,10 +373,10 @@ static int ssl_set_error(SSL *ssl, int error) | |||
switch (err) { | |||
case SSL_ERROR_WANT_CONNECT: | |||
case SSL_ERROR_WANT_ACCEPT: | |||
giterr_set(GITERR_NET, "SSL error: connection failure"); | |||
giterr_set(GITERR_SSL, "SSL error: connection failure"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these changes make sense. The other secure streams also use GITERR_SSL
for various SSL-related failures
@@ -602,6 +600,8 @@ int openssl_connect(git_stream *stream) | |||
if ((ret = SSL_connect(st->ssl)) <= 0) | |||
return ssl_set_error(st->ssl, ret); | |||
|
|||
st->connected = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, too. One might wonder about whether the BIO will still get free'd if we never call SSL_shutdown
. But documentation of SSL_set_bio
states that BIO's ownership is transferred to the SSL handle and will get free'd with BIO_free_all
as soon the SSL handle is free'd.
Thanks! |
@ethomson Though it's minor, also next-release (unless it was backported).
|
Reading #4644 was eerily familiar, so it made me start looking for "that patch I had written but now I can't find it" 馃槈.
This is extracted from #4786, I'm not sure if it is the complete story, but at least it would help downstream (and us) to pinpoint the reason for those failures.