C API: Safer function pointer casting

See NixOS#8699 (comment) Casting a function pointer to `void*` is undefined behavior in the C spec, since there are platforms with different sizes for these two kinds of pointers. A safe alternative might be `void (*callback)()`
tweag · Apr 12, 2024 · 01bad63 · 01bad63
1 parent 5b9cb8b
commit 01bad63
Show file tree

Hide file tree

Showing 8 changed files with 79 additions and 29 deletions.
diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc
@@ -15,9 +15,9 @@
 #include "value/context.hh"
 
 #ifdef HAVE_BOEHMGC
-# include "gc/gc.h"
-# define GC_INCLUDE_NEW 1
-# include "gc_cpp.h"
+#include "gc/gc.h"
+#define GC_INCLUDE_NEW 1
+#include "gc_cpp.h"
 #endif
 
 // Helper function to throw an exception if value is null
@@ -537,7 +537,7 @@ nix_realised_string * nix_string_realise(nix_c_context * context, EvalState * st
     if (context)
         context->last_err_code = NIX_OK;
     try {
-        auto &v = check_value_not_null(value);
+        auto & v = check_value_not_null(value);
         nix::NixStringContext stringContext;
         auto rawStr = state->state.coerceToString(nix::noPos, v, stringContext, "while realising a string").toOwned();
         nix::StorePathSet storePaths;
@@ -547,14 +547,11 @@ nix_realised_string * nix_string_realise(nix_c_context * context, EvalState * st
 
         // Convert to the C API StorePath type and convert to vector for index-based access
         std::vector<StorePath> vec;
-        for (auto &sp : storePaths) {
+        for (auto & sp : storePaths) {
             vec.push_back(StorePath{sp});
         }
 
-        return new nix_realised_string {
-            .str = s,
-            .storePaths = vec
-        };
+        return new nix_realised_string{.str = s, .storePaths = vec};
     }
     NIXC_CATCH_ERRS_NULL
 }

diff --git a/src/libstore-c/nix_api_store.cc b/src/libstore-c/nix_api_store.cc
@@ -56,7 +56,11 @@ void nix_store_free(Store * store)
     delete store;
 }
 
-nix_err nix_store_get_uri(nix_c_context * context, Store * store, void * callback, void * user_data)
+nix_err nix_store_get_uri(
+    nix_c_context * context,
+    Store * store,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data)
 {
     if (context)
         context->last_err_code = NIX_OK;
@@ -67,7 +71,11 @@ nix_err nix_store_get_uri(nix_c_context * context, Store * store, void * callbac
     NIXC_CATCH_ERRS
 }
 
-nix_err nix_store_get_version(nix_c_context * context, Store * store, void * callback, void * user_data)
+nix_err nix_store_get_version(
+    nix_c_context * context,
+    Store * store,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data)
 {
     if (context)
         context->last_err_code = NIX_OK;
@@ -128,13 +136,15 @@ nix_err nix_store_realise(
     NIXC_CATCH_ERRS
 }
 
-void nix_store_path_name(const StorePath *store_path, void * callback, void * user_data)
+void nix_store_path_name(
+    const StorePath * store_path,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data)
 {
     std::string_view name = store_path->path.name();
     ((nix_get_string_callback) callback)(name.data(), name.size(), user_data);
 }
 
-
 void nix_store_path_free(StorePath * sp)
 {
     delete sp;

diff --git a/src/libstore-c/nix_api_store.h b/src/libstore-c/nix_api_store.h
@@ -76,7 +76,11 @@ void nix_store_free(Store * store);
  * @see nix_get_string_callback
  * @return error code, NIX_OK on success.
  */
-nix_err nix_store_get_uri(nix_c_context * context, Store * store, void * callback, void * user_data);
+nix_err nix_store_get_uri(
+    nix_c_context * context,
+    Store * store,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data);
 
 // returns: owned StorePath*
 /**
@@ -97,7 +101,10 @@ StorePath * nix_store_parse_path(nix_c_context * context, Store * store, const c
  * @param[in] callback called with the name
  * @param[in] user_data arbitrary data, passed to the callback when it's called.
  */
-void nix_store_path_name(const StorePath *store_path, void * callback, void * user_data);
+void nix_store_path_name(
+    const StorePath * store_path,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data);
 
 /**
  * @brief Copy a StorePath
@@ -130,7 +137,8 @@ bool nix_store_is_valid_path(nix_c_context * context, Store * store, StorePath *
  *
  * Blocking, calls callback once for each realised output.
  *
- * @note When working with expressions, consider using e.g. nix_string_realise to get the output. `.drvPath` may not be accurate or available in the future. See https://github.com/NixOS/nix/issues/6507
+ * @note When working with expressions, consider using e.g. nix_string_realise to get the output. `.drvPath` may not be
+ * accurate or available in the future. See https://github.com/NixOS/nix/issues/6507
  *
  * @param[out] context Optional, stores error information
  * @param[in] store Nix Store reference
@@ -155,7 +163,11 @@ nix_err nix_store_realise(
  * @see nix_get_string_callback
  * @return error code, NIX_OK on success.
  */
-nix_err nix_store_get_version(nix_c_context * context, Store * store, void * callback, void * user_data);
+nix_err nix_store_get_version(
+    nix_c_context * context,
+    Store * store,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data);
 
 // cffi end
 #ifdef __cplusplus

diff --git a/src/libutil-c/nix_api_util.cc b/src/libutil-c/nix_api_util.cc
@@ -64,7 +64,11 @@ const char * nix_version_get()
 
 // Implementations
 
-nix_err nix_setting_get(nix_c_context * context, const char * key, void * callback, void * user_data)
+nix_err nix_setting_get(
+    nix_c_context * context,
+    const char * key,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data)
 {
     if (context)
         context->last_err_code = NIX_OK;
@@ -115,7 +119,11 @@ const char * nix_err_msg(nix_c_context * context, const nix_c_context * read_con
     return nullptr;
 }
 
-nix_err nix_err_name(nix_c_context * context, const nix_c_context * read_context, void * callback, void * user_data)
+nix_err nix_err_name(
+    nix_c_context * context,
+    const nix_c_context * read_context,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data)
 {
     if (context)
         context->last_err_code = NIX_OK;
@@ -125,7 +133,11 @@ nix_err nix_err_name(nix_c_context * context, const nix_c_context * read_context
     return call_nix_get_string_callback(read_context->name, callback, user_data);
 }
 
-nix_err nix_err_info_msg(nix_c_context * context, const nix_c_context * read_context, void * callback, void * user_data)
+nix_err nix_err_info_msg(
+    nix_c_context * context,
+    const nix_c_context * read_context,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data)
 {
     if (context)
         context->last_err_code = NIX_OK;
@@ -141,7 +153,8 @@ nix_err nix_err_code(const nix_c_context * read_context)
 }
 
 // internal
-nix_err call_nix_get_string_callback(const std::string str, void * callback, void * user_data)
+nix_err call_nix_get_string_callback(
+    const std::string str, void (*callback)(const char * start, unsigned int n, void * user_data), void * user_data)
 {
     ((nix_get_string_callback) callback)(str.c_str(), str.size(), user_data);
     return NIX_OK;

diff --git a/src/libutil-c/nix_api_util.h b/src/libutil-c/nix_api_util.h
@@ -175,7 +175,11 @@ nix_err nix_libutil_init(nix_c_context * context);
  * @return NIX_ERR_KEY if the setting is unknown, or NIX_OK if the setting was retrieved
  * successfully.
  */
-nix_err nix_setting_get(nix_c_context * context, const char * key, void * callback, void * user_data);
+nix_err nix_setting_get(
+    nix_c_context * context,
+    const char * key,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data);
 
 /**
  * @brief Sets a setting in the nix global configuration.
@@ -241,8 +245,11 @@ const char * nix_err_msg(nix_c_context * context, const nix_c_context * ctx, uns
  * @see nix_get_string_callback
  * @return NIX_OK if there were no errors, an error code otherwise.
  */
-nix_err
-nix_err_info_msg(nix_c_context * context, const nix_c_context * read_context, void * callback, void * user_data);
+nix_err nix_err_info_msg(
+    nix_c_context * context,
+    const nix_c_context * read_context,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data);
 
 /**
  * @brief Retrieves the error name from a context.
@@ -260,7 +267,11 @@ nix_err_info_msg(nix_c_context * context, const nix_c_context * read_context, vo
  * @see nix_get_string_callback
  * @return NIX_OK if there were no errors, an error code otherwise.
  */
-nix_err nix_err_name(nix_c_context * context, const nix_c_context * read_context, void * callback, void * user_data);
+nix_err nix_err_name(
+    nix_c_context * context,
+    const nix_c_context * read_context,
+    void (*callback)(const char * start, unsigned int n, void * user_data),
+    void * user_data);
 
 /**
  * @brief Retrieves the most recent error code from a nix_c_context

diff --git a/src/libutil-c/nix_api_util_internal.h b/src/libutil-c/nix_api_util_internal.h
@@ -29,7 +29,8 @@ nix_err nix_context_error(nix_c_context * context);
  * @return NIX_OK if there were no errors.
  * @see nix_get_string_callback
  */
-nix_err call_nix_get_string_callback(const std::string str, void * callback, void * user_data);
+nix_err call_nix_get_string_callback(
+    const std::string str, void (*callback)(const char * start, unsigned int n, void * user_data), void * user_data);
 
 #define NIXC_CATCH_ERRS \
     catch (...) \

diff --git a/tests/unit/libutil-support/tests/string_callback.cc b/tests/unit/libutil-support/tests/string_callback.cc
@@ -2,7 +2,8 @@
 
 namespace nix::testing {
 
-void observe_string_cb(const char * start, unsigned int n, std::string * user_data) {
+void observe_string_cb(const char * start, unsigned int n, std::string * user_data)
+{
     *user_data = std::string(start);
 }
 

diff --git a/tests/unit/libutil-support/tests/string_callback.hh b/tests/unit/libutil-support/tests/string_callback.hh
@@ -4,9 +4,14 @@
 namespace nix::testing {
 
 void observe_string_cb(const char * start, unsigned int n, std::string * user_data);
-inline void * observe_string_cb_data(std::string & out) {
+
+inline void * observe_string_cb_data(std::string & out)
+{
     return (void *) &out;
 };
-#define OBSERVE_STRING(str) (void *)nix::testing::observe_string_cb, nix::testing::observe_string_cb_data(str)
+
+#define OBSERVE_STRING(str) \
+    (void (*)(const char *, unsigned int, void *)) nix::testing::observe_string_cb, \
+        nix::testing::observe_string_cb_data(str)
 
 }