From 01bad63c720cb3d7280484f87f3ff9734b2b7117 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Lafuente?= Date: Fri, 12 Apr 2024 21:41:15 +0200 Subject: [PATCH] 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 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) }