Hi!
Interesting crash in dnsmasq were reported to me. I can reproduce it
reliably on RHEL9, but not anymore on the most recent Fedora. But the
difference seems to be based on used dbus library, not depending on
dnsmasq code. RHEL9 dbus libraries installs 2 daemon->watchers, Fedora
rawhide version does not. If those watchers are present and dbus system
instance is restarted, dnsmasq crashes. RHEL9 uses
dbus-libs-1.12.20-7.el9_1.x86_64.
If I configure dnsmasq to use dbus and then restart dbus.service with
watchers present, it crashes dnsmasq. The reason is simple, it uses loop
to walk over watchers to call dbus handling code. But from that code the
same list can be modified and watchers removed. But the list iteration
continues anyway.
In my first patch I fixed that problem by restarting the loop if list
were modified.
Second patch is just optimization of socket events handling of dbus.
Reduces calls to locate the file descriptor structure. Should lower CPU
usage when monitoring dbus watches. It is not directly related to the
issue, I just noticed repeated function calls where just one were needed.
Red Hat bug: https://bugzilla.redhat.com/show_bug.cgi?id=2185878
Cheers,
Petr
--
Petr Menšík
Software Engineer, RHEL
Red Hat, https://www.redhat.com/
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB
From 7abc1f58988b1f299ce1dd008c48890314f3581b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= <pemen...@redhat.com>
Date: Thu, 13 Apr 2023 14:31:22 +0200
Subject: [PATCH 2/2] Small optimization around dbus socket events
Reduce calls to fd_search function and use get or set all flags by a
single call. Then do some bit operations and set the flags.
This is done in hope compiler will be optimize separate flag checks and
using just single integer. DBUS_WATCH_READABLE should equal to POLLIN on
normal system anyway.
---
src/dbus.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/dbus.c b/src/dbus.c
index c2142b4..f58fb46 100644
--- a/src/dbus.c
+++ b/src/dbus.c
@@ -961,14 +961,14 @@ void set_dbus_listeners(void)
{
unsigned int flags = dbus_watch_get_flags(w->watch);
int fd = dbus_watch_get_unix_fd(w->watch);
+ int poll_flags = POLLERR;
if (flags & DBUS_WATCH_READABLE)
- poll_listen(fd, POLLIN);
-
+ poll_flags |= POLLIN;
if (flags & DBUS_WATCH_WRITABLE)
- poll_listen(fd, POLLOUT);
+ poll_flags |= POLLOUT;
- poll_listen(fd, POLLERR);
+ poll_listen(fd, poll_flags);
}
}
@@ -982,14 +982,13 @@ static int check_dbus_watches()
{
unsigned int flags = 0;
int fd = dbus_watch_get_unix_fd(w->watch);
-
- if (poll_check(fd, POLLIN))
+ int poll_flags = poll_check(fd, POLLIN|POLLOUT|POLLERR);
+
+ if ((poll_flags & POLLIN) != 0)
flags |= DBUS_WATCH_READABLE;
-
- if (poll_check(fd, POLLOUT))
+ if ((poll_flags & POLLOUT) != 0)
flags |= DBUS_WATCH_WRITABLE;
-
- if (poll_check(fd, POLLERR))
+ if ((poll_flags & POLLERR) != 0)
flags |= DBUS_WATCH_ERROR;
if (flags != 0)
--
2.39.2
From 9ac7a5433527f88b3603c00769c4545011abaa17 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= <pemen...@redhat.com>
Date: Thu, 13 Apr 2023 12:59:17 +0200
Subject: [PATCH 1/2] Watch modification of dbus watches list
If watches are modified during dbus_watch_handle() call, existing
watch pointers might be invalidated. Always restart the loop if
modification is detected. Should prevent crashes when dbus service is
restarted.
---
src/dbus.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/src/dbus.c b/src/dbus.c
index 360985a..c2142b4 100644
--- a/src/dbus.c
+++ b/src/dbus.c
@@ -106,6 +106,7 @@ const char* introspection_xml_template =
"</node>\n";
static char *introspection_xml = NULL;
+static int watches_modified = 0;
struct watch {
DBusWatch *watch;
@@ -127,6 +128,7 @@ static dbus_bool_t add_watch(DBusWatch *watch, void *data)
w->watch = watch;
w->next = daemon->watches;
daemon->watches = w;
+ watches_modified++;
(void)data; /* no warning */
return TRUE;
@@ -134,7 +136,7 @@ static dbus_bool_t add_watch(DBusWatch *watch, void *data)
static void remove_watch(DBusWatch *watch, void *data)
{
- struct watch **up, *w, *tmp;
+ struct watch **up, *w, *tmp;
for (up = &(daemon->watches), w = daemon->watches; w; w = tmp)
{
@@ -143,6 +145,7 @@ static void remove_watch(DBusWatch *watch, void *data)
{
*up = tmp;
free(w);
+ watches_modified++;
}
else
up = &(w->next);
@@ -969,11 +972,11 @@ void set_dbus_listeners(void)
}
}
-void check_dbus_listeners()
+static int check_dbus_watches()
{
- DBusConnection *connection = (DBusConnection *)daemon->dbus;
struct watch *w;
+ watches_modified = 0;
for (w = daemon->watches; w; w = w->next)
if (dbus_watch_get_enabled(w->watch))
{
@@ -990,9 +993,22 @@ void check_dbus_listeners()
flags |= DBUS_WATCH_ERROR;
if (flags != 0)
- dbus_watch_handle(w->watch, flags);
+ {
+ dbus_watch_handle(w->watch, flags);
+ if (watches_modified)
+ return 0;
+ }
}
+ return 1;
+}
+
+void check_dbus_listeners()
+{
+ DBusConnection *connection = (DBusConnection *)daemon->dbus;
+
+ while (!check_dbus_watches()) ;
+
if (connection)
{
dbus_connection_ref (connection);
--
2.39.2
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
