On 11/18/2013 10:39 PM, Steve Holme wrote:
> On Sun, 17 Nov 2013, Christian Grothoff wrote:
>
>>> Wouldn't option 2 be better as a) it is more in keeping with the
>>> the existing API where the application owns the memory and 2)
>>> application can then destroy it at its own leisure - potentially
>>> isn't there a scenario where libcurl destroys the connection and
>>> the application still has a pointer to the tlsinfo address that has
>>> now been destroyed?
>>
>> Right now, the tlsinfo pointer's destination would be freed (on
>> curl_easy_cleanup), in the other case, the TLS session information
>> reachable via the application-owned tlsinfo memory would be
>> freed (on curl_easy_cleanup). Thus, in both cases doing anything
>> with that memory will lead to disaster.
>
> Exactly - neither one is great but I feel option 2 is the lesser of two
> evils so to speak.

Well, my first patch had the application own the memory.
Daniel didn't like that idea as too error-prone.
So go figure.

>>> Additionally, is the SSL backend type needed in this structure or
>>> should we consider having it separate?
>>
>> I left is separate, as otherwise we need to include the header of
>> the SSL backend in curl.h, which would be ugly given that there are
>> many different backends.
>
> Excuse my ignorance here (sorry) but I got slightly confused with your point
> there - could you please elaborate?

By declaring the type as "void *" instead of "gnutls_session_t", I can
avoid having to #include <gnutls.h>", which might be inappropriate if
curl is linked against OpenSSL. Imagine I defined

struct tlsinfo
{
  enum backend e;
  union
  {
    SSL_CTX *openssl;
    gnutls_session_t gnutls;
    NSS *nss;
    // ...
  };
};

Then you could ONLY #include that struct in your C code if you
before also included the headers for openssl, gnutls and NSS.
With 6 (?) optional SSL backends for curl -- some of which may
not even be available on all platforms -- this gets rather messy.

Yes, you could also just 'typedef struct gnutls_session * gnutls_session_t'
instead of including the full header, but I don't think that's any
more pretty. Just using 'void *' instead "always" works for the
compiler, at the expense of conveying minimally less type information.
But this is C after all... YMMV.

>> You are assuming that there will always be a 1:1 correspondence
>> between SSL backends and returned void *-types. However, it might
>> be that at some point we decide to return a different type depending
>> on say the OpenSSL version in use (say OpeenSSL v2 offers a completely
>> different kind of session handle). Then, with the enum, we can indicate
>> this change easily by defining a second enum value for OpenSSL2, while
>> possibly still returning the same name for the backend in curl_version
>> and (if it is added) CURLINFO_SSL_BACKEND. I also don't like having
>> another option just to get the backend, but that's a lesser reason for me.
>
> Partly because we have a 1:1 relationship at the moment... but even if we
> add OpenSSL2 to the enum, wouldn't CURLINFO_SSL_BACKEND still return that
> enum (and as such "OpenSSL2") whilst CURLINFO_TLS_SESSION would return a
> pointer to a void* that would then point to the new tlsinfo2 structure?

My point was that we might want to keep returning the "old" backend value
from CURLINFO_SSL_BACKEND. In essence, the 'enum' in the struct tlsinfo
simply only serves as the discriminant in a tagged union (the union
here being the 'void *'), and as such should not be overloaded with
uses elsewhere.

> Even if CURLINFO_TLS_SESSION returned a pointer to the proposed tlsinfo
> structure, that would then need modifying for the second handle, or whatever
> it may be, and that would break ABI would it? I hope I haven't misunderstood
> you there.
>
> If that is the case then alternatively we could have either a) a union
> contained within the structure containing each of the backend specific
> variables or b) separate variables for each of the backends rather than a
> simple "internals" (for example: ctx, session, handle, etc...). Both would
> allow us to add to the structure at some point in the future.

Yes, we could do the union thing, but I don't see a way to keep this simple
and build nicely given that the types for the members of the union would be
scattered over the various SSL backend #include files, and we can hardly
include them all. Including only the "available" subset would mean patching
the curl.h header depending on the build system and introduce binary
compatibility issues simply because the C compiler might choose a different
layout for the union depending on which of the conditionally defined members
happen to be around. So yes, you can do this, but are you fully aware of
all of the possible consequences if you did this? I considered it, and
figured 'void *' was simple enough to use in practice.

> If we went down this route, then I can see a reason for having the backend
> as a member variable but I would still recommend that we have a lightweight
> CURLINFO_SSL_BACKEND option so an application can easily find out what the
> backend is without having to use CURLINFO_TLS_SESSION.

Why? It's not like calling CURLINFO_TLS_SESSION is expensive -- getting
the enum + void * back vs. just getting an enum back hardly justifies
another
option, especially given that the operation is virtually free either way.

> Thanks for your efforts so far and sorry if I have made things more
> complicated ;-)

No problem.

Happy hacking!

Christian
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2013-11-19