Обсуждение: SSL BIO wrappers

Поиск
Список
Период
Сортировка

SSL BIO wrappers

От
Magnus Hagander
Дата:
Attached patch replaces the SSL BIO wrapper code we have now, with one
that directly calls the send() and recv() functions instead. THis means
that they get passed through the rewrite macros to our internal
functions on Win32, and I think this will fix some of the strange errors
that seem to be platform specific there (there are some really hard to
reproduce bug reports around that).

They're of course tightly modeled around the code from OpenSSL - found
here: http://cvs.openssl.org/fileview?f=openssl/crypto/bio/bss_sock.c&v=1.15

(functions sock_read and sock_write)

So:

1) Thoughts in general?

2) Per my_sock_write - should we do the prepare read there as well, even
though it's a write? :-)

//Magnus
*** a/src/backend/libpq/be-secure.c
--- b/src/backend/libpq/be-secure.c
***************
*** 394,438 **** wloop:
  #ifdef USE_SSL

  /*
!  * Private substitute BIO: this wraps the SSL library's standard socket BIO
!  * so that we can enable and disable interrupts just while calling recv().
!  * We cannot have interrupts occurring while the bulk of openssl runs,
!  * because it uses malloc() and possibly other non-reentrant libc facilities.
   *
-  * As of openssl 0.9.7, we can use the reasonably clean method of interposing
-  * a wrapper around the standard socket BIO's sock_read() method.  This relies
-  * on the fact that sock_read() doesn't call anything non-reentrant, in fact
-  * not much of anything at all except recv().  If this ever changes we'd
-  * probably need to duplicate the code of sock_read() in order to push the
-  * interrupt enable/disable down yet another level.
   */

  static bool my_bio_initialized = false;
  static BIO_METHOD my_bio_methods;
- static int    (*std_sock_read) (BIO *h, char *buf, int size);

  static int
  my_sock_read(BIO *h, char *buf, int size)
  {
!     int            res;

      prepare_for_client_read();

!     res = std_sock_read(h, buf, size);

      client_read_ended();

      return res;
  }

  static BIO_METHOD *
  my_BIO_s_socket(void)
  {
      if (!my_bio_initialized)
      {
          memcpy(&my_bio_methods, BIO_s_socket(), sizeof(BIO_METHOD));
-         std_sock_read = my_bio_methods.bread;
          my_bio_methods.bread = my_sock_read;
          my_bio_initialized = true;
      }
      return &my_bio_methods;
--- 394,469 ----
  #ifdef USE_SSL

  /*
!  * Private substitute BIO: this does the sending and receiving using send() and
!  * recv() instead. This is so that we can enable and disable interrupts
!  * just while calling recv(). We cannot have interrupts occurring while
!  * the bulk of openssl runs, because it uses malloc() and possibly other
!  * non-reentrant libc facilities. We also need to call send() and recv()
!  * directly so it gets passed through the socket/signals layer on Win32.
!  *
!  * They are closely modelled on the original socket implementations in OpenSSL.
   *
   */

  static bool my_bio_initialized = false;
  static BIO_METHOD my_bio_methods;

  static int
  my_sock_read(BIO *h, char *buf, int size)
  {
!     int            res = 0;

      prepare_for_client_read();

!     if (buf != NULL)
!     {
!         res = recv(h->num, buf, size, 0);
!         BIO_clear_retry_flags(h);
!         if (res <= 0)
!         {
!             /* If we were interrupted, tell caller to retry */
!             if (errno == EINTR)
!             {
!                 BIO_set_retry_read(h);
!             }
!         }
!     }

      client_read_ended();

      return res;
  }

+ static int
+ my_sock_write(BIO *h, const char *buf, int size)
+ {
+     int            res = 0;
+
+     /*
+      * XXX: should we do a prepare_for_client_read here as well,
+      * even though it's not a read operation?
+      */
+
+     res = send(h->num, buf, size, 0);
+     if (res <= 0)
+     {
+         if (errno == EINTR)
+         {
+             BIO_set_retry_write(h);
+         }
+     }
+
+     return res;
+ }
+
  static BIO_METHOD *
  my_BIO_s_socket(void)
  {
      if (!my_bio_initialized)
      {
          memcpy(&my_bio_methods, BIO_s_socket(), sizeof(BIO_METHOD));
          my_bio_methods.bread = my_sock_read;
+         my_bio_methods.bwrite = my_sock_write;
          my_bio_initialized = true;
      }
      return &my_bio_methods;

Re: SSL BIO wrappers

От
Tom Lane
Дата:
Magnus Hagander <magnus@hagander.net> writes:
> Attached patch replaces the SSL BIO wrapper code we have now, with one
> that directly calls the send() and recv() functions instead. THis means
> that they get passed through the rewrite macros to our internal
> functions on Win32, and I think this will fix some of the strange errors
> that seem to be platform specific there (there are some really hard to
> reproduce bug reports around that).

Hmm.  Basically what this is doing is exactly what the comment says we
didn't want to do, namely copy-and-paste the implementations of
OpenSSL's socket BIO functions.  How stable is that code?  If the
functions haven't changed textually in a long time (at least across all
the OpenSSL versions we claim to support) then maybe it's okay.
        regards, tom lane


Re: SSL BIO wrappers

От
Magnus Hagander
Дата:
Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> Attached patch replaces the SSL BIO wrapper code we have now, with one
>> that directly calls the send() and recv() functions instead. THis means
>> that they get passed through the rewrite macros to our internal
>> functions on Win32, and I think this will fix some of the strange errors
>> that seem to be platform specific there (there are some really hard to
>> reproduce bug reports around that).
> 
> Hmm.  Basically what this is doing is exactly what the comment says we
> didn't want to do, namely copy-and-paste the implementations of
> OpenSSL's socket BIO functions.  How stable is that code?  If the
> functions haven't changed textually in a long time (at least across all
> the OpenSSL versions we claim to support) then maybe it's okay.

The logic in it is identical to the original import of code in OpenSSL.
It originally had #ifdefs around how the BIO interface worked. That was
tidied up in a commit back in 2001. I think it's fair to say it's been
pretty stable.

I don't read the comment as saying that, fwiw. It just says we may
eventually need to do what I did now, but for other reasons.

Do you have a comment around the "should we prepare for read even though
it's a write" part?

//Magnus


Re: SSL BIO wrappers

От
Tom Lane
Дата:
Magnus Hagander <magnus@hagander.net> writes:
> The logic in it is identical to the original import of code in OpenSSL.
> It originally had #ifdefs around how the BIO interface worked. That was
> tidied up in a commit back in 2001. I think it's fair to say it's been
> pretty stable.

Fair enough.

> Do you have a comment around the "should we prepare for read even though
> it's a write" part?

We shouldn't; in fact that would be quite wrong.
        regards, tom lane