[Xfce-bugs] [Bug 16796] New: NULL pointer dereference in xfsm_manager_dbus_suspend()

[bugzilla-daemon at xfce.org]

https://bugzilla.xfce.org/show_bug.cgi?id=16796

            Bug ID: 16796
           Summary: NULL pointer dereference in
                    xfsm_manager_dbus_suspend()
    Classification: Xfce Core
           Product: Xfce4-session
           Version: Unspecified
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: Medium
         Component: General
          Assignee: xfce-bugs at xfce.org
          Reporter: mail at maciej.szmigiero.name
  Target Milestone: Xfce 4.14

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:
1) A call to lock_screen() failed but didn't set the "error" variable,

2) 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;
     }

-- 
You are receiving this mail because:
You are the assignee for the bug.