diff mbox series

[Openvpn-devel] ssl_openssl: fix compiler warning by removing getbio() wrapper

Message ID 1513246897-28171-1-git-send-email-steffan@karger.me
State Accepted
Headers
Series [Openvpn-devel] ssl_openssl: fix compiler warning by removing getbio() wrapper |

Commit Message

Steffan Karger Dec. 13, 2017, 11:21 p.m. UTC 
  An API change in openssl 1.1 made the BIO_METHOD * returned by BIO_f_ssl()
and BIO_s_mem() const, as well as the BIO_METHOD * argment of BIO_new()
const.  This meant that our getbio() function would either have an API
inconsistent with 1.0 or 1.1.

The wrapper was basically an ASSERT, so fix this by replacing the wrapper
with an ASSERT.

Signed-off-by: Steffan Karger <steffan@karger.me>
---
 src/openvpn/ssl_openssl.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

Comments

Arne Schwabe Dec. 14, 2017, 12:05 a.m. UTC | #1 
Am 14.12.17 um 11:21 schrieb Steffan Karger:
> An API change in openssl 1.1 made the BIO_METHOD * returned by BIO_f_ssl()
> and BIO_s_mem() const, as well as the BIO_METHOD * argment of BIO_new()
> const.  This meant that our getbio() function would either have an API
> inconsistent with 1.0 or 1.1.
> 
> The wrapper was basically an ASSERT, so fix this by replacing the wrapper
> with an ASSERT.
> 

Looks good. The wrapper does also not add much clarity or otherwise
functionality.

Acked-by: Arne Schwabe

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Dec. 14, 2017, 4:44 a.m. UTC | #2 
Hi,

On Thu, Dec 14, 2017 at 11:21:37AM +0100, Steffan Karger wrote:
> -    if (!ret)
> -    {
> -        crypto_msg(M_FATAL, "Error creating %s BIO", desc);
> -    }

I'm not sure how likely these are going to fail, but the crypto_msg()
had the benefit of actually telling us what the underlying library
returned as error code...

If this won't ever happen except under very exceptional conditions, it
won't matter much, though.

gert
Steffan Karger Dec. 14, 2017, 5:04 a.m. UTC | #3 
Hi,

On 14-12-17 16:44, Gert Doering wrote:
> On Thu, Dec 14, 2017 at 11:21:37AM +0100, Steffan Karger wrote:
>> -    if (!ret)
>> -    {
>> -        crypto_msg(M_FATAL, "Error creating %s BIO", desc);
>> -    }
> 
> I'm not sure how likely these are going to fail, but the crypto_msg()
> had the benefit of actually telling us what the underlying library
> returned as error code...
> 
> If this won't ever happen except under very exceptional conditions, it
> won't matter much, though.

Fair point, but this is very unlikely to fail indeed.  We use standard
openssl-provided BIO_METHOD's, and do not do threading.  The one error I
could imagine is a malloc failure.

I still think it's fine to just go with the ASSERT().

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Jan. 11, 2018, 12:59 a.m. UTC | #4 
Your patch has been applied to the master and release/2.4 branch.

commit 006d6a57b8835c15222359bfb42c95005723394c (master)
commit 041b94c0ab1981c35f4294afc3f9e4f73ba9056c (release/2.4)
Author: Steffan Karger
Date:   Thu Dec 14 11:21:37 2017 +0100

     ssl_openssl: fix compiler warning by removing getbio() wrapper

     Signed-off-by: Steffan Karger <steffan@karger.me>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <1513246897-28171-1-git-send-email-steffan@karger.me>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16083.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff mbox series

Patch

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 34c31b9..711bba1 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -1416,23 +1416,6 @@  bio_debug_oc(const char *mode, BIO *bio)
 #endif /* ifdef BIO_DEBUG */
 
 /*
- * OpenVPN's interface to SSL/TLS authentication,
- * encryption, and decryption is exclusively
- * through "memory BIOs".
- */
-static BIO *
-getbio(BIO_METHOD *type, const char *desc)
-{
-    BIO *ret;
-    ret = BIO_new(type);
-    if (!ret)
-    {
-        crypto_msg(M_FATAL, "Error creating %s BIO", desc);
-    }
-    return ret;
-}
-
-/*
  * Write to an OpenSSL BIO in non-blocking mode.
  */
 static int
@@ -1573,9 +1556,9 @@  key_state_ssl_init(struct key_state_ssl *ks_ssl, const struct tls_root_ctx *ssl_
      * from verify callback*/
     SSL_set_ex_data(ks_ssl->ssl, mydata_index, session);
 
-    ks_ssl->ssl_bio = getbio(BIO_f_ssl(), "ssl_bio");
-    ks_ssl->ct_in = getbio(BIO_s_mem(), "ct_in");
-    ks_ssl->ct_out = getbio(BIO_s_mem(), "ct_out");
+    ASSERT((ks_ssl->ssl_bio = BIO_new(BIO_f_ssl())));
+    ASSERT((ks_ssl->ct_in = BIO_new(BIO_s_mem())));
+    ASSERT((ks_ssl->ct_out = BIO_new(BIO_s_mem())));
 
 #ifdef BIO_DEBUG
     bio_debug_oc("open ssl_bio", ks_ssl->ssl_bio);