From 01bad63c720cb3d7280484f87f3ff9734b2b7117 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20Luis=20Lafuente?= <jl@lafuente.dev>
Date: Fri, 12 Apr 2024 21:41:15 +0200
Subject: [PATCH 1/3] C API: Safer function pointer casting

See https://github.com/NixOS/nix/pull/8699#discussion_r1554312181

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)()`
---
 src/libexpr-c/nix_api_value.cc                | 15 ++++++-------
 src/libstore-c/nix_api_store.cc               | 18 ++++++++++++----
 src/libstore-c/nix_api_store.h                | 20 ++++++++++++++----
 src/libutil-c/nix_api_util.cc                 | 21 +++++++++++++++----
 src/libutil-c/nix_api_util.h                  | 19 +++++++++++++----
 src/libutil-c/nix_api_util_internal.h         |  3 ++-
 .../libutil-support/tests/string_callback.cc  |  3 ++-
 .../libutil-support/tests/string_callback.hh  |  9 ++++++--
 8 files changed, 79 insertions(+), 29 deletions(-)

diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc
index fd1bfc165..87e25f9d9 100644
--- 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
index 511ba0fad..aa4ab521a 100644
--- 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
index ca8996681..efeedbf7b 100644
--- 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
index 8d0f7ac38..4999e28e9 100644
--- 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
index cb506ca90..36a3f76cb 100644
--- 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
index 6e8eac020..fe5d5db18 100644
--- 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
index 28ac8b10c..2d0e0dad0 100644
--- 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
index 808fb707b..6420810b6 100644
--- 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)
 
 }

From 76444a395825cca5e00b3eecef78109740b24114 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20Luis=20Lafuente?= <jl@lafuente.dev>
Date: Sun, 14 Apr 2024 16:18:32 +0200
Subject: [PATCH 2/3] C API: proper `ifdef` `endif` indentation

---
 src/libexpr-c/nix_api_value.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc
index 87e25f9d9..817464fa8 100644
--- 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

From 774e7213e85447e159b93437f5f269a2f14621ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20Luis=20Lafuente?= <jl@lafuente.dev>
Date: Mon, 15 Apr 2024 12:05:57 +0200
Subject: [PATCH 3/3] C API: Use `nix_get_string_callback` typedef

---
 src/libstore-c/nix_api_store.cc               | 20 +++++-------------
 src/libstore-c/nix_api_store.h                | 18 ++++------------
 src/libutil-c/nix_api_util.cc                 | 21 +++++--------------
 src/libutil-c/nix_api_util.h                  | 16 +++-----------
 src/libutil-c/nix_api_util_internal.h         |  3 +--
 .../libutil-support/tests/string_callback.hh  |  3 +--
 6 files changed, 19 insertions(+), 62 deletions(-)

diff --git a/src/libstore-c/nix_api_store.cc b/src/libstore-c/nix_api_store.cc
index aa4ab521a..6ce4d01bb 100644
--- a/src/libstore-c/nix_api_store.cc
+++ b/src/libstore-c/nix_api_store.cc
@@ -56,11 +56,7 @@ void nix_store_free(Store * store)
     delete store;
 }
 
-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)
+nix_err nix_store_get_uri(nix_c_context * context, Store * store, nix_get_string_callback callback, void * user_data)
 {
     if (context)
         context->last_err_code = NIX_OK;
@@ -71,11 +67,8 @@ nix_err nix_store_get_uri(
     NIXC_CATCH_ERRS
 }
 
-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)
+nix_err
+nix_store_get_version(nix_c_context * context, Store * store, nix_get_string_callback callback, void * user_data)
 {
     if (context)
         context->last_err_code = NIX_OK;
@@ -136,13 +129,10 @@ nix_err nix_store_realise(
     NIXC_CATCH_ERRS
 }
 
-void nix_store_path_name(
-    const StorePath * store_path,
-    void (*callback)(const char * start, unsigned int n, void * user_data),
-    void * user_data)
+void nix_store_path_name(const StorePath * store_path, nix_get_string_callback callback, void * user_data)
 {
     std::string_view name = store_path->path.name();
-    ((nix_get_string_callback) callback)(name.data(), name.size(), user_data);
+    callback(name.data(), name.size(), user_data);
 }
 
 void nix_store_path_free(StorePath * sp)
diff --git a/src/libstore-c/nix_api_store.h b/src/libstore-c/nix_api_store.h
index efeedbf7b..c83aca3f7 100644
--- a/src/libstore-c/nix_api_store.h
+++ b/src/libstore-c/nix_api_store.h
@@ -76,11 +76,7 @@ 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)(const char * start, unsigned int n, void * user_data),
-    void * user_data);
+nix_err nix_store_get_uri(nix_c_context * context, Store * store, nix_get_string_callback callback, void * user_data);
 
 // returns: owned StorePath*
 /**
@@ -101,10 +97,7 @@ 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)(const char * start, unsigned int n, void * user_data),
-    void * user_data);
+void nix_store_path_name(const StorePath * store_path, nix_get_string_callback callback, void * user_data);
 
 /**
  * @brief Copy a StorePath
@@ -163,11 +156,8 @@ 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)(const char * start, unsigned int n, void * user_data),
-    void * user_data);
+nix_err
+nix_store_get_version(nix_c_context * context, Store * store, nix_get_string_callback callback, 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
index 4999e28e9..0a9b49345 100644
--- a/src/libutil-c/nix_api_util.cc
+++ b/src/libutil-c/nix_api_util.cc
@@ -64,11 +64,7 @@ const char * nix_version_get()
 
 // Implementations
 
-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)
+nix_err nix_setting_get(nix_c_context * context, const char * key, nix_get_string_callback callback, void * user_data)
 {
     if (context)
         context->last_err_code = NIX_OK;
@@ -120,10 +116,7 @@ const char * nix_err_msg(nix_c_context * context, const nix_c_context * read_con
 }
 
 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)
+    nix_c_context * context, const nix_c_context * read_context, nix_get_string_callback callback, void * user_data)
 {
     if (context)
         context->last_err_code = NIX_OK;
@@ -134,10 +127,7 @@ nix_err nix_err_name(
 }
 
 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)
+    nix_c_context * context, const nix_c_context * read_context, nix_get_string_callback callback, void * user_data)
 {
     if (context)
         context->last_err_code = NIX_OK;
@@ -153,9 +143,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)(const char * start, unsigned int n, void * user_data), void * user_data)
+nix_err call_nix_get_string_callback(const std::string str, nix_get_string_callback callback, void * user_data)
 {
-    ((nix_get_string_callback) callback)(str.c_str(), str.size(), user_data);
+    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
index 36a3f76cb..e0ca04e69 100644
--- a/src/libutil-c/nix_api_util.h
+++ b/src/libutil-c/nix_api_util.h
@@ -175,11 +175,7 @@ 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)(const char * start, unsigned int n, void * user_data),
-    void * user_data);
+nix_err nix_setting_get(nix_c_context * context, const char * key, nix_get_string_callback callback, void * user_data);
 
 /**
  * @brief Sets a setting in the nix global configuration.
@@ -246,10 +242,7 @@ const char * nix_err_msg(nix_c_context * context, const nix_c_context * ctx, uns
  * @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)(const char * start, unsigned int n, void * user_data),
-    void * user_data);
+    nix_c_context * context, const nix_c_context * read_context, nix_get_string_callback callback, void * user_data);
 
 /**
  * @brief Retrieves the error name from a context.
@@ -268,10 +261,7 @@ nix_err nix_err_info_msg(
  * @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)(const char * start, unsigned int n, void * user_data),
-    void * user_data);
+    nix_c_context * context, const nix_c_context * read_context, nix_get_string_callback callback, 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
index fe5d5db18..aa829feaf 100644
--- a/src/libutil-c/nix_api_util_internal.h
+++ b/src/libutil-c/nix_api_util_internal.h
@@ -29,8 +29,7 @@ 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)(const char * start, unsigned int n, void * user_data), void * user_data);
+nix_err call_nix_get_string_callback(const std::string str, nix_get_string_callback callback, void * user_data);
 
 #define NIXC_CATCH_ERRS \
     catch (...) \
diff --git a/tests/unit/libutil-support/tests/string_callback.hh b/tests/unit/libutil-support/tests/string_callback.hh
index 6420810b6..3a3e545e9 100644
--- a/tests/unit/libutil-support/tests/string_callback.hh
+++ b/tests/unit/libutil-support/tests/string_callback.hh
@@ -11,7 +11,6 @@ inline void * observe_string_cb_data(std::string & out)
 };
 
 #define OBSERVE_STRING(str) \
-    (void (*)(const char *, unsigned int, void *)) nix::testing::observe_string_cb, \
-        nix::testing::observe_string_cb_data(str)
+  (nix_get_string_callback) nix::testing::observe_string_cb, nix::testing::observe_string_cb_data(str)
 
 }