Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing attached easy handles from OpenSSL SSL instances. Refs #10150. #10151

Closed
wants to merge 1 commit into from

Conversation

icing
Copy link
Contributor

@icing icing commented Dec 23, 2022

  • keeping the "current" easy handle registered at SSL* is no longer necessary, since the "calling" data object is already stored in the cfilter's context (and used by other SSL backends from there).
  • The "detach" of an easy handle that goes out of scope is then avoided.
  • This could be the cause of the crash reported in 7.87.0: core dump in curl_easy_cleanup #10150.

@bagder bagder linked an issue Dec 23, 2022 that may be closed by this pull request
@icing icing added not-a-bug This is not a bug in curl and removed not-a-bug This is not a bug in curl labels Dec 27, 2022

#else
/* old style, with some weird backward compat handling,
* we prefer the more controlled case above. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"old" here seems to be before 1.1.0 (according to https://www.openssl.org/docs/man1.1.1/man3/SSL_set0_rbio.html) - maybe that can be mentioned here as it might be useful at some point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, also libressl added it in some of its versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote the comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strangely, latest LibreSSL (3.7.0) has SSL_set0_rbio, but not SSL_set0_wbio. (it has this instead: BIO * SSL_get_wbio(const SSL *s);)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds very cautious of them.

…#10150.

    - keeping the "current" easy handle registered at SSL* is no longer
      necessary, since the "calling" data object is already stored in the
      cfilter's context (and used by other SSL backends from there).
    - The "detach" of an easy handle that goes out of scope is then avoided.
    - This could be the cause of the crash reported in curl#10150.
    - using SSL_set0_wbio for clear reference counting where available.
@bagder bagder closed this in f39472e Dec 28, 2022
@bagder
Copy link
Member

bagder commented Dec 28, 2022

Thanks!

vszakats added a commit to curl/curl-for-win that referenced this pull request Dec 28, 2022
An upcoming curl 7.88.0 build option that is auto-detected with
autotools, but not with CMake or GNU Make.

Cursory tests indicate that BoringSSL and OpenSSL supports this,
but not LibreSSL and wolfSSL.

Untested.

Ref: curl/curl@f39472e
Ref: curl/curl#10151
@@ -2674,7 +2644,7 @@ static void ossl_trace(int direction, int ssl_ver, int content_type,
connssl = cf->ctx;
DEBUGASSERT(connssl);
DEBUGASSERT(connssl->backend);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7.87.0: core dump in curl_easy_cleanup
4 participants