Fix the error handling in CRYPTO_dup_ex_data.

Fix a strict aliasing issue in ui_dup_method_data.
Add test coverage for CRYPTO_dup_ex_data, use OPENSSL_assert.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2988)

4 files changed, 56 insertions, 27 deletions

diff --git a/crypto/ex_data.c b/crypto/ex_data.c
index 84b65551c6..4a3201a953 100644
--- a/crypto/ex_data.c
+++ b/crypto/ex_data.c
@@ -124,7 +124,7 @@ static int dummy_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
                      void *from_d, int idx,
                      long argl, void *argp)
 {
-    return 0;
+    return 1;
 }
 
 int CRYPTO_free_ex_index(int class_index, int idx)
@@ -254,10 +254,11 @@ int CRYPTO_dup_ex_data(int class_index, CRYPTO_EX_DATA *to,
                        const CRYPTO_EX_DATA *from)
 {
     int mx, j, i;
-    char *ptr;
+    void *ptr;
     EX_CALLBACK *stack[10];
     EX_CALLBACK **storage = NULL;
     EX_CALLBACKS *ip;
+    int toret = 0;
 
     if (from->sk == NULL)
         /* Nothing to copy over */
@@ -280,21 +281,28 @@ int CRYPTO_dup_ex_data(int class_index, CRYPTO_EX_DATA *to,
     }
     CRYPTO_THREAD_unlock(ex_data_lock);
 
-    if (mx > 0 && storage == NULL) {
+    if (mx == 0)
+        return 1;
+    if (storage == NULL) {
         CRYPTOerr(CRYPTO_F_CRYPTO_DUP_EX_DATA, ERR_R_MALLOC_FAILURE);
         return 0;
     }
+    if (!CRYPTO_set_ex_data(to, mx - 1, NULL))
+        goto err;
 
     for (i = 0; i < mx; i++) {
         ptr = CRYPTO_get_ex_data(from, i);
         if (storage[i] && storage[i]->dup_func)
-            storage[i]->dup_func(to, from, &ptr, i,
-                                 storage[i]->argl, storage[i]->argp);
+            if (!storage[i]->dup_func(to, from, &ptr, i,
+                                      storage[i]->argl, storage[i]->argp))
+                goto err;
         CRYPTO_set_ex_data(to, i, ptr);
     }
+    toret = 1;
+ err:
     if (storage != stack)
         OPENSSL_free(storage);
-    return 1;
+    return toret;
 }
 
 
diff --git a/doc/man3/CRYPTO_get_ex_new_index.pod b/doc/man3/CRYPTO_get_ex_new_index.pod
index ede5fc14ce..0853ce588c 100644
--- a/doc/man3/CRYPTO_get_ex_new_index.pod
+++ b/doc/man3/CRYPTO_get_ex_new_index.pod
@@ -130,12 +130,15 @@ the same callback handles different types of exdata.
 dup_func() is called when a structure is being copied.  This is only done
 for B<SSL> and B<SSL_SESSION> objects.  The B<to> and B<from> parameters
 are pointers to the destination and source B<CRYPTO_EX_DATA> structures,
-respectively.  The B<srcp> parameter is a pointer to the source exdata.
-When the dup_func() returns, the value in B<srcp> is copied to the
-destination ex_data.  If the pointer contained in B<srcp> is not modified
+respectively.  The B<from_d> parameter needs to be cast to a B<void **pptr>
+as the API has currently the wrong signature; that will be changed in a
+future version.  The B<*pptr> is a pointer to the source exdata.
+When the dup_func() returns, the value in B<*pptr> is copied to the
+destination ex_data.  If the pointer contained in B<*pptr> is not modified
 by the dup_func(), then both B<to> and B<from> will point to the same data.
 The B<idx>, B<argl> and B<argp> parameters are as described for the other
-two callbacks.
+two callbacks.  If the dup_func() returns B<0> the whole CRYPTO_dup_ex_data()
+will fail.
 
 =head1 RETURN VALUES
 
diff --git a/include/openssl/crypto.h b/include/openssl/crypto.h
index 3b75dbe577..a8b8dc1696 100644
--- a/include/openssl/crypto.h
+++ b/include/openssl/crypto.h
@@ -175,7 +175,7 @@ typedef void CRYPTO_EX_new (void *parent, void *ptr, CRYPTO_EX_DATA *ad,
 typedef void CRYPTO_EX_free (void *parent, void *ptr, CRYPTO_EX_DATA *ad,
                              int idx, long argl, void *argp);
 typedef int CRYPTO_EX_dup (CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
-                           void *srcp, int idx, long argl, void *argp);
+                           void *from_d, int idx, long argl, void *argp);
 __owur int CRYPTO_get_ex_new_index(int class_index, long argl, void *argp,
                             CRYPTO_EX_new *new_func, CRYPTO_EX_dup *dup_func,
                             CRYPTO_EX_free *free_func);
diff --git a/test/exdatatest.c b/test/exdatatest.c
index e82fdbfaea..e0eadd32dd 100644
--- a/test/exdatatest.c
+++ b/test/exdatatest.c
@@ -10,7 +10,6 @@
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
-#include <assert.h>
 #include <openssl/crypto.h>
 
 static long saved_argl;
@@ -20,26 +19,28 @@ static int saved_idx;
 static void exnew(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
           int idx, long argl, void *argp)
 {
-    assert(idx == saved_idx);
-    assert(argl == saved_argl);
-    assert(argp == saved_argp);
+    OPENSSL_assert(idx == saved_idx);
+    OPENSSL_assert(argl == saved_argl);
+    OPENSSL_assert(argp == saved_argp);
+    OPENSSL_assert(ptr == NULL);
 }
 
 static int exdup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
           void *from_d, int idx, long argl, void *argp)
 {
-    assert(idx == saved_idx);
-    assert(argl == saved_argl);
-    assert(argp == saved_argp);
-    return 0;
+    OPENSSL_assert(idx == saved_idx);
+    OPENSSL_assert(argl == saved_argl);
+    OPENSSL_assert(argp == saved_argp);
+    OPENSSL_assert(from_d != NULL);
+    return 1;
 }
 
 static void exfree(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
             int idx, long argl, void *argp)
 {
-    assert(idx == saved_idx);
-    assert(argl == saved_argl);
-    assert(argp == saved_argp);
+    OPENSSL_assert(idx == saved_idx);
+    OPENSSL_assert(argl == saved_argl);
+    OPENSSL_assert(argp == saved_argp);
 }
 
 typedef struct myobj_st {
@@ -55,14 +56,14 @@ static MYOBJ *MYOBJ_new()
 
     obj->id = ++count;
     obj->st = CRYPTO_new_ex_data(CRYPTO_EX_INDEX_APP, obj, &obj->ex_data);
-    assert(obj->st != 0);
+    OPENSSL_assert(obj->st != 0);
     return obj;
 }
 
 static void MYOBJ_sethello(MYOBJ *obj, char *cp)
 {
     obj->st = CRYPTO_set_ex_data(&obj->ex_data, saved_idx, cp);
-    assert(obj->st != 0);
+    OPENSSL_assert(obj->st != 0);
 }
 
 static char *MYOBJ_gethello(MYOBJ *obj)
