Introduce hash thunking functions to do proper casting

ubsan on clang17 has started warning about the following undefined behavior: crypto/lhash/lhash.c:299:12: runtime error: call to function err_string_data_hash through pointer to incorrect function type 'unsigned long (*)(const void *)' [...]/crypto/err/err.c:184: note: err_string_data_hash defined here #0 0x7fa569e3a434 in getrn [...]/crypto/lhash/lhash.c:299:12 #1 0x7fa569e39a46 in OPENSSL_LH_insert [...]/crypto/lhash/lhash.c:119:10 #2 0x7fa569d866ee in err_load_strings [...]/crypto/err/err.c:280:15 [...] The issue occurs because, the generic hash functions (OPENSSL_LH_*) will occasionaly call back to the type specific registered functions for hash generation/comparison/free/etc, using functions of the (example) prototype: [return value] <hash|cmp|free> (void *, [void *], ...) While the functions implementing hash|cmp|free|etc are defined as [return value] <fnname> (TYPE *, [TYPE *], ...) The compiler, not knowing the type signature of the function pointed to by the implementation, performs no type conversion on the function arguments While the C language specification allows for pointers to data of one type to be converted to pointers of another type, it does not allow for pointers to functions with one signature to be called while pointing to functions of another signature. Compilers often allow this behavior, but strictly speaking it results in undefined behavior As such, ubsan warns us about this issue This is an potential fix for the issue, implemented using, in effect, thunking macros. For each hash type, an additional set of wrapper funtions is created (currently for compare and hash, but more will be added for free/doall/etc). The corresponding thunking macros for each type cases the actuall corresponding callback to a function pointer of the proper type, and then calls that with the parameters appropriately cast, avoiding the ubsan warning This approach is adventageous as it maintains a level of type safety, but comes at the cost of having to implement several additional functions per hash table type. Related to openssl#22896 Reviewed-by: Sasa Nedvedicky <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#23192)
arthenica · Jan 17, 2024 · 5c42ced · 5c42ced
1 parent 2464d8d
commit 5c42ced
Show file tree

Hide file tree

Showing 6 changed files with 150 additions and 21 deletions.
diff --git a/crypto/lhash/lhash.c b/crypto/lhash/lhash.c
@@ -44,6 +44,22 @@ static int expand(OPENSSL_LHASH *lh);
 static void contract(OPENSSL_LHASH *lh);
 static OPENSSL_LH_NODE **getrn(OPENSSL_LHASH *lh, const void *data, unsigned long *rhash);
 
+OPENSSL_LHASH *OPENSSL_LH_set_thunks(OPENSSL_LHASH *lh,
+                                     OPENSSL_LH_HASHFUNCTHUNK hw,
+                                     OPENSSL_LH_COMPFUNCTHUNK cw,
+                                     OPENSSL_LH_DOALL_FUNC_THUNK daw,
+                                     OPENSSL_LH_DOALL_FUNCARG_THUNK daaw)
+{
+
+    if (lh == NULL)
+        return NULL;
+    lh->compw = cw;
+    lh->hashw = hw;
+    lh->daw = daw;
+    lh->daaw = daaw;
+    return lh;
+}
+
 OPENSSL_LHASH *OPENSSL_LH_new(OPENSSL_LH_HASHFUNC h, OPENSSL_LH_COMPFUNC c)
 {
     OPENSSL_LHASH *ret;
@@ -168,8 +184,11 @@ void *OPENSSL_LH_retrieve(OPENSSL_LHASH *lh, const void *data)
 }
 
 static void doall_util_fn(OPENSSL_LHASH *lh, int use_arg,
+                          OPENSSL_LH_DOALL_FUNC_THUNK wfunc,
                           OPENSSL_LH_DOALL_FUNC func,
-                          OPENSSL_LH_DOALL_FUNCARG func_arg, void *arg)
+                          OPENSSL_LH_DOALL_FUNCARG func_arg,
+                          OPENSSL_LH_DOALL_FUNCARG_THUNK wfunc_arg,
+                          void *arg)
 {
     int i;
     OPENSSL_LH_NODE *a, *n;
@@ -186,22 +205,39 @@ static void doall_util_fn(OPENSSL_LHASH *lh, int use_arg,
         while (a != NULL) {
             n = a->next;
             if (use_arg)
-                func_arg(a->data, arg);
+                wfunc_arg(a->data, arg, func_arg);
             else
-                func(a->data);
+                wfunc(a->data, func);
             a = n;
         }
     }
 }
 
 void OPENSSL_LH_doall(OPENSSL_LHASH *lh, OPENSSL_LH_DOALL_FUNC func)
 {
-    doall_util_fn(lh, 0, func, (OPENSSL_LH_DOALL_FUNCARG)0, NULL);
+    if (lh == NULL)
+        return;
+
+    doall_util_fn(lh, 0, lh->daw, func, (OPENSSL_LH_DOALL_FUNCARG)NULL,
+                  (OPENSSL_LH_DOALL_FUNCARG_THUNK)NULL, NULL);
 }
 
