From c4e9a1106aa1a81a1894763f803abb462c7494cd Mon Sep 17 00:00:00 2001 From: Eval Exec Date: Sat, 26 Sep 2026 00:30:13 -0400 Subject: [PATCH] fix(dbusbind): resolve signal and method-call handlers before queueing `dbus-check-event` requires `functionp` on a `dbus-event`'s handler slot, and the port filled that slot only for method-return/error replies. Every incoming signal therefore queued an event carrying a nil handler, which the wait loop rejected with `dbus-error "Not a valid D-Bus event"`. One Avahi service-type browser signal was enough: byte-compiling `lisp/net/tramp-archive.el` runs its toplevel `(require 'tramp-gvfs)`, which calls `zeroconf-init`, which opens that browser; the file's exit status then takes the whole `cargo xtask fresh-build` down with it. Measured here: 9 of 10 isolated byte-compiles of that one file failed, while the pipeline failed whenever the reply landed inside that file's ~1.3s window. Port the method-call/signal arm of GNU `xd_read_message_1` (src/dbusbind.c:1864-1918): look the registration up under `(:method|:signal BUS INTERFACE MEMBER)`, plus the three wildcard keys `dbus-register-signal` fills when INTERFACE or SIGNAL is nil; match each entry on sender and object path; queue one event per distinct handler; and store nothing when no registration matches. Add the `(:monitor BUS)` arm (src/dbusbind.c:1921), which must not queue a handlerless event either. An error message carries its name in the D-Bus ERROR_NAME header field, not MEMBER, and GNU puts `dbus_message_get_error_name` into the member slot. Reading `Message::member` left that slot nil, so any error reply -- a peer's "no such method", a rejected call, a timeout -- was rejected the same way. Read it back through `Message::as_result`, since the dbus crate exposes no header-field accessor. Tests: two bus-free dispatch tests (an unregistered signal stores nothing; a registered one fills the handler slot and honours the path filter) and two session-bus tests (a directed signal and a directed method call reach their handlers). Verified: `cargo nextest run --release -p neovm-core --lib dbusbind` 10/10; `cargo xtask fresh-build --release` exit 0 with 1687/1687 files; the isolated byte-compile of lisp/net/tramp-archive.el 10/10 (was 1/10). Not addressed here: Lisp cannot send replies (`dbus-message-internal` type 2/3 answers "Unable to create a return message"), so a dispatched method call runs its handler but cannot answer its caller. --- .../src/emacs_core/system/dbusbind/event.rs | 234 ++++++++++++++++-- .../emacs_core/system/dbusbind/tests/mod.rs | 1 + .../system/dbusbind/tests/signal.rs | 208 ++++++++++++++++ 3 files changed, 417 insertions(+), 26 deletions(-) create mode 100644 crates/neovm-core/src/emacs_core/system/dbusbind/tests/signal.rs diff --git a/crates/neovm-core/src/emacs_core/system/dbusbind/event.rs b/crates/neovm-core/src/emacs_core/system/dbusbind/event.rs index c8880c846a..baf0e84a7f 100644 --- a/crates/neovm-core/src/emacs_core/system/dbusbind/event.rs +++ b/crates/neovm-core/src/emacs_core/system/dbusbind/event.rs @@ -1,11 +1,19 @@ //! Incoming `dbus-event` construction — GNU `xd_read_message_1`. +//! +//! GNU stores one `dbus-event` per registration that matches the message, and +//! stores nothing at all when nothing matches (`src/dbusbind.c:1864-1918`). +//! The handler slot is therefore never nil, and it must not be: +//! `dbus-check-event` (`lisp/net/dbus.el`) requires `functionp` on it, and +//! signals the whole message back as "Not a valid D-Bus event" otherwise. +//! A message nobody registered for -- Avahi's `CacheExhausted` on the +//! service-type browser `zeroconf-init` opens, say -- is dropped. use dbus::arg::ArgType; use dbus::message::MessageType; use crate::emacs_core::error::Flow; use crate::emacs_core::eval::Context; -use crate::emacs_core::value::Value; +use crate::emacs_core::value::{Value, eq_value}; use super::connection; use super::types::retrieve_arg; @@ -14,12 +22,19 @@ pub(super) fn drain(ctx: &mut Context) -> Result { let incoming = connection::pump_messages()?; let count = incoming.len(); for (bus, message) in incoming { - queue_event(ctx, bus.to_lisp(), message)?; + queue_events(ctx, bus.to_lisp(), message)?; } Ok(count) } -fn queue_event(ctx: &mut Context, bus: Value, message: dbus::Message) -> Result<(), Flow> { +/// Build the `dbus-event`s one incoming message produces. +/// +/// `bus` is the Lisp value (`:system`, `:session`) the message arrived on. +pub(super) fn queue_events( + ctx: &mut Context, + bus: Value, + mut message: dbus::Message, +) -> Result<(), Flow> { let mtype = match message.msg_type() { MessageType::MethodCall => 1, MessageType::MethodReturn => 2, @@ -42,35 +57,175 @@ fn queue_event(ctx: &mut Context, bus: Value, message: dbus::Message) -> Result< let _ = dbus::arg::Iter::next(&mut iter); } - let member_or_error = optional_str(message.member()); + let sender = message.sender().map(|sender| sender.to_string()); + let destination = message.destination().map(|name| name.to_string()); + let path = message.path().map(|path| path.to_string()); + let interface = message.interface().map(|name| name.to_string()); + // GNU's member slot is the error name for `DBUS_MESSAGE_TYPE_ERROR` + // (`dbus_message_get_error_name`), which lives in the D-Bus ERROR_NAME + // header field -- `Message::member` reads MEMBER and is empty there. + // `dbus-check-event` requires a string in that slot for every type but + // method-return, so an error reply has to be named. + let member = match message.msg_type() { + MessageType::Error => message + .as_result() + .err() + .and_then(|error| error.name().map(str::to_owned)), + _ => message.member().map(|name| name.to_string()), + }; - let mut event = vec![ + // `(dbus-event BUS TYPE SERIAL SERVICE DESTINATION PATH INTERFACE MEMBER + // HANDLER . ARGS)`. GNU appends the handler to this prefix in + // `xd_store_event`, once per matching registration. + let prefix = vec![ Value::symbol("dbus-event"), bus, Value::fixnum(mtype), Value::fixnum(serial as i64), - optional_str(message.sender()), - optional_str(message.destination()), - optional_str(message.path()), - optional_str(message.interface()), - member_or_error, + opt_string(&sender), + opt_string(&destination), + opt_string(&path), + opt_string(&interface), + opt_string(&member), ]; - let handler = if matches!( - message.msg_type(), - MessageType::MethodReturn | MessageType::Error - ) { - lookup_handler(ctx, bus, serial)? - } else { - Value::NIL - }; - event.push(handler); - event.extend(args); - ctx.queue_special_event(Value::list(event)); + match message.msg_type() { + // Answered from the `(:serial BUS SERIAL)` reply registry, which the + // caller filled with `dbus-message-internal`'s HANDLER argument. + MessageType::MethodReturn | MessageType::Error => { + let handler = take_serial_handler(ctx, bus, serial)?; + if !handler.is_nil() { + store_event(ctx, &prefix, handler, &args); + } + } + // Every registration that matches the message is called; one event + // per handler, each handler at most once. + MessageType::MethodCall | MessageType::Signal => { + dispatch_registered( + ctx, + bus, + mtype, + &prefix, + &sender, + &path, + opt_string(&interface), + opt_string(&member), + &args, + )?; + } + } + + // `monitor:` (`src/dbusbind.c:1921`) — every valid message also reaches a + // registered monitor, in addition to the handlers above. + if let Some(handler) = monitor_handler(ctx, bus)? { + store_event(ctx, &prefix, handler, &args); + } Ok(()) } -fn lookup_handler(ctx: &mut Context, bus: Value, serial: u32) -> Result { +/// The method-call/signal arm of `xd_read_message_1`. +#[allow(clippy::too_many_arguments)] +fn dispatch_registered( + ctx: &mut Context, + bus: Value, + mtype: i64, + prefix: &[Value], + sender: &Option, + path: &Option, + interface: Value, + member: Value, + args: &[Value], +) -> Result<(), Flow> { + // "Vdbus_registered_objects_table requires non-nil interface and member." + if interface.is_nil() || member.is_nil() { + return Ok(()); + } + + let mut called: Vec = Vec::new(); + for entry in registered_entries(ctx, bus, mtype, interface, member)? { + // An entry is `(UNAME SERVICE PATH HANDLER RULE)`. GNU matches the + // sender against UNAME and the object path against PATH, and ignores + // SERVICE -- the daemon already routed the message to this bus name. + let key_uname = car(entry); + if let Some(sender) = sender.as_deref() + && !key_uname.is_nil() + && key_uname.as_utf8_str() != Some(sender) + { + continue; + } + let key_path = car(cdr(cdr(entry))); + if let Some(path) = path.as_deref() + && !key_path.is_nil() + && key_path.as_utf8_str() != Some(path) + { + continue; + } + let handler = car(cdr(cdr(cdr(entry)))); + if handler.is_nil() { + continue; + } + if called.iter().any(|seen| eq_value(seen, &handler)) { + continue; + } + called.push(handler); + store_event(ctx, prefix, handler, args); + } + Ok(()) +} + +/// The registration lists one message is dispatched against. +/// +/// The exact `(KIND BUS INTERFACE MEMBER)` key first, then -- for signals -- +/// the three wildcard keys `dbus-register-signal` also fills when INTERFACE +/// or SIGNAL is nil (`src/dbusbind.c:1872-1890`). +fn registered_entries( + ctx: &mut Context, + bus: Value, + mtype: i64, + interface: Value, + member: Value, +) -> Result, Flow> { + let Some(table) = ctx.obarray.symbol_value("dbus-registered-objects-table") else { + return Ok(Vec::new()); + }; + let kind = Value::keyword_by_name(if mtype == 1 { ":method" } else { ":signal" }); + let mut keys = vec![Value::list(vec![kind, bus, interface, member])]; + if mtype != 1 { + keys.push(Value::list(vec![kind, bus, Value::NIL, member])); + keys.push(Value::list(vec![kind, bus, interface, Value::NIL])); + keys.push(Value::list(vec![kind, bus, Value::NIL, Value::NIL])); + } + + let mut entries = Vec::new(); + for key in keys { + let value = crate::emacs_core::builtins::builtin_gethash(vec![key, *table, Value::NIL])?; + let mut rest = value; + while rest.is_cons() { + entries.push(rest.cons_car()); + rest = rest.cons_cdr(); + } + } + Ok(entries) +} + +/// A registered monitor's handler: the first entry's HANDLER. +fn monitor_handler(ctx: &mut Context, bus: Value) -> Result, Flow> { + let Some(table) = ctx.obarray.symbol_value("dbus-registered-objects-table") else { + return Ok(None); + }; + let key = Value::list(vec![Value::keyword_by_name(":monitor"), bus]); + let value = crate::emacs_core::builtins::builtin_gethash(vec![key, *table, Value::NIL])?; + if !value.is_cons() { + return Ok(None); + } + // An entry is `(UNAME SERVICE PATH HANDLER RULE)`; `BecomeMonitor` gives + // the monitor the whole bus, so only its handler is used. + let handler = car(cdr(cdr(cdr(value.cons_car())))); + Ok(if handler.is_nil() { None } else { Some(handler) }) +} + +/// Take the `dbus-message-internal` handler registered for a reply serial. +fn take_serial_handler(ctx: &mut Context, bus: Value, serial: u32) -> Result { let Some(table) = ctx.obarray.symbol_value("dbus-registered-objects-table") else { return Ok(Value::NIL); }; @@ -90,8 +245,35 @@ fn lookup_handler(ctx: &mut Context, bus: Value, serial: u32) -> Result) -> Value { - value - .map(|s| Value::string(s.to_string())) - .unwrap_or(Value::NIL) +/// GNU `xd_store_event`: queue `(HANDLER . EVENT-PREFIX) ++ ARGS`. +fn store_event(ctx: &mut Context, prefix: &[Value], handler: Value, args: &[Value]) { + let mut event = prefix.to_vec(); + event.push(handler); + event.extend_from_slice(args); + ctx.queue_special_event(Value::list(event)); +} + +/// GNU `CAR_SAFE`. +fn car(value: Value) -> Value { + if value.is_cons() { + value.cons_car() + } else { + Value::NIL + } +} + +/// GNU `CDR_SAFE`. +fn cdr(value: Value) -> Value { + if value.is_cons() { + value.cons_cdr() + } else { + Value::NIL + } +} + +fn opt_string(value: &Option) -> Value { + match value { + Some(text) => Value::string(text.clone()), + None => Value::NIL, + } } diff --git a/crates/neovm-core/src/emacs_core/system/dbusbind/tests/mod.rs b/crates/neovm-core/src/emacs_core/system/dbusbind/tests/mod.rs index 6b0f8d0bec..06bacde3f1 100644 --- a/crates/neovm-core/src/emacs_core/system/dbusbind/tests/mod.rs +++ b/crates/neovm-core/src/emacs_core/system/dbusbind/tests/mod.rs @@ -4,6 +4,7 @@ std::cfg_select! { neomacs_have_dbus => { mod call; mod connection; + mod signal; mod types; } _ => {} diff --git a/crates/neovm-core/src/emacs_core/system/dbusbind/tests/signal.rs b/crates/neovm-core/src/emacs_core/system/dbusbind/tests/signal.rs new file mode 100644 index 0000000000..e462b999d7 --- /dev/null +++ b/crates/neovm-core/src/emacs_core/system/dbusbind/tests/signal.rs @@ -0,0 +1,208 @@ +//! Signal and method-call dispatch — GNU `xd_read_message_1`'s +//! `(:signal ...)` / `(:method ...)` arms. +//! +//! `unregistered_signal_stores_no_event` and +//! `registered_signal_fills_the_handler_slot` need no bus: they exercise +//! `dbus-registered-objects-table` and the input queue, both of which exist at +//! startup. The end-to-end tests need the session bus and are skipped without +//! one, like `call.rs`. + +use crate::emacs_core::eval::Context; +use crate::emacs_core::value::{Value, eq_value}; + +use super::super::event; + +fn session() -> Value { + Value::keyword_by_name(":session") +} + +fn signal_message(path: &str, interface: &str, member: &str) -> dbus::Message { + dbus::Message::new_signal(path, interface, member).expect("signal message") +} + +/// The event's HANDLER slot: nine fields precede it +/// (`BUS TYPE SERIAL SERVICE DESTINATION PATH INTERFACE MEMBER`). +fn handler_slot(queued: Value) -> Value { + let mut rest = queued; + for _ in 0..9 { + rest = rest.cons_cdr(); + } + rest.cons_car() +} + +fn load_dbus(eval: &mut Context) -> bool { + match eval.eval_str("(progn (require 'dbus) (dbus-ignore-errors (dbus-get-unique-name :session)))") + { + Ok(name) if name.is_string() => true, + Ok(_) => false, + Err(err) => panic!("(require 'dbus) failed: {err:?}"), + } +} + +/// A message nobody registered for stores nothing. +/// +/// GNU never queues a `dbus-event` with a nil handler, and `dbus-check-event` +/// rejects that shape with "Not a valid D-Bus event". One unhandled signal — +/// Avahi's `CacheExhausted` on the service-type browser `zeroconf-init` opens — +/// therefore aborted whatever the wait loop was pumping, which is how +/// byte-compiling `lisp/net/tramp-archive.el` (its toplevel +/// `(require 'tramp-gvfs)`) failed a whole `fresh-build`. +#[test] +fn unregistered_signal_stores_no_event() { + crate::test_utils::init_test_tracing(); + super::super::reset_thread_locals(); + let mut eval = crate::test_utils::runtime_startup_context(); + + event::queue_events( + &mut eval, + session(), + signal_message("/neomacs/test", "org.neomacs.Test", "Boom"), + ) + .expect("dispatch should not fail"); + + assert!( + eval.command_loop.keyboard.kboard.unread_events.is_empty(), + "a signal with no registration must not queue a dbus-event" + ); + super::super::reset_thread_locals(); +} + +/// A registered handler lands in the handler slot, and the path filter still +/// applies. +#[test] +fn registered_signal_fills_the_handler_slot() { + crate::test_utils::init_test_tracing(); + super::super::reset_thread_locals(); + let mut eval = crate::test_utils::runtime_startup_context(); + + eval.eval_str( + r#"(progn + (defun neomacs-signal-test-handler (&rest _) t) + (puthash '(:signal :session "org.neomacs.Test" "Boom") + (list (list nil nil "/neomacs/test" 'neomacs-signal-test-handler nil)) + dbus-registered-objects-table) + t)"#, + ) + .expect("registering the signal should work"); + + event::queue_events( + &mut eval, + session(), + signal_message("/neomacs/test", "org.neomacs.Test", "Boom"), + ) + .expect("dispatch should not fail"); + + let registered = eval + .eval_str("'neomacs-signal-test-handler") + .expect("handler symbol"); + let queued = eval + .command_loop + .keyboard + .kboard + .unread_events + .pop_front() + .expect("a registered signal queues one dbus-event"); + let slot = handler_slot(queued); + assert!( + eq_value(&slot, ®istered), + "handler slot should hold the registered handler, got {slot:?}" + ); + assert!( + eval.command_loop.keyboard.kboard.unread_events.is_empty(), + "one message queues one event per handler" + ); + + // The same signal at another object path matches nothing. + event::queue_events( + &mut eval, + session(), + signal_message("/neomacs/other", "org.neomacs.Test", "Boom"), + ) + .expect("dispatch should not fail"); + assert!( + eval.command_loop.keyboard.kboard.unread_events.is_empty(), + "a path the registration does not name must not match" + ); + super::super::reset_thread_locals(); +} + +/// A directed signal reaches the handler `dbus-register-signal` installed. +#[test] +fn directed_signal_reaches_its_handler() { + crate::test_utils::init_test_tracing(); + super::super::reset_thread_locals(); + let mut eval = crate::test_utils::runtime_startup_context(); + if !load_dbus(&mut eval) { + super::super::reset_thread_locals(); + return; + } + + let calls = eval + .eval_str( + r#"(progn + (defvar neomacs-signal-test-calls nil) + (defun neomacs-signal-test-handler (&rest args) + (setq neomacs-signal-test-calls (cons args neomacs-signal-test-calls))) + (dbus-register-signal + :session nil "/neomacs/test" "org.neomacs.Test" "Boom" + #'neomacs-signal-test-handler) + (dbus-send-signal + :session (dbus-get-unique-name :session) + "/neomacs/test" "org.neomacs.Test" "Boom" :string "payload") + ;; Pump the wait loop so the queued dbus-event is dispatched. + (ignore-errors (read-event nil nil 0.5)) + neomacs-signal-test-calls)"#, + ) + .unwrap_or_else(|err| panic!("signal dispatch failed: {err:?}")); + + assert!( + !calls.is_nil(), + "the handler registered for the signal should have been called" + ); + super::super::reset_thread_locals(); +} + +/// A directed method call reaches the handler `dbus-register-method` installed. +#[test] +fn directed_method_call_reaches_its_handler() { + crate::test_utils::init_test_tracing(); + super::super::reset_thread_locals(); + let mut eval = crate::test_utils::runtime_startup_context(); + if !load_dbus(&mut eval) { + super::super::reset_thread_locals(); + return; + } + + let report = eval + .eval_str( + r#"(condition-case err + (progn + (defvar neomacs-method-test-calls nil) + (defvar neomacs-method-test-error nil) + (defun neomacs-method-test-handler (&rest args) + (setq neomacs-method-test-calls (cons args neomacs-method-test-calls)) + :ignore) + (dbus-register-method + :session nil "/neomacs/test" "org.neomacs.Test" "Call" + #'neomacs-method-test-handler + ;; DONT-REGISTER-SERVICE: SERVICE is nil here, and + ;; registering it would ask the bus to RequestName nil. + t) + ;; Sent without a reply handler, so this does not wait. + (dbus-message-internal + 1 :session (dbus-get-unique-name :session) + "/neomacs/test" "org.neomacs.Test" "Call" nil) + (condition-case e (read-event nil nil 0.5) + (error (setq neomacs-method-test-error (format "%S" e)))) + (format "calls=%S error=%S" + neomacs-method-test-calls neomacs-method-test-error)) + (error (format "OUTER=%S" err)))"#, + ) + .expect("the form itself should evaluate"); + let text = report.as_utf8_str().unwrap_or(""); + assert!( + text.starts_with("calls=(nil)"), + "the handler registered for the method should have been called: {text}" + ); + super::super::reset_thread_locals(); +}