GNOME Bugzilla – Bug 598339
segfault in gpm_disks_register() on error == NULL
Last modified: 2009-10-14 07:56:11 UTC
Recently, we started to get lots of crash reports (https://launchpad.net/bugs/431023) about gpm segfaulting when pulling the AC plug:
+ Trace 218285
This shows that it's crashing in gpm_disks_register() in -------------- 8< ------------- /* set spindown timeouts */ ret = dbus_g_proxy_call (disks->priv->proxy, "DriveSetAllSpindownTimeouts", &error, G_TYPE_INT, timeout, G_TYPE_STRV, options, G_TYPE_INVALID, G_TYPE_STRING, disks->priv->cookie, G_TYPE_INVALID); if (!ret) { egg_warning ("failed to set spindown timeout: %s", error->message); -------------- 8< ------------- in the last line, on a NULL pointer access (as apport figured out from the disassembly/registers). The only plausible cause for this is that dbus_g_proxy_call() returned FALSE without setting error. According to [1] this isn't supposed to happen, so there might be an underlying bug in dbus-glib. For robustness, would you consider explicitly checking for NULL there? (I'll attach a patch in a minute) [1] http://library.gnome.org/devel/dbus-glib/unstable/dbus-glib-DBusGProxy.html#dbus-g-proxy-call
Created attachment 145381 [details] [review] fix error messages/comments While fixing that, I noticed that the error messages/comments in gpm-disks.c were wrong; it's actually talking to DK-disks, not DK-power. Attaching git formatted patch for "git am" love.
Created attachment 145382 [details] [review] fix crash
Chris Coulson pointed out that bug 23617 is a likely cause for this condition; it's a code path in dbus-glib which returns FALSE/error=NULL on a demarshalling error.
As Chris Coulson spotted, the actual bug is the missing indirection in the return value. It should be &disks->priv->cookie.
Sorry, that was a bug in the fd.o bug tracker: http://bugs.freedesktop.org/show_bug.cgi?id=23617
Heh, I was just about to comment there but got a collision :)
Created attachment 145385 [details] [review] git formatted patch Chris' change in git formatted patch version.
Comment on attachment 145385 [details] [review] git formatted patch Whoops, no need to change the one in unregister.
I've committed the change now (only in gpm_disks_register). I can't change the status of this bug to fixed though ;)
Chris, thanks! Chris, Richard, can you please also apply my first patch ("fix error messages/comments")?
(In reply to comment #4) > As Chris Coulson spotted, the actual bug is the missing indirection in the > return value. It should be &disks->priv->cookie. Excellent, thanks. (In reply to comment #10) > Chris, thanks! Chris, Richard, can you please also apply my first patch ("fix > error messages/comments")? Applied, thanks. I've also marked this bug as fixed. I've been unwell for the last couple of days, and it was nice to come back to a nicely fixed bug like this. Thanks guys!