NULL pointer dereference in xfsm_manager_dbus_suspend()
Submitted by Maciej S. Szmigiero
Assigned to Xfce Bug Triage
Description
A NULL pointer dereference can happen in xfsm_manager_dbus_suspend() on suspend in some conditions, causing a crash of xfce4-session and so a termination of the whole session.
Specifically, it happens when xfsm_shutdown_try_suspend() returns false while leaving the "error" variable unset. Then the "error->message" expression in throw_error() call in the next line will cause a NULL pointer dereference.
xfsm_shutdown_try_suspend() does not set the "error" variable if its call xfsm_shutdown_fallback_try_action() did not do that. xfsm_shutdown_fallback_try_action() can do so at least in the following situations:
-
A call to lock_screen() failed but didn't set the "error" variable,
-
There was no back-end compiled-in to perform the actual suspend operation.
In fact, the lock_screen() function never sets the "error" variable so if it fails there will be a NULL pointer dereference in xfsm_manager_dbus_suspend(), crashing xfce4-session. I've observed this on my system when there was an issue with communication with the screensaver.
A simplest workaround to avoid the crash is to change the code like this: diff --git a/xfce4-session/xfsm-manager.c b/xfce4-session/xfsm-manager.c --- a/xfce4-session/xfsm-manager.c +++ b/xfce4-session/xfsm-manager.c @@ -2424,7 +2424,7 @@ xfsm_manager_dbus_suspend (XfsmDbusManager *object, g_return_val_if_fail (XFSM_IS_MANAGER (object), FALSE); if (xfsm_shutdown_try_suspend (XFSM_MANAGER (object)->shutdown_helper, &error) == FALSE) {
-
throw_error (invocation, XFSM_ERROR_BAD_STATE, error->message);
-
throw_error (invocation, XFSM_ERROR_BAD_STATE, error ? error->message : "UNKNOWN ERROR"); g_clear_error (&error); return TRUE;
However, there is a lot of similar code in xfce4-session, so I think that for consistency the xfsm_shutdown_fallback_try_action() and lock_screen() functions should be modified instead to always set the "error" variable when they return with a failure.
BTW. The third argument to throw_error() is actually a printf()-like format string.
Passing a variable content as a format string invites format string vulnerabilities, so I think this and similar calls really should use something like this: diff --git a/xfce4-session/xfsm-manager.c b/xfce4-session/xfsm-manager.c --- a/xfce4-session/xfsm-manager.c +++ b/xfce4-session/xfsm-manager.c @@ -2424,7 +2424,7 @@ xfsm_manager_dbus_suspend (XfsmDbusManager *object, g_return_val_if_fail (XFSM_IS_MANAGER (object), FALSE); if (xfsm_shutdown_try_suspend (XFSM_MANAGER (object)->shutdown_helper, &error) == FALSE) {
-
throw_error (invocation, XFSM_ERROR_BAD_STATE, error->message);
-
throw_error (invocation, XFSM_ERROR_BAD_STATE, "%s", error->message); g_clear_error (&error); return TRUE;
Version: Unspecified