-void OPENSSL_LH_doall_arg(OPENSSL_LHASH *lh, OPENSSL_LH_DOALL_FUNCARG func, void *arg)
+void OPENSSL_LH_doall_arg(OPENSSL_LHASH *lh,
+                          OPENSSL_LH_DOALL_FUNCARG func, void *arg)
 {
-    doall_util_fn(lh, 1, (OPENSSL_LH_DOALL_FUNC)0, func, arg);
+    if (lh == NULL)
+        return;
+
+    doall_util_fn(lh, 1, (OPENSSL_LH_DOALL_FUNC_THUNK)NULL,
+                  (OPENSSL_LH_DOALL_FUNC)NULL, func, lh->daaw, arg);
+}
+
+void OPENSSL_LH_doall_arg_thunk(OPENSSL_LHASH *lh,
+                                OPENSSL_LH_DOALL_FUNCARG_THUNK daaw,
+                                OPENSSL_LH_DOALL_FUNCARG fn, void *arg)
+{
+    doall_util_fn(lh, 1, (OPENSSL_LH_DOALL_FUNC_THUNK)NULL,
+                  (OPENSSL_LH_DOALL_FUNC)NULL, fn, daaw, arg);
 }
 
 static int expand(OPENSSL_LHASH *lh)
@@ -286,24 +322,32 @@ static OPENSSL_LH_NODE **getrn(OPENSSL_LHASH *lh,
 {
     OPENSSL_LH_NODE **ret, *n1;
     unsigned long hash, nn;
-    OPENSSL_LH_COMPFUNC cf;
 
-    hash = (*(lh->hash)) (data);
+    if (lh->hashw != NULL)
+        hash = lh->hashw(data, lh->hash);
+    else
+        hash = lh->hash(data);
+
     *rhash = hash;
 
     nn = hash % lh->pmax;
     if (nn < lh->p)
         nn = hash % lh->num_alloc_nodes;
 
-    cf = lh->comp;
     ret = &(lh->b[(int)nn]);
     for (n1 = *ret; n1 != NULL; n1 = n1->next) {
         if (n1->hash != hash) {
             ret = &(n1->next);
             continue;
         }
-        if (cf(n1->data, data) == 0)
-            break;
+
+        if (lh->compw != NULL) {
+            if (lh->compw(n1->data, data, lh->comp) == 0)
+                break;
+        } else {
+            if (lh->comp(n1->data, data) == 0)
+                break;
+        }
         ret = &(n1->next);
     }
     return ret;

diff --git a/crypto/lhash/lhash_local.h b/crypto/lhash/lhash_local.h
@@ -20,6 +20,10 @@ struct lhash_st {
     OPENSSL_LH_NODE **b;
     OPENSSL_LH_COMPFUNC comp;
     OPENSSL_LH_HASHFUNC hash;
+    OPENSSL_LH_HASHFUNCTHUNK hashw;
+    OPENSSL_LH_COMPFUNCTHUNK compw;
+    OPENSSL_LH_DOALL_FUNC_THUNK daw;
+    OPENSSL_LH_DOALL_FUNCARG_THUNK daaw;
     unsigned int num_nodes;
     unsigned int num_alloc_nodes;
     unsigned int p;

diff --git a/doc/man3/OPENSSL_LH_COMPFUNC.pod b/doc/man3/OPENSSL_LH_COMPFUNC.pod
@@ -12,7 +12,8 @@ lh_TYPE_doall, lh_TYPE_doall_arg, lh_TYPE_num_items, lh_TYPE_get_down_load,
 lh_TYPE_set_down_load, lh_TYPE_error,
 OPENSSL_LH_new, OPENSSL_LH_free,  OPENSSL_LH_flush,
 OPENSSL_LH_insert, OPENSSL_LH_delete, OPENSSL_LH_retrieve,
-OPENSSL_LH_doall, OPENSSL_LH_doall_arg, OPENSSL_LH_num_items,
+OPENSSL_LH_doall, OPENSSL_LH_doall_arg, OPENSSL_LH_doall_arg_thunk,
+OPENSSL_LH_set_thunks, OPENSSL_LH_num_items,
 OPENSSL_LH_get_down_load, OPENSSL_LH_set_down_load, OPENSSL_LH_error
 - dynamic hash table
 
@@ -29,6 +30,11 @@ OPENSSL_LH_get_down_load, OPENSSL_LH_set_down_load, OPENSSL_LH_error
  LHASH_OF(TYPE) *lh_TYPE_new(OPENSSL_LH_HASHFUNC hash, OPENSSL_LH_COMPFUNC compare);
  void lh_TYPE_free(LHASH_OF(TYPE) *table);
  void lh_TYPE_flush(LHASH_OF(TYPE) *table);
+ OPENSSL_LHASH *OPENSSL_LH_set_thunks(OPENSSL_LHASH *lh,
+                                      OPENSSL_LH_HASHFUNCTHUNK hw,
+                                      OPENSSL_LH_COMPFUNCTHUNK cw,
+                                      OPENSSL_LH_DOALL_FUNC_THUNK daw,
+                                      OPENSSL_LH_DOALL_FUNCARG_THUNK daaw)
 
  TYPE *lh_TYPE_insert(LHASH_OF(TYPE) *table, TYPE *data);
  TYPE *lh_TYPE_delete(LHASH_OF(TYPE) *table, TYPE *data);
@@ -37,6 +43,9 @@ OPENSSL_LH_get_down_load, OPENSSL_LH_set_down_load, OPENSSL_LH_error
  void lh_TYPE_doall(LHASH_OF(TYPE) *table, OPENSSL_LH_DOALL_FUNC func);
  void lh_TYPE_doall_arg(LHASH_OF(TYPE) *table, OPENSSL_LH_DOALL_FUNCARG func,
                         TYPE *arg);
+ void OPENSSL_LH_doall_arg_thunk(OPENSSL_LHASH *lh,
+                                 OPENSSL_LH_DOALL_FUNCARG_THUNK daaw,
+                                 OPENSSL_LH_DOALL_FUNCARG fn, void *arg)
 
  unsigned long lh_TYPE_num_items(OPENSSL_LHASH *lh);
  unsigned long lh_TYPE_get_down_load(OPENSSL_LHASH *lh);
@@ -232,6 +241,9 @@ B<lh_I<TYPE>> functions are implemented as type checked wrappers around the
 B<OPENSSL_LH> functions. Most applications should not call the B<OPENSSL_LH>
 functions directly.
 
+OPENSSL_LH_set_thunks() and OPENSSL_LH_doall_arg_thunk(), while public by
+necessity, are actually internal functions and should not be used.
+
 =head1 RETURN VALUES
 
 B<lh_I<TYPE>_new>() and OPENSSL_LH_new() return NULL on error, otherwise a

diff --git a/include/openssl/lhash.h.in b/include/openssl/lhash.h.in
@@ -36,9 +36,13 @@ extern "C" {
 
 typedef struct lhash_node_st OPENSSL_LH_NODE;
 typedef int (*OPENSSL_LH_COMPFUNC) (const void *, const void *);
+typedef int (*OPENSSL_LH_COMPFUNCTHUNK) (const void *, const void *, OPENSSL_LH_COMPFUNC cfn);
 typedef unsigned long (*OPENSSL_LH_HASHFUNC) (const void *);
+typedef unsigned long (*OPENSSL_LH_HASHFUNCTHUNK) (const void *, OPENSSL_LH_HASHFUNC hfn);
 typedef void (*OPENSSL_LH_DOALL_FUNC) (void *);
+typedef void (*OPENSSL_LH_DOALL_FUNC_THUNK) (void *, OPENSSL_LH_DOALL_FUNC doall);
 typedef void (*OPENSSL_LH_DOALL_FUNCARG) (void *, void *);
+typedef void (*OPENSSL_LH_DOALL_FUNCARG_THUNK) (void *, void *, OPENSSL_LH_DOALL_FUNCARG doall);
 typedef struct lhash_st OPENSSL_LHASH;
 
 /*
@@ -84,13 +88,23 @@ typedef struct lhash_st OPENSSL_LHASH;
 
 int OPENSSL_LH_error(OPENSSL_LHASH *lh);
 OPENSSL_LHASH *OPENSSL_LH_new(OPENSSL_LH_HASHFUNC h, OPENSSL_LH_COMPFUNC c);
+OPENSSL_LHASH *OPENSSL_LH_set_thunks(OPENSSL_LHASH *lh,
+                                     OPENSSL_LH_HASHFUNCTHUNK hw,
+                                     OPENSSL_LH_COMPFUNCTHUNK cw,
+                                     OPENSSL_LH_DOALL_FUNC_THUNK daw,
+                                     OPENSSL_LH_DOALL_FUNCARG_THUNK daaw);
 void OPENSSL_LH_free(OPENSSL_LHASH *lh);
 void OPENSSL_LH_flush(OPENSSL_LHASH *lh);
 void *OPENSSL_LH_insert(OPENSSL_LHASH *lh, void *data);
 void *OPENSSL_LH_delete(OPENSSL_LHASH *lh, const void *data);
 void *OPENSSL_LH_retrieve(OPENSSL_LHASH *lh, const void *data);
 void OPENSSL_LH_doall(OPENSSL_LHASH *lh, OPENSSL_LH_DOALL_FUNC func);
-void OPENSSL_LH_doall_arg(OPENSSL_LHASH *lh, OPENSSL_LH_DOALL_FUNCARG func, void *arg);
+void OPENSSL_LH_doall_arg(OPENSSL_LHASH *lh,
+                          OPENSSL_LH_DOALL_FUNCARG func, void *arg);
+void OPENSSL_LH_doall_arg_thunk(OPENSSL_LHASH *lh,
+                          OPENSSL_LH_DOALL_FUNCARG_THUNK daaw,
+                          OPENSSL_LH_DOALL_FUNCARG fn, void *arg);
+
 unsigned long OPENSSL_LH_strhash(const char *c);
 unsigned long OPENSSL_LH_num_items(const OPENSSL_LHASH *lh);
 unsigned long OPENSSL_LH_get_down_load(const OPENSSL_LHASH *lh);
@@ -144,6 +158,26 @@ OSSL_DEPRECATEDIN_3_1 void OPENSSL_LH_node_usage_stats_bio(const OPENSSL_LHASH *
     typedef int (*lh_##type##_compfunc)(const type *a, const type *b); \
     typedef unsigned long (*lh_##type##_hashfunc)(const type *a); \
     typedef void (*lh_##type##_doallfunc)(type *a); \
+    static ossl_inline unsigned long lh_##type##_hash_thunk(const void *data, OPENSSL_LH_HASHFUNC hfn) \
+    { \
+        unsigned long (*hfn_conv)(const type *) = (unsigned long (*)(const type *))hfn; \
+        return hfn_conv((const type *)data); \
+    } \
+    static ossl_inline int lh_##type##_comp_thunk(const void *da, const void *db, OPENSSL_LH_COMPFUNC cfn) \
+    { \
+        int (*cfn_conv)(const type *, const type *) = (int (*)(const type *, const type *))cfn; \
+        return cfn_conv((const type *)da, (const type *)db); \
+    } \
+    static ossl_inline void lh_##type##_doall_thunk(void *node, OPENSSL_LH_DOALL_FUNC doall) \
+    { \
+        void (*doall_conv)(type *) = (void (*)(type *))doall; \
+        doall_conv((type *)node); \
+    } \
+    static ossl_inline void lh_##type##_doall_arg_thunk(void *node, void *arg, OPENSSL_LH_DOALL_FUNCARG doall) \
+    { \
+        void (*doall_conv)(type *, void *) = (void (*)(type *, void *))doall; \
+        doall_conv((type *)node, arg); \
+    } \
     static ossl_unused ossl_inline type *\
     ossl_check_##type##_lh_plain_type(type *ptr) \
     { \
@@ -206,12 +240,16 @@ OSSL_DEPRECATEDIN_3_1 void OPENSSL_LH_node_usage_stats_bio(const OPENSSL_LHASH *
     LHASH_OF(type) { \
         union lh_##type##_dummy { void* d1; unsigned long d2; int d3; } dummy; \
     }; \
-    static ossl_unused ossl_inline LHASH_OF(type) * \
-    lh_##type##_new(unsigned long (*hfn)(const type *), \
-                    int (*cfn)(const type *, const type *)) \
+    static unsigned long \
+    lh_##type##_hfn_thunk(const void *data, OPENSSL_LH_HASHFUNC hfn) \
     { \
-        return (LHASH_OF(type) *) \
-            OPENSSL_LH_new((OPENSSL_LH_HASHFUNC)hfn, (OPENSSL_LH_COMPFUNC)cfn); \
+        unsigned long (*hfn_conv)(const type *) = (unsigned long (*)(const type *))hfn; \
+        return hfn_conv((const type *)data); \
+    } \
+    static int lh_##type##_cfn_thunk(const void *da, const void *db, OPENSSL_LH_COMPFUNC cfn) \
+    { \
+        int (*cfn_conv)(const type *, const type *) = (int (*)(const type *, const type *))cfn; \
+        return cfn_conv((const type *)da, (const type *)db); \
     } \
     static ossl_unused ossl_inline void \
     lh_##type##_free(LHASH_OF(type) *lh) \
@@ -259,10 +297,31 @@ OSSL_DEPRECATEDIN_3_1 void OPENSSL_LH_node_usage_stats_bio(const OPENSSL_LHASH *
         OPENSSL_LH_set_down_load((OPENSSL_LHASH *)lh, dl); \
     } \
     static ossl_unused ossl_inline void \
+    lh_##type##_doall_thunk(void *node, OPENSSL_LH_DOALL_FUNC doall) \
+    { \
+        void (*doall_conv)(type *) = (void (*)(type *))doall; \
+        doall_conv((type *)node); \
+    } \
+    static ossl_unused ossl_inline void \
+    lh_##type##_doall_arg_thunk(void *node, void *arg, OPENSSL_LH_DOALL_FUNCARG doall) \
+    { \
+        void (*doall_conv)(type *, void *) = (void (*)(type *, void *))doall; \
+        doall_conv((type *)node, arg); \
+    } \
+    static ossl_unused ossl_inline void \
     lh_##type##_doall(LHASH_OF(type) *lh, void (*doall)(type *)) \
     { \
         OPENSSL_LH_doall((OPENSSL_LHASH *)lh, (OPENSSL_LH_DOALL_FUNC)doall); \
     } \
+    static ossl_unused ossl_inline LHASH_OF(type) * \
+    lh_##type##_new(unsigned long (*hfn)(const type *), \
+                    int (*cfn)(const type *, const type *)) \
+    { \
+        return (LHASH_OF(type) *)OPENSSL_LH_set_thunks(OPENSSL_LH_new((OPENSSL_LH_HASHFUNC)hfn, (OPENSSL_LH_COMPFUNC)cfn), \
+                                lh_##type##_hfn_thunk, lh_##type##_cfn_thunk, \
+                                lh_##type##_doall_thunk, \
+                                lh_##type##_doall_arg_thunk); \
+    } \
     static ossl_unused ossl_inline void \
     lh_##type##_doall_arg(LHASH_OF(type) *lh, \
                           void (*doallarg)(type *, void *), void *arg) \
@@ -284,13 +343,21 @@ OSSL_DEPRECATEDIN_3_1 void OPENSSL_LH_node_usage_stats_bio(const OPENSSL_LHASH *
     int_implement_lhash_doall(type, argtype, type)
 
 #define int_implement_lhash_doall(type, argtype, cbargtype) \
+    static ossl_unused ossl_inline void \
+    lh_##type##_doall_##argtype##_thunk(void *node, void *arg, OPENSSL_LH_DOALL_FUNCARG fn) \
+    { \
+        void (*fn_conv)(cbargtype *, argtype *) = (void (*)(cbargtype *, argtype *))fn; \
+        fn_conv((cbargtype *)node, (argtype *)arg); \
+    } \
     static ossl_unused ossl_inline void \
         lh_##type##_doall_##argtype(LHASH_OF(type) *lh, \
                                    void (*fn)(cbargtype *, argtype *), \
                                    argtype *arg) \
     { \
-        OPENSSL_LH_doall_arg((OPENSSL_LHASH *)lh, \
-                             (OPENSSL_LH_DOALL_FUNCARG)fn, (void *)arg); \
+        OPENSSL_LH_doall_arg_thunk((OPENSSL_LHASH *)lh, \
+                             lh_##type##_doall_##argtype##_thunk, \
+                             (OPENSSL_LH_DOALL_FUNCARG)fn, \
+                             (void *)arg); \
     } \
     LHASH_OF(type)
 

diff --git a/util/libcrypto.num b/util/libcrypto.num
@@ -5544,3 +5544,5 @@ OSSL_CMP_SRV_CTX_init_trans             ?	3_3_0	EXIST::FUNCTION:CMP
 EVP_DigestSqueeze                       ?	3_3_0	EXIST::FUNCTION:
 ERR_pop                                 ?	3_3_0	EXIST::FUNCTION:
 X509_STORE_get1_objects                 ?	3_3_0	EXIST::FUNCTION:
+OPENSSL_LH_set_thunks                   ?	3_3_0	EXIST::FUNCTION:
+OPENSSL_LH_doall_arg_thunk              ?	3_3_0	EXIST::FUNCTION:
diff --git a/util/perl/OpenSSL/stackhash.pm b/util/perl/OpenSSL/stackhash.pm
@@ -85,7 +85,7 @@ sub generate_lhash_macros {
 
     my $macros = <<END_MACROS;
 DEFINE_LHASH_OF_INTERNAL(${type});
-#define lh_${type}_new(hfn, cmp) ((LHASH_OF(${type}) *)OPENSSL_LH_new(ossl_check_${type}_lh_hashfunc_type(hfn), ossl_check_${type}_lh_compfunc_type(cmp)))
+#define lh_${type}_new(hfn, cmp) ((LHASH_OF(${type}) *)OPENSSL_LH_set_thunks(OPENSSL_LH_new(ossl_check_${type}_lh_hashfunc_type(hfn), ossl_check_${type}_lh_compfunc_type(cmp)), lh_${type}_hash_thunk, lh_${type}_comp_thunk, lh_${type}_doall_thunk, lh_${type}_doall_arg_thunk))
 #define lh_${type}_free(lh) OPENSSL_LH_free(ossl_check_${type}_lh_type(lh))
 #define lh_${type}_flush(lh) OPENSSL_LH_flush(ossl_check_${type}_lh_type(lh))
 #define lh_${type}_insert(lh, ptr) ((${type} *)OPENSSL_LH_insert(ossl_check_${type}_lh_type(lh), ossl_check_${type}_lh_plain_type(ptr)))