diff mbox series

[Openvpn-devel] Fix crash in xkey-provider in msvc builds

Message ID 20220707035151.25469-1-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] Fix crash in xkey-provider in msvc builds | expand

Commit Message

Selva Nair July 6, 2022, 5:51 p.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

The function signature for xkey_load_generic_key had
function pointers defined as function types that seems
to work in gcc but not in msvc.

Fix it by changing the function signatures to what was
intended.
Also revert part of commit 627d1a3d28638... as that work-
around should be no longer required.

Reported by: Lev Stipakov https://github.com/lstipakov

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/xkey_common.h | 2 +-
 src/openvpn/xkey_helper.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Selva Nair July 14, 2022, 4:52 a.m. UTC | #1 
Hi,

Any thoughts on this? Apart from the broken msvc builds that led to this,
looks like the right thing to do, isn't it?

Selva

On Wed, Jul 6, 2022 at 11:52 PM <selva.nair@gmail.com> wrote:

> From: Selva Nair <selva.nair@gmail.com>
>
> The function signature for xkey_load_generic_key had
> function pointers defined as function types that seems
> to work in gcc but not in msvc.
>
> Fix it by changing the function signatures to what was
> intended.
> Also revert part of commit 627d1a3d28638... as that work-
> around should be no longer required.
>
> Reported by: Lev Stipakov https://github.com/lstipakov
>
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
>  src/openvpn/xkey_common.h | 2 +-
>  src/openvpn/xkey_helper.c | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/xkey_common.h b/src/openvpn/xkey_common.h
> index e0e5ed5b..6d6a1e2c 100644
> --- a/src/openvpn/xkey_common.h
> +++ b/src/openvpn/xkey_common.h
> @@ -152,7 +152,7 @@ xkey_digest(const unsigned char *src, size_t srclen,
> unsigned char *buf,
>   */
>  EVP_PKEY *
>  xkey_load_generic_key(OSSL_LIB_CTX *libctx, void *handle, EVP_PKEY
> *pubkey,
> -                      XKEY_EXTERNAL_SIGN_fn sign_op, XKEY_PRIVKEY_FREE_fn
> free_op);
> +                      XKEY_EXTERNAL_SIGN_fn *sign_op,
> XKEY_PRIVKEY_FREE_fn *free_op);
>
>  extern OSSL_LIB_CTX *tls_libctx; /* Global */
>
> diff --git a/src/openvpn/xkey_helper.c b/src/openvpn/xkey_helper.c
> index 14f074ae..73235fd6 100644
> --- a/src/openvpn/xkey_helper.c
> +++ b/src/openvpn/xkey_helper.c
> @@ -115,7 +115,7 @@ xkey_load_management_key(OSSL_LIB_CTX *libctx,
> EVP_PKEY *pubkey)
>   */
>  EVP_PKEY *
>  xkey_load_generic_key(OSSL_LIB_CTX *libctx, void *handle, EVP_PKEY
> *pubkey,
> -                      XKEY_EXTERNAL_SIGN_fn sign_op, XKEY_PRIVKEY_FREE_fn
> free_op)
> +                      XKEY_EXTERNAL_SIGN_fn *sign_op,
> XKEY_PRIVKEY_FREE_fn *free_op)
>  {
>      EVP_PKEY *pkey = NULL;
>      const char *origin = "external";
> @@ -125,8 +125,8 @@ xkey_load_generic_key(OSSL_LIB_CTX *libctx, void
> *handle, EVP_PKEY *pubkey,
>          {"xkey-origin", OSSL_PARAM_UTF8_STRING, (char *) origin, 0, 0},
>          {"pubkey", OSSL_PARAM_OCTET_STRING, &pubkey, sizeof(pubkey), 0},
>          {"handle", OSSL_PARAM_OCTET_PTR, &handle, sizeof(handle), 0},
> -        {"sign_op", OSSL_PARAM_OCTET_PTR, (void **) &sign_op, sizeof(void
> *), 0},
> -        {"free_op", OSSL_PARAM_OCTET_PTR, (void **) &free_op, sizeof(void
> *), 0},
> +        {"sign_op", OSSL_PARAM_OCTET_PTR, (void **) &sign_op,
> sizeof(sign_op), 0},
> +        {"free_op", OSSL_PARAM_OCTET_PTR, (void **) &free_op,
> sizeof(free_op), 0},
>          {NULL, 0, NULL, 0, 0}
>      };
>      msg(M_INFO, "%s: handle = %p sign_op = %p free_op = %p ",
> __FUNCTION__,
> --
> 2.30.2
>
>
<div dir="ltr"><div>Hi,</div><div><br></div>Any thoughts on this? Apart from the broken msvc builds that led to this, looks like the right thing to do, isn&#39;t it?<div><br></div><div>Selva</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 6, 2022 at 11:52 PM &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br>
<br>
The function signature for xkey_load_generic_key had<br>
function pointers defined as function types that seems<br>
to work in gcc but not in msvc.<br>
<br>
Fix it by changing the function signatures to what was<br>
intended.<br>
Also revert part of commit 627d1a3d28638... as that work-<br>
around should be no longer required.<br>
<br>
Reported by: Lev Stipakov <a href="https://github.com/lstipakov" rel="noreferrer" target="_blank">https://github.com/lstipakov</a><br>
<br>
Signed-off-by: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br>
---<br>
 src/openvpn/xkey_common.h | 2 +-<br>
 src/openvpn/xkey_helper.c | 6 +++---<br>
 2 files changed, 4 insertions(+), 4 deletions(-)<br>
<br>
diff --git a/src/openvpn/xkey_common.h b/src/openvpn/xkey_common.h<br>
index e0e5ed5b..6d6a1e2c 100644<br>
--- a/src/openvpn/xkey_common.h<br>
+++ b/src/openvpn/xkey_common.h<br>
@@ -152,7 +152,7 @@ xkey_digest(const unsigned char *src, size_t srclen, unsigned char *buf,<br>
  */<br>
 EVP_PKEY *<br>
 xkey_load_generic_key(OSSL_LIB_CTX *libctx, void *handle, EVP_PKEY *pubkey,<br>
-                      XKEY_EXTERNAL_SIGN_fn sign_op, XKEY_PRIVKEY_FREE_fn free_op);<br>
+                      XKEY_EXTERNAL_SIGN_fn *sign_op, XKEY_PRIVKEY_FREE_fn *free_op);<br>
<br>
 extern OSSL_LIB_CTX *tls_libctx; /* Global */<br>
<br>
diff --git a/src/openvpn/xkey_helper.c b/src/openvpn/xkey_helper.c<br>
index 14f074ae..73235fd6 100644<br>
--- a/src/openvpn/xkey_helper.c<br>
+++ b/src/openvpn/xkey_helper.c<br>
@@ -115,7 +115,7 @@ xkey_load_management_key(OSSL_LIB_CTX *libctx, EVP_PKEY *pubkey)<br>
  */<br>
 EVP_PKEY *<br>
 xkey_load_generic_key(OSSL_LIB_CTX *libctx, void *handle, EVP_PKEY *pubkey,<br>
-                      XKEY_EXTERNAL_SIGN_fn sign_op, XKEY_PRIVKEY_FREE_fn free_op)<br>
+                      XKEY_EXTERNAL_SIGN_fn *sign_op, XKEY_PRIVKEY_FREE_fn *free_op)<br>
 {<br>
     EVP_PKEY *pkey = NULL;<br>
     const char *origin = &quot;external&quot;;<br>
@@ -125,8 +125,8 @@ xkey_load_generic_key(OSSL_LIB_CTX *libctx, void *handle, EVP_PKEY *pubkey,<br>
         {&quot;xkey-origin&quot;, OSSL_PARAM_UTF8_STRING, (char *) origin, 0, 0},<br>
         {&quot;pubkey&quot;, OSSL_PARAM_OCTET_STRING, &amp;pubkey, sizeof(pubkey), 0},<br>
         {&quot;handle&quot;, OSSL_PARAM_OCTET_PTR, &amp;handle, sizeof(handle), 0},<br>
-        {&quot;sign_op&quot;, OSSL_PARAM_OCTET_PTR, (void **) &amp;sign_op, sizeof(void *), 0},<br>
-        {&quot;free_op&quot;, OSSL_PARAM_OCTET_PTR, (void **) &amp;free_op, sizeof(void *), 0},<br>
+        {&quot;sign_op&quot;, OSSL_PARAM_OCTET_PTR, (void **) &amp;sign_op, sizeof(sign_op), 0},<br>
+        {&quot;free_op&quot;, OSSL_PARAM_OCTET_PTR, (void **) &amp;free_op, sizeof(free_op), 0},<br>
         {NULL, 0, NULL, 0, 0}<br>
     };<br>
     msg(M_INFO, &quot;%s: handle = %p sign_op = %p free_op = %p &quot;, __FUNCTION__,<br>
-- <br>
2.30.2<br>
<br>
</blockquote></div>
Arne Schwabe July 14, 2022, 5:28 a.m. UTC | #2 
Am 07.07.22 um 05:51 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
> 
> The function signature for xkey_load_generic_key had
> function pointers defined as function types that seems
> to work in gcc but not in msvc.
> 
> Fix it by changing the function signatures to what was
> intended.
> Also revert part of commit 627d1a3d28638... as that work-
> around should be no longer required.
> 
> Reported by: Lev Stipakov https://github.com/lstipakov
>

I am surprised that this commit is not a noop but it is "more" correct 
this. So without any idea if this actually fixes Windows or not, I 
approve this commit on the basis of a cosmetic fix that is actually right.

Arne
Gert Doering July 14, 2022, 8:30 a.m. UTC | #3 
Taking the agreement from Arne as formal ACK (checked on IRC).

I'm not sure I grok all the details of this, but I can attest that 
unit_tests/test_provider still works (on a 3.0.x build), and that calls
the "xkey_load_generic_key()" function being modified here.  So, this
is good.  Lev promised to verify MSVC builds early next week.

Your patch has been applied to the master branch.

commit 2b2e4d0ad5c3a309af8425be8e6e605975fb2a43
Author: Selva Nair
Date:   Wed Jul 6 23:51:51 2022 -0400

     Fix crash in xkey-provider in msvc builds

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20220707035151.25469-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24664.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/xkey_common.h b/src/openvpn/xkey_common.h
index e0e5ed5b..6d6a1e2c 100644
--- a/src/openvpn/xkey_common.h
+++ b/src/openvpn/xkey_common.h
@@ -152,7 +152,7 @@  xkey_digest(const unsigned char *src, size_t srclen, unsigned char *buf,
  */
 EVP_PKEY *
 xkey_load_generic_key(OSSL_LIB_CTX *libctx, void *handle, EVP_PKEY *pubkey,
-                      XKEY_EXTERNAL_SIGN_fn sign_op, XKEY_PRIVKEY_FREE_fn free_op);
+                      XKEY_EXTERNAL_SIGN_fn *sign_op, XKEY_PRIVKEY_FREE_fn *free_op);
 
 extern OSSL_LIB_CTX *tls_libctx; /* Global */
 
diff --git a/src/openvpn/xkey_helper.c b/src/openvpn/xkey_helper.c
index 14f074ae..73235fd6 100644
--- a/src/openvpn/xkey_helper.c
+++ b/src/openvpn/xkey_helper.c
@@ -115,7 +115,7 @@  xkey_load_management_key(OSSL_LIB_CTX *libctx, EVP_PKEY *pubkey)
  */
 EVP_PKEY *
 xkey_load_generic_key(OSSL_LIB_CTX *libctx, void *handle, EVP_PKEY *pubkey,
-                      XKEY_EXTERNAL_SIGN_fn sign_op, XKEY_PRIVKEY_FREE_fn free_op)
+                      XKEY_EXTERNAL_SIGN_fn *sign_op, XKEY_PRIVKEY_FREE_fn *free_op)
 {
     EVP_PKEY *pkey = NULL;
     const char *origin = "external";
@@ -125,8 +125,8 @@  xkey_load_generic_key(OSSL_LIB_CTX *libctx, void *handle, EVP_PKEY *pubkey,
         {"xkey-origin", OSSL_PARAM_UTF8_STRING, (char *) origin, 0, 0},
         {"pubkey", OSSL_PARAM_OCTET_STRING, &pubkey, sizeof(pubkey), 0},
         {"handle", OSSL_PARAM_OCTET_PTR, &handle, sizeof(handle), 0},
-        {"sign_op", OSSL_PARAM_OCTET_PTR, (void **) &sign_op, sizeof(void *), 0},
-        {"free_op", OSSL_PARAM_OCTET_PTR, (void **) &free_op, sizeof(void *), 0},
+        {"sign_op", OSSL_PARAM_OCTET_PTR, (void **) &sign_op, sizeof(sign_op), 0},
+        {"free_op", OSSL_PARAM_OCTET_PTR, (void **) &free_op, sizeof(free_op), 0},
         {NULL, 0, NULL, 0, 0}
     };
     msg(M_INFO, "%s: handle = %p sign_op = %p free_op = %p ", __FUNCTION__,