[1/2] rtmpdh: Don't use the OpenSSL DH struct

Message ID 20161021110244.687-1-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö Oct. 21, 2016, 11:02 a.m.
Instead use our own struct, which we already use when using
gcrypt and gnutls.

In OpenSSL 1.1, the DH struct has been made opaque.
---
This produces the following warnings when building with gnutls:
warning: unused label 'fail' [-Wunused-label]

Any suggestions on avoiding that?
---
 libavformat/rtmpdh.c | 78 +++++++++++++++++++++++-----------------------------
 libavformat/rtmpdh.h | 14 ++++------
 2 files changed, 40 insertions(+), 52 deletions(-)

Comments

Diego Biurrun Oct. 21, 2016, 11:43 a.m. | #1
On Fri, Oct 21, 2016 at 02:02:43PM +0300, Martin Storsjö wrote:
> Instead use our own struct, which we already use when using
> gcrypt and gnutls.
> 
> In OpenSSL 1.1, the DH struct has been made opaque.
> ---
> This produces the following warnings when building with gnutls:
> warning: unused label 'fail' [-Wunused-label]
> 
> Any suggestions on avoiding that?

You could make bn_modexp a function instead of a macro and check its
return value. In that case you don't need to add a fail label to the
calling function.

Diego
Luca Barbato Oct. 21, 2016, 2:04 p.m. | #2
On 21/10/2016 13:02, Martin Storsjö wrote:
> Instead use our own struct, which we already use when using
> gcrypt and gnutls.
> 
> In OpenSSL 1.1, the DH struct has been made opaque.
> ---
> This produces the following warnings when building with gnutls:
> warning: unused label 'fail' [-Wunused-label]
> 
> Any suggestions on avoiding that?


You could make the macro a function, for gmp and gcrypt you always
return 0, for the openssl variant you return the error and in the common
code you check for ret < 0 as usual.

Patch

diff --git a/libavformat/rtmpdh.c b/libavformat/rtmpdh.c
index e7a83e1..d56b2cb 100644
--- a/libavformat/rtmpdh.c
+++ b/libavformat/rtmpdh.c
@@ -54,7 +54,6 @@ 
     "F71C35FDAD44CFD2D74F9208BE258FF324943328F67329C0" \
     "FFFFFFFFFFFFFFFF"
 
