Replace variable length array with malloc

PATCH Superseded Lev Stipakov 2 participants 6 messages Oct 20, 2015 – Oct 22, 2015

Share & tools

Lev Stipakov

author

2015-10-20 13:22 UTC #0 lore raw

PATCH 1/1 Replace variable length array with malloc

Commit https://github.com/OpenVPN/openvpn/commit/685e486e8b8f70c25f09590c24762ff734f94a51
introduced a variable length array. Although C99 supports that, MSVS 2013 still requires size of array to be compiler time constant. As a fix, use OPENSSL_malloc/free.

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpn/ssl_openssl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

src/openvpn/ssl_openssl.c +6 -1

index c08d4fe..1b4b1da 100644
@@ -141,7 +141,10 @@ key_state_export_keying_material(struct key_state_ssl *ssl,
     {
 #if (OPENSSL_VERSION_NUMBER >= 0x10001000)
       unsigned int size = session->opt->ekm_size;
-      unsigned char ekm[size];
+      unsigned char* ekm = OPENSSL_malloc(size);
+
+      if (ekm == NULL)
+	crypto_msg (M_FATAL, "Failed to allocate memory for export key material.");
 
       if (SSL_export_keying_material(ssl->ssl, ekm, sizeof(ekm),
           session->opt->ekm_label, session->opt->ekm_label_size, NULL, 0, 0))

@@ -162,6 +165,8 @@ key_state_export_keying_material(struct key_state_ssl *ssl,
          msg (M_WARN, "WARNING: Export keying material failed!");
          setenv_del (session->opt->es, "exported_keying_material");
        }
+
+      OPENSSL_free(ekm);
 #endif
     }
 }

Signature · 1 line

1.9.1

Gert Doering

2015-10-20 13:52 UTC #1 lore raw

Hi,

Quoted 3 lines · Lev Stipakov

Commit https://github.com/OpenVPN/openvpn/commit/685e486e8b8f70c25f09590c24762ff734f94a51
introduced a variable length array. Although C99 supports that, MSVS 2013 still requires size of array to be compiler time constant. As a fix, use OPENSSL_malloc/free.

Why OPENSSL_malloc() in particular?

(As I have no clue about the intricacies of openssl-interfacing code, this might be a stupid question, but it looks like "normal gc_malloc() should be perfectly fine")

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Lev Stipakov

author

2015-10-20 14:26 UTC #2 lore raw

Quoted 1 line

Why OPENSSL_malloc() in particular?

I looked for other malloc calls in that file and only example I've found was OPENSSL_malloc in show_available_curves().

On the other side Dr. Stephen Henson says (quote unedited):

http://permalink.gmane.org/gmane.comp.encryption.openssl.user/11291

Quoted 1 line

You don't have to use OPENSSL_malloc() in an application but you do you can make use of OpenSSLs memory leak checking routines if you do.

-Lev

Quoted 26 lines · Gert Doering

Hi,
Quoted 3 lines · Lev Stipakov
Commit https://github.com/OpenVPN/openvpn/commit/685e486e8b8f70c25f09590c24762ff734f94a51
introduced a variable length array. Although C99 supports that, MSVS 2013 still requires size of array to be compiler time constant. As a fix, use OPENSSL_malloc/free.
Why OPENSSL_malloc() in particular?
(As I have no clue about the intricacies of openssl-interfacing code, this might be a stupid question, but it looks like "normal gc_malloc() should be perfectly fine")
gert
------------------------------------------------------------------------------
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Lev Stipakov

author

2015-10-20 14:28 UTC #5 lore raw

In reply to Gert Doering

Quoted 1 line

Why OPENSSL_malloc() in particular?

I looked for other malloc calls in that file and only example I've found was OPENSSL_malloc in show_available_curves().

On the other side Dr. Stephen Henson says (quote unedited):

http://permalink.gmane.org/gmane.comp.encryption.openssl.user/11291

Quoted 1 line

You don't have to use OPENSSL_malloc() in an application but you do

you can make use of OpenSSLs memory leak checking routines if you do.

-Lev

Quoted 26 lines · Gert Doering

Hi,
Quoted 3 lines · Lev Stipakov
Commit https://github.com/OpenVPN/openvpn/commit/685e486e8b8f70c25f09590c24762ff734f94a51
introduced a variable length array. Although C99 supports that, MSVS 2013 still requires size of array to be compiler time constant. As a fix, use OPENSSL_malloc/free.
Why OPENSSL_malloc() in particular?
(As I have no clue about the intricacies of openssl-interfacing code, this might be a stupid question, but it looks like "normal gc_malloc() should be perfectly fine")
gert
------------------------------------------------------------------------------
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Lev Stipakov

author

2015-10-21 07:13 UTC #3 lore raw

PATCH v2 1/1 Replace variable length array with malloc

In reply to Lev Stipakov

v2:
Replace OPENSSL_malloc with gc_malloc

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpn/ssl_openssl.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

src/openvpn/ssl_openssl.c +3 -4

index c08d4fe..3c8d41f 100644
@@ -141,12 +141,12 @@ key_state_export_keying_material(struct key_state_ssl *ssl,
     {
 #if (OPENSSL_VERSION_NUMBER >= 0x10001000)
       unsigned int size = session->opt->ekm_size;
-      unsigned char ekm[size];
+      struct gc_arena gc = gc_new();
+      unsigned char* ekm = (unsigned char*) gc_malloc(size, true, &gc);
 
       if (SSL_export_keying_material(ssl->ssl, ekm, sizeof(ekm),
           session->opt->ekm_label, session->opt->ekm_label_size, NULL, 0, 0))
        {
-         struct gc_arena gc = gc_new();
          unsigned int len = (size * 2) + 2;
 
          const char *key = format_hex_ex (ekm, size, len, 0, NULL, &gc);

@@ -154,14 +154,13 @@ key_state_export_keying_material(struct key_state_ssl *ssl,
 
          dmsg(D_TLS_DEBUG_MED, "%s: exported keying material: %s",
               __func__, key);
-
-         gc_free(&gc);
        }
       else
        {
          msg (M_WARN, "WARNING: Export keying material failed!");
          setenv_del (session->opt->es, "exported_keying_material");
        }
+      gc_free(&gc);
 #endif
     }
 }

Signature · 1 line

1.9.1

Gert Doering

2015-10-22 18:05 UTC #4 lore raw

ACK. Looks sane and is totally in line with coding conventions :-) - and very useful to actually have a gc around!

Your patch has been applied to the master branch.

commit 41e4b67a229e774ebc57a882c386e10d80e10e7e

Author: Lev Stipakov
Date:   Wed Oct 21 10:13:26 2015 +0300

     Replace variable length array with malloc

     Signed-off-by: Lev Stipakov <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10344
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering