[PATCH v1 proxmox] router: compare the Fn trait object instead of the ApiHandler address

[PATCH v1 proxmox] router: compare the Fn trait object instead of the ApiHandler address

[Robert Obkircher]

The previous implementation was effectively `ptr::eq(self, rhs)`,
because it compared the addresses of the fields instead of the trait
object fat pointers.

This worked anyway, because the tests only compare const values that
the compiler manages to deduplicate.

I discovered this problem because those handler tests fail under miri.
However, this change does not fix that, because miri assigns a unique
address every time a reference to a function is created, meaning even
`ptr::eq(&some_fn, &some_fn)` evaluates to false.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-router/src/router.rs | 56 ++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

diff --git a/proxmox-router/src/router.rs b/proxmox-router/src/router.rs
index fea47ba6..a92c527d 100644
--- a/proxmox-router/src/router.rs
+++ b/proxmox-router/src/router.rs
@@ -514,42 +514,36 @@ impl Eq for ApiHandler {}
 #[cfg(feature = "test-harness")]
 impl PartialEq for ApiHandler {
     fn eq(&self, rhs: &Self) -> bool {
-        unsafe {
-            #[allow(clippy::missing_transmute_annotations)]
-            match (self, rhs) {
-                (ApiHandler::Sync(l), ApiHandler::Sync(r)) => {
-                    core::mem::transmute::<_, usize>(l) == core::mem::transmute::<_, usize>(r)
-                }
-                (ApiHandler::SerializingSync(l), ApiHandler::SerializingSync(r)) => {
-                    core::mem::transmute::<_, usize>(l) == core::mem::transmute::<_, usize>(r)
-                }
-                (ApiHandler::StreamSync(l), ApiHandler::StreamSync(r)) => {
-                    core::mem::transmute::<_, usize>(l) == core::mem::transmute::<_, usize>(r)
-                }
-                (ApiHandler::Async(l), ApiHandler::Async(r)) => {
-                    core::mem::transmute::<_, usize>(l) == core::mem::transmute::<_, usize>(r)
-                }
-                (ApiHandler::SerializingAsync(l), ApiHandler::SerializingAsync(r)) => {
-                    core::mem::transmute::<_, usize>(l) == core::mem::transmute::<_, usize>(r)
-                }
-                (ApiHandler::StreamAsync(l), ApiHandler::StreamAsync(r)) => {
-                    core::mem::transmute::<_, usize>(l) == core::mem::transmute::<_, usize>(r)
-                }
-                #[cfg(feature = "server")]
-                (ApiHandler::AsyncHttp(l), ApiHandler::AsyncHttp(r)) => {
-                    core::mem::transmute::<_, usize>(l) == core::mem::transmute::<_, usize>(r)
-                }
-                #[cfg(feature = "server")]
-                (
-                    ApiHandler::AsyncHttpBodyParameters(l),
-                    ApiHandler::AsyncHttpBodyParameters(r),
-                ) => core::mem::transmute::<_, usize>(l) == core::mem::transmute::<_, usize>(r),
-                _ => false,
+        // This can result in false positives and false negatives (see ptr::fn_addr_eq).
+        use core::ptr::eq;
+        match (self, rhs) {
+            (ApiHandler::Sync(l), ApiHandler::Sync(r)) => eq(*l, *r),
+            (ApiHandler::SerializingSync(l), ApiHandler::SerializingSync(r)) => eq(*l, *r),
+            (ApiHandler::StreamSync(l), ApiHandler::StreamSync(r)) => eq(*l, *r),
+            (ApiHandler::Async(l), ApiHandler::Async(r)) => eq(*l, *r),
+            (ApiHandler::SerializingAsync(l), ApiHandler::SerializingAsync(r)) => eq(*l, *r),
+            (ApiHandler::StreamAsync(l), ApiHandler::StreamAsync(r)) => eq(*l, *r),
+            #[cfg(feature = "server")]
+            (ApiHandler::AsyncHttp(l), ApiHandler::AsyncHttp(r)) => eq(*l, *r),
+            #[cfg(feature = "server")]
+            (ApiHandler::AsyncHttpBodyParameters(l), ApiHandler::AsyncHttpBodyParameters(r)) => {
+                eq(*l, *r)
             }
+            _ => false,
         }
     }
 }
 
+#[test]
+#[cfg(feature = "test-harness")]
+fn test_handler_eq() {
+    static DUMMY: ApiHandlerFn = &|_, _, _| unimplemented!();
+    static H1: ApiHandler = ApiHandler::Sync(DUMMY);
+    static H2: ApiHandler = ApiHandler::Sync(DUMMY);
+    assert!(!core::ptr::eq(&H1, &H2)); // statics have unique addresses
+    assert!(H1 == H2); // this used to fail because it compared &&dyn instead of &dyn
+}
+
 /// Lookup table to child `Router`s
 ///
 /// Stores a sorted list of `(name, router)` tuples:
-- 
2.47.3

applied: [PATCH v1 proxmox] router: compare the Fn trait object instead of the ApiHandler address

[Thomas Lamprecht]

On Fri, 14 Aug 2026 14:40:48 +0200, Robert Obkircher wrote:
> The previous implementation was effectively `ptr::eq(self, rhs)`,
> because it compared the addresses of the fields instead of the trait
> object fat pointers.
> 
> This worked anyway, because the tests only compare const values that
> the compiler manages to deduplicate.
> 
> [...]

Applied, thanks!

[1/1] router: compare the Fn trait object instead of the ApiHandler address
      commit: 9bf4383b6ad4225367c7824023ad74769388de8f