@@ -76,9 +77,19 @@ static void MYOBJ_free(MYOBJ *obj)
     OPENSSL_free(obj);
 }
 
+static MYOBJ *MYOBJ_dup(MYOBJ *in)
+{
+    MYOBJ *obj = MYOBJ_new();
+
+    obj->st = CRYPTO_dup_ex_data(CRYPTO_EX_INDEX_APP, &obj->ex_data,
+                                 &in->ex_data);
+    OPENSSL_assert(obj->st != 0);
+    return obj;
+}
+
 int main()
 {
-    MYOBJ *t1, *t2;
+    MYOBJ *t1, *t2, *t3;
     const char *cp;
     char *p;
 
@@ -92,15 +103,22 @@ int main()
     t2 = MYOBJ_new();
     MYOBJ_sethello(t1, p);
     cp = MYOBJ_gethello(t1);
-    assert(cp == p);
+    OPENSSL_assert(cp == p);
     if (cp != p)
         return 1;
     cp = MYOBJ_gethello(t2);
-    assert(cp == NULL);
+    OPENSSL_assert(cp == NULL);
     if (cp != NULL)
         return 1;
+    t3 = MYOBJ_dup(t1);
+    cp = MYOBJ_gethello(t3);
+    OPENSSL_assert(cp == p);
+    if (cp != p)
+        return 1;
+    cp = MYOBJ_gethello(t2);
     MYOBJ_free(t1);
     MYOBJ_free(t2);
+    MYOBJ_free(t3);
     free(saved_argp);
     free(p);
     return 0;