Re: PQinitSSL broken in some use casesf - Mailing list pgsql-hackers

Merlin Moncure wrote:
> On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > Merlin Moncure wrote:
> >> > PQinitSSL(0) was specifically designed to allow applications to set up
> >> > SSL on their own.  How does this not work properly?
> >>
> >> this has nothing to do with who initializes ssl.  this is all about
> >> *crypto*.  remember,  crypto and ssl are two separate libraries.  The
> >> application or library in question may not even link with ssl or use
> >> ssl headers.
> >>
> >> The problem is PQinitSSL (re-) initializes crypto without asking if that's ok.
> >
> > PQinitSSL(false) initializes crypto?  Please point me to exact function
> > calls that are the problem?  Everything is very vague.
>
> nooo, you are not listening :-)
>
> PQinitSSL(0) initializes libpq for ssl but leaves crypto and ssl
> initialization to the app
> PQinitSSL(1) initializes libpq, crypto, and ssl libraries
>
> Now, consider an app that uses libcrypto for its own requirements *but
> not libssl*.  It initializes libcrypto, passing its own lock vector,
> etc.  It cannot however initialize ssl because it does not link with
> ssl, or include ssl headers.  There are no ssl functions to call, and
> it wouldn't make sense to expect the app to do this even if there
> were.
>
> Now, if this app also has libpq dependency, it needs a way to tell
> libpq: 'i have already initialized the crypto library, but could you
> please set up libssl'.  otherwise you end up re-initializing libcrypto
> with different lock vector which is very bad if there are any locks
> already in use, which is quite likely.
>
> There is no way to do that with libpq....so you see that no matter how
> you call PQinitSSL, the application is broken in some way.  Passing 0
> breaks because ssl never ends up getting set up, and passing 1 breaks
> because libcrypto's locks get messed up.
>
> The main problem is that libpq PQinitSSL makes broad (and extremely
> dangerous assumption) that it is the only one interested in libcrypto
> lock vector.  In short, it's broken.

I am back to looking at this.  I dropped off this discussion back in
February because I felt people didn't want to answer questions I had,
but now it seems we have to close this out somehow.

I have applied the attached patch which does several things:

    o  documents that libssl _and_ libcrypto initialization is
       turned off by PQinitSSL(0)
    o  clarified cases where this behavior is important
    o  added comments that the CRYPTO_set_* calls reference
       libcrypto, not libssl

I think we can now say that the current behavior is not a bug because it
is documented, even though the PQinitSSL() function name is inaccurate.

In fact, 8.4 is the first time we are documenting the valid parameter
value to PQinitSSL(), in 8.3 we have:

   to inside <application>libpq</application>), you can use
   <function>PQinitSSL(int)</> to tell <application>libpq</application>
   that the <acronym>SSL</> library has already been initialized by your
   application.

So we have some flexibility in defining how it behaves, and this also
illustrates how little the function is used because no one ever
complained about it.

I think there is a good argument that PQinitSSL(X) where X > 1 would
work fine for more fine-grained control.  The new libpq init function
idea was interesting, but having a documented solution for
WSAStartup()/WSACleanup() usage, we now don't have another libpq init
use-case so it is hard to suggest a new libpq function.

I am figuring we have to keep the current behavior and see what happens
after 8.4;  the new documentation should make the behavior clear and
perhaps trigger other users to report suggestions.

I assume this item is closed for 8.4 unless I hear otherwise.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.279
diff -c -c -r1.279 libpq.sgml
*** doc/src/sgml/libpq.sgml    23 Mar 2009 01:45:29 -0000    1.279
--- doc/src/sgml/libpq.sgml    28 Mar 2009 01:17:21 -0000
***************
*** 6169,6179 ****
    </para>

    <para>
!    If you are using <acronym>SSL</> inside your application (in addition
!    to inside <application>libpq</application>), you can call
!    <function>PQinitSSL(int)</> with <literal>0</> to tell
!    <application>libpq</application> that the <acronym>SSL</> library
!    has already been initialized by your application.
     <!-- If this URL changes replace it with a URL to www.archive.org. -->
     See <ulink
     url="http://h71000.www7.hp.com/doc/83final/BA554_90007/ch04.html"></ulink>
--- 6169,6181 ----
    </para>

    <para>
!    If your application initializes <literal>libssl</> or
!    <literal>libcrypto</> libraries and <application>libpq</application>
!    is built with <acronym>SSL</> support, you should call
!    <function>PQinitSSL(0)</> to tell <application>libpq</application>
!    that the <literal>libssl</> and <literal>libcrypto</> libraries
!    have been initialized by your application so
!    <application>libpq</application> will not initialize those libraries.
     <!-- If this URL changes replace it with a URL to www.archive.org. -->
     See <ulink
     url="http://h71000.www7.hp.com/doc/83final/BA554_90007/ch04.html"></ulink>
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.119
diff -c -c -r1.119 fe-secure.c
*** src/interfaces/libpq/fe-secure.c    28 Jan 2009 15:06:47 -0000    1.119
--- src/interfaces/libpq/fe-secure.c    28 Mar 2009 01:17:22 -0000
***************
*** 870,875 ****
--- 870,876 ----

          if (ssl_open_connections++ == 0)
          {
+             /* This is actually libcrypto, not libssl. */
              /* These are only required for threaded SSL applications */
              CRYPTO_set_id_callback(pq_threadidcallback);
              CRYPTO_set_locking_callback(pq_lockingcallback);
***************
*** 934,939 ****
--- 935,941 ----

          if (ssl_open_connections == 0)
          {
+             /* This is actually libcrypto, not libssl. */
              /* No connections left, unregister all callbacks */
              CRYPTO_set_locking_callback(NULL);
              CRYPTO_set_id_callback(NULL);