-#if CONFIG_GMP || CONFIG_GCRYPT
 #if CONFIG_GMP
 #define bn_new(bn)                      \
     do {                                \
@@ -118,15 +117,43 @@ 
 #define bn_hex2bn(bn, buf, ret)     ret = (gcry_mpi_scan(&bn, GCRYMPI_FMT_HEX, buf, 0, 0) == 0)
 #define bn_modexp(bn, y, q, p)      gcry_mpi_powm(bn, y, q, p)
 #define bn_random(bn, num_bits)     gcry_mpi_randomize(bn, num_bits, GCRY_WEAK_RANDOM)
+#elif CONFIG_OPENSSL
+#define bn_new(bn)                  bn = BN_new()
+#define bn_free(bn)                 BN_free(bn)
+#define bn_set_word(bn, w)          BN_set_word(bn, w)
+#define bn_cmp(a, b)                BN_cmp(a, b)
+#define bn_copy(to, from)           BN_copy(to, from)
+#define bn_sub_word(bn, w)          BN_sub_word(bn, w)
+#define bn_cmp_1(bn)                BN_cmp(bn, BN_value_one())
+#define bn_num_bytes(bn)            BN_num_bytes(bn)
+#define bn_bn2bin(bn, buf, len)     BN_bn2bin(bn, buf)
+#define bn_bin2bn(bn, buf, len)     bn = BN_bin2bn(buf, len, 0)
+#define bn_hex2bn(bn, buf, ret)     ret = BN_hex2bn(&bn, buf)
+#define bn_modexp(bn, y, q, p)               \
+    do {                                     \
+        BN_CTX *ctx = BN_CTX_new();          \
+        if (!ctx) {                          \
+            ret = AVERROR(ENOMEM);           \
+            goto fail;                       \
+        }                                    \
+        if (!BN_mod_exp(bn, y, q, p, ctx)) { \
+            BN_CTX_free(ctx);                \
+            ret = AVERROR(EINVAL);           \
+            goto fail;                       \
+        }                                    \
+        BN_CTX_free(ctx);                    \
+    } while (0)
+#define bn_random(bn, num_bits)     BN_rand(bn, num_bits, 0, 0)
 #endif
 
 #define MAX_BYTES 18000
 
-#define dh_new()                    av_malloc(sizeof(FF_DH))
+#define dh_new()                    av_mallocz(sizeof(FF_DH))
 
 static FFBigNum dh_generate_key(FF_DH *dh)
 {
     int num_bytes;
+    av_unused int ret;
 
     num_bytes = bn_num_bytes(dh->p) - 1;
     if (num_bytes <= 0 || num_bytes > MAX_BYTES)
@@ -146,12 +173,15 @@  static FFBigNum dh_generate_key(FF_DH *dh)
     bn_modexp(dh->pub_key, dh->g, dh->priv_key, dh->p);
 
     return dh->pub_key;
+fail:
+    return NULL;
 }
 
 static int dh_compute_key(FF_DH *dh, FFBigNum pub_key_bn,
                           uint32_t secret_key_len, uint8_t *secret_key)
 {
     FFBigNum k;
+    int ret;
 
     bn_new(k);
     if (!k)
@@ -163,6 +193,8 @@  static int dh_compute_key(FF_DH *dh, FFBigNum pub_key_bn,
 
     /* return the length of the shared secret key like DH_compute_key */
     return secret_key_len;
+fail:
+    return ret;
 }
 
 void ff_dh_free(FF_DH *dh)
@@ -175,48 +207,6 @@  void ff_dh_free(FF_DH *dh)
     bn_free(dh->priv_key);
     av_free(dh);
 }
-#elif CONFIG_OPENSSL
-#define bn_new(bn)                  bn = BN_new()
-#define bn_free(bn)                 BN_free(bn)
-#define bn_set_word(bn, w)          BN_set_word(bn, w)
-#define bn_cmp(a, b)                BN_cmp(a, b)
-#define bn_copy(to, from)           BN_copy(to, from)
-#define bn_sub_word(bn, w)          BN_sub_word(bn, w)
-#define bn_cmp_1(bn)                BN_cmp(bn, BN_value_one())
-#define bn_num_bytes(bn)            BN_num_bytes(bn)
-#define bn_bn2bin(bn, buf, len)     BN_bn2bin(bn, buf)
-#define bn_bin2bn(bn, buf, len)     bn = BN_bin2bn(buf, len, 0)
-#define bn_hex2bn(bn, buf, ret)     ret = BN_hex2bn(&bn, buf)
-#define bn_modexp(bn, y, q, p)               \
-    do {                                     \
-        BN_CTX *ctx = BN_CTX_new();          \
-        if (!ctx)                            \
-            return AVERROR(ENOMEM);          \
-        if (!BN_mod_exp(bn, y, q, p, ctx)) { \
-            BN_CTX_free(ctx);                \
-            return AVERROR(EINVAL);          \
-        }                                    \
-        BN_CTX_free(ctx);                    \
-    } while (0)
-
-#define dh_new()                                DH_new()
-#define dh_generate_key(dh)                     DH_generate_key(dh)
-
-static int dh_compute_key(FF_DH *dh, FFBigNum pub_key_bn,
-                          uint32_t secret_key_len, uint8_t *secret_key)
-{
-    if (secret_key_len < DH_size(dh))
-        return AVERROR(EINVAL);
-    return DH_compute_key(secret_key, pub_key_bn, dh);
-}
-
-void ff_dh_free(FF_DH *dh)
-{
-    if (!dh)
-        return;
-    DH_free(dh);
-}
-#endif
 
 static int dh_is_valid_public_key(FFBigNum y, FFBigNum p, FFBigNum q)
 {
diff --git a/libavformat/rtmpdh.h b/libavformat/rtmpdh.h
index eb742dd..5233de0 100644
--- a/libavformat/rtmpdh.h
+++ b/libavformat/rtmpdh.h
@@ -26,7 +26,6 @@ 
 
 #include "config.h"
 
-#if CONFIG_GMP || CONFIG_GCRYPT
 #if CONFIG_GMP
 #include <gmp.h>
 
@@ -35,6 +34,12 @@  typedef mpz_ptr FFBigNum;
 #include <gcrypt.h>
 
 typedef gcry_mpi_t FFBigNum;
+
+#elif CONFIG_OPENSSL
+#include <openssl/bn.h>
+#include <openssl/dh.h>
+
+typedef BIGNUM *FFBigNum;
 #endif
 
 typedef struct FF_DH {
@@ -45,13 +50,6 @@  typedef struct FF_DH {
     long length;
 } FF_DH;
 
-#elif CONFIG_OPENSSL
-#include <openssl/bn.h>
-#include <openssl/dh.h>
-
-typedef BIGNUM *FFBigNum;
-typedef DH FF_DH;
-#endif
 
 /**
  * Initialize a Diffie-Hellmann context.