[v4,3/4] qapi: Do not cast function pointers

Commit Message

Akihiko Odaki May 24, 2024, 5:35 a.m. UTC

-fsanitize=undefined complains if function pointers are casted. It
also prevents enabling teh strict mode of CFI which is currently
disabled with -fsanitize-cfi-icall-generalize-pointers.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2346
Signed-off-by: Akihiko Odaki <[e-mail]>
---
 include/qapi/clone-visitor.h | 37 ++++++++++++++++++++++++-------------
 qapi/qapi-clone-visitor.c    | 30 ++++--------------------------
 2 files changed, 28 insertions(+), 39 deletions(-)

Comments

Markus Armbruster May 28, 2024, 11:02 a.m. UTC | #1

Akihiko Odaki <[e-mail]> writes:

> -fsanitize=undefined complains if function pointers are casted. It
> also prevents enabling teh strict mode of CFI which is currently

Typo: the

> disabled with -fsanitize-cfi-icall-generalize-pointers.

The above describes the problem the patch solves.  Good!  Two
suggestions:

1. Quote the error message.

2. Briefly describe the solution as well.  Perhaps:

  The problematic casts are necessary to pass visit_type_T() and
  visit_type_T_members() as callbacks to qapi_clone() and
  qapi_clone_members(), respectively.  Open-code these two functions to
  avoid the callbacks, and thus the type casts.

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2346
> Signed-off-by: Akihiko Odaki <[e-mail]>

Always kind of sad to move implementation code to headers, but getting
rid of the function pointer casts makes sense, and I don't have better
ideas for doing that.

With an improved commit message
Reviewed-by: Markus Armbruster <[e-mail]>

Patch

diff --git a/include/qapi/clone-visitor.h b/include/qapi/clone-visitor.h
index adf9a788e232..ebc182b034d7 100644
--- a/include/qapi/clone-visitor.h
+++ b/include/qapi/clone-visitor.h
@@ -11,6 +11,7 @@ 
 #ifndef QAPI_CLONE_VISITOR_H
 #define QAPI_CLONE_VISITOR_H
 
+#include "qapi/error.h"
 #include "qapi/visitor.h"
 
 /*
@@ -20,11 +21,8 @@ 
  */
 typedef struct QapiCloneVisitor QapiCloneVisitor;
 
-void *qapi_clone(const void *src, bool (*visit_type)(Visitor *, const char *,
-                                                     void **, Error **));
-void qapi_clone_members(void *dst, const void *src, size_t sz,
-                        bool (*visit_type_members)(Visitor *, void *,
-                                                   Error **));
+Visitor *qapi_clone_visitor_new(void);
+Visitor *qapi_clone_members_visitor_new(void);
 
 /*
  * Deep-clone QAPI object @src of the given @type, and return the result.
@@ -32,10 +30,18 @@  void qapi_clone_members(void *dst, const void *src, size_t sz,
  * Not usable on QAPI scalars (integers, strings, enums), nor on a
  * QAPI object that references the 'any' type.  Safe when @src is NULL.
  */
-#define QAPI_CLONE(type, src)                                           \
-    ((type *)qapi_clone(src,                                            \
-                        (bool (*)(Visitor *, const char *, void **,     \
-                                  Error **))visit_type_ ## type))
+#define QAPI_CLONE(type, src)                                   \
+    ({                                                          \
+        Visitor *v_;                                            \
+        type *dst_ = (type *) (src); /* Cast away const */      \
+                                                                \
+        if (dst_) {                                             \
+            v_ = qapi_clone_visitor_new();                      \
+            visit_type_ ## type(v_, NULL, &dst_, &error_abort); \
+            visit_free(v_);                                     \
+        }                                                       \
+        dst_;                                                   \
+    })
 
 /*
  * Copy deep clones of @type members from @src to @dst.
@@ -43,9 +49,14 @@  void qapi_clone_members(void *dst, const void *src, size_t sz,
  * Not usable on QAPI scalars (integers, strings, enums), nor on a
  * QAPI object that references the 'any' type.
  */
-#define QAPI_CLONE_MEMBERS(type, dst, src)                              \
-    qapi_clone_members(dst, src, sizeof(type),                          \
-                       (bool (*)(Visitor *, void *,                     \
-                                 Error **))visit_type_ ## type ## _members)
+#define QAPI_CLONE_MEMBERS(type, dst, src)                                \
+    ({                                                                    \
+        Visitor *v_;                                                      \
+                                                                          \
+        v_ = qapi_clone_members_visitor_new();                            \
+        *(type *)(dst) = *(src);                                          \
+        visit_type_ ## type ## _members(v_, (type *)(dst), &error_abort); \
+        visit_free(v_);                                                   \
+    })
 
 #endif
diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c
index c45c5caa3b89..bbf953698f38 100644
--- a/qapi/qapi-clone-visitor.c
+++ b/qapi/qapi-clone-visitor.c
@@ -149,7 +149,7 @@  static void qapi_clone_free(Visitor *v)
     g_free(v);
 }
 
-static Visitor *qapi_clone_visitor_new(void)
+Visitor *qapi_clone_visitor_new(void)
 {
     QapiCloneVisitor *v;
 
@@ -174,31 +174,9 @@  static Visitor *qapi_clone_visitor_new(void)
     return &v->visitor;
 }
 
-void *qapi_clone(const void *src, bool (*visit_type)(Visitor *, const char *,
-                                                     void **, Error **))
+Visitor *qapi_clone_members_visitor_new(void)
 {
-    Visitor *v;
-    void *dst = (void *) src; /* Cast away const */
-
-    if (!src) {
-        return NULL;
-    }
-
-    v = qapi_clone_visitor_new();
-    visit_type(v, NULL, &dst, &error_abort);
-    visit_free(v);
-    return dst;
-}
-
-void qapi_clone_members(void *dst, const void *src, size_t sz,
-                        bool (*visit_type_members)(Visitor *, void *,
-                                                   Error **))
-{
-    Visitor *v;
-
-    v = qapi_clone_visitor_new();
-    memcpy(dst, src, sz);
+    Visitor *v = qapi_clone_visitor_new();
     to_qcv(v)->depth++;
-    visit_type_members(v, dst, &error_abort);
-    visit_free(v);
+    return v;
 }