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

CURLOPT_SSL_CTX_FUNCTION: adhere to documented behavior #1290

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Feb 26, 2017

  • Allocate a new CTX object before freeing the old one.
    This is because CURLOPT_SSL_CTX_FUNCTION manpage says:
    "pointer will be a new one every time".

  • manpage: Fix formatting in EXAMPLE by escaping all backslashes.

  • manpage: Document that CURLE_NOT_BUILT_IN is a RETURN VALUE.

@mention-bot
Copy link

@jay, thanks for your PR! By analyzing the history of the files in this pull request, we identified @captain-caveman2k, @rousskov and @bagder to be potential reviewers.

@bagder
Copy link
Member

bagder commented Mar 4, 2017

Shouldn't we just fix the docs instead? It seems unnecessary to close and re-init those handles, and probably possibly a rather time-consuming operation.

And why do we want to escape the backslashes? The intent seems to be to pass on newline separated lines, isn't it?

I don't think you should merge these independent changes into the same commit.

jay added a commit that referenced this pull request Mar 4, 2017
.. also document that CURLE_NOT_BUILT_IN is a RETURN VALUE.

Ref: #1290
@jay
Copy link
Member Author

jay commented Mar 4, 2017

Shouldn't we just fix the docs instead? It seems unnecessary to close and re-init those handles, and probably possibly a rather time-consuming operation.

Typically (edit: almost always.. don't wanna seem so breezy about it!) I lean towards supporting what is documented. For example someone may have written code expecting a new pointer every time. However at present it isn't guaranteed unless we hold open the old one before allocating the new one, otherwise the memory manager could just return the same pointer.

It could be clarified to say the contents of the object will be reinitialized every time, or something like that, but the pointer may be the same.

And why do we want to escape the backslashes? The intent seems to be to pass on newline separated lines, isn't it?

I don't think you should merge these independent changes into the same commit.

I landed this part in 21512a0, you're right I should have separated it. You are forgetting we have to escape the backslashes. foo\n is incorrect; it must be foo\\n which is then displayed as the former. If you don't escape the \ you get weird results:

input:

    "MIIHPTCCBSWgAwIBAgIBADANBgkqhkiG9w0BAQQFADB5MRAwDgYDVQQKEwdSb290\n"
    "IENBMR4wHAYDVQQLExVodHRwOi8vd3d3LmNhY2VydC5vcmcxIjAgBgNVBAMTGUNB\n"
    "IENlcnQgU2lnbmluZyBBdXRob3JpdHkxITAfBgkqhkiG9w0BCQEWEnN1cHBvcnRA\n"
    "Y2FjZXJ0Lm9yZzAeFw0wMzAzMzAxMjI5NDlaFw0zMzAzMjkxMjI5NDlaMHkxEDAO\n"
    "GCSNe9FINSkYQKyTYOGWhlC0elnYjyELn8+CkcY7v2vcB5G5l1YjqrZslMZIBjzk\n"
    "zk6q5PYvCdxTby78dOs6Y5nCpqyJvKeyRKANihDjbPIky/qbn3BHLt4Ui9SyIAmW\n"
    "omTxJBzcoTWcFbLUvFUufQb1nA5V9FrWk9p2rSVzTMVD\n"

output manpage/nroff:

           "MIIHPTCCBSWgAwIBAgIBADANBgkqhkiG9w0BAQQFADB5MRAwDgYDVQQKEwdSb2900
           "IENBMR4wHAYDVQQLExVodHRwOi8vd3d3LmNhY2VydC5vcmcxIjAgBgNVBAMTGUNB0
           "IENlcnQgU2lnbmluZyBBdXRob3JpdHkxITAfBgkqhkiG9w0BCQEWEnN1cHBvcnRA0
           "Y2FjZXJ0Lm9yZzAeFw0wMzAzMzAxMjI5NDlaFw0zMzAzMjkxMjI5NDlaMHkxEDAO0
           "GCSNe9FINSkYQKyTYOGWhlC0elnYjyELn8+CkcY7v2vcB5G5l1YjqrZslMZIBjzk0
           "zk6q5PYvCdxTby78dOs6Y5nCpqyJvKeyRKANihDjbPIky/qbn3BHLt4Ui9SyIAmW0
           "omTxJBzcoTWcFbLUvFUufQb1nA5V9FrWk9p2rSVzTMVD0

output roffit:

    "MIIHPTCCBSWgAwIBAgIBADANBgkqhkiG9w0BAQQFADB5MRAwDgYDVQQKEwdSb290n"
    "IENBMR4wHAYDVQQLExVodHRwOi8vd3d3LmNhY2VydC5vcmcxIjAgBgNVBAMTGUNBn"
    "IENlcnQgU2lnbmluZyBBdXRob3JpdHkxITAfBgkqhkiG9w0BCQEWEnN1cHBvcnRAn"
    "Y2FjZXJ0Lm9yZzAeFw0wMzAzMzAxMjI5NDlaFw0zMzAzMjkxMjI5NDlaMHkxEDAOn"
    "GCSNe9FINSkYQKyTYOGWhlC0elnYjyELn8+CkcY7v2vcB5G5l1YjqrZslMZIBjzkn"
    "zk6q5PYvCdxTby78dOs6Y5nCpqyJvKeyRKANihDjbPIky/qbn3BHLt4Ui9SyIAmWn"
    "omTxJBzcoTWcFbLUvFUufQb1nA5V9FrWk9p2rSVzTMVDn"

- Allocate a new CTX object before freeing the old one.
  This is because CURLOPT_SSL_CTX_FUNCTION manpage says:
  "pointer will be a new one every time".

Ref: curl#1290
@jay jay force-pushed the sslctx_adhere_to_doc branch from 263fe17 to e27961d Compare March 4, 2017 21:25
@jay
Copy link
Member Author

jay commented Mar 4, 2017

How about this clarification:

diff --git a/docs/libcurl/opts/CURLOPT_SSL_CTX_FUNCTION.3 b/docs/libcurl/opts/CURLOPT_SSL_CTX_FUNCTION.3
index b260126..4ec11f6 100644
--- a/docs/libcurl/opts/CURLOPT_SSL_CTX_FUNCTION.3
+++ b/docs/libcurl/opts/CURLOPT_SSL_CTX_FUNCTION.3
@@ -48,7 +48,10 @@ callback's error code. Set the \fIuserptr\fP argument with the
 \fICURLOPT_SSL_CTX_DATA(3)\fP option.
 
 This function will get called on all new connections made to a server, during
-the SSL negotiation. The SSL_CTX pointer will be a new one every time.
+the SSL negotiation. The \fIssl_ctx\fP pointer will point to an object that has
+been initialized and is new in the sense that it is the one and only time that
+object will be passed to the callback. Note the pointer may point to the same
+address in subsequent calls but it would be a different object every time.
 
 To use this properly, a non-trivial amount of knowledge of your SSL library is
 necessary. For example, you can use this function to call library-specific

@jay
Copy link
Member Author

jay commented Mar 11, 2017

ping on this, is the proposed clarification patch above an acceptable alternative to this PR?

@jay
Copy link
Member Author

jay commented Mar 22, 2017

Since the feature window was about to close and this was holding up 1272 I went ahead with that one and used a slightly less verbose explanation to explain this issue.

 \fICURLOPT_SSL_CTX_DATA(3)\fP option.
 
 This function will get called on all new connections made to a server, during
-the SSL negotiation. The SSL_CTX/mbedtls_ssl_config pointer will be a new one 
-every time.
+the SSL negotiation. The \fIssl_ctx\fP will point to a newly initialized object
+each time, but note the pointer may be the same as from a prior call.
 
 To use this properly, a non-trivial amount of knowledge of your SSL library is
 necessary. For example, you can use this function to call library-specific

@jay jay closed this Mar 22, 2017
@jay jay deleted the sslctx_adhere_to_doc branch March 22, 2017 04:01
@lock lock bot locked as resolved and limited conversation to collaborators May 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants