Bug 65162 - act-user-manager fails when there is no active session on seat0.
Summary: act-user-manager fails when there is no active session on seat0.
Status: RESOLVED FIXED
Alias: None
Product: accountsservice
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Matthias Clasen
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-30 08:00 UTC by Marius Vollmer
Modified: 2013-06-30 18:22 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Don't assume that we are part of a session or have a seat. (3.08 KB, patch)
2013-05-30 08:00 UTC, Marius Vollmer
Details | Splinter Review

Description Marius Vollmer 2013-05-30 08:00:43 UTC
Created attachment 80017 [details] [review]
Don't assume that we are part of a session or have a seat.

When using logind:

During initialization, act-user-manager gets the active session of seat0, and then gets the seat of that session (which most likely is seat0).  If one of these steps fail, act-user-manager fails to track sessions.

While seat0 is guaranteed to exist, there doesn't have to be an active session on it.  Also, I think the intention is to figure out the seat of the session of the current process, in order to manage sessions on it.  That doesn't have to be seat0.

The attached patch tries to address this somewhat.
Comment 1 Ray Strode [halfline] 2013-06-05 18:15:40 UTC
(just a heads up, i plan to review this patch and your others ones soon.  I've been tied up in other things)
Comment 2 Marius Vollmer 2013-06-05 18:53:42 UTC
(In reply to comment #1)
> (just a heads up, i plan to review this patch and your others ones soon. 
> I've been tied up in other things)

(Cool!  The longer you wait, the more testing the patches get here, so no hurry! :-)
Comment 3 Matthias Clasen 2013-06-06 01:43:03 UTC
Comment on attachment 80017 [details] [review]
Don't assume that we are part of a session or have a seat.

Review of attachment 80017 [details] [review]:
-----------------------------------------------------------------

::: src/libaccountsservice/act-user-manager.c
@@ +703,3 @@
>                             strerror (-res));
>                  unload_seat (manager);
>                  return;

makes sense, but looking through the uses of manager->priv->seat.id, there are several places where it being NULL will cause problems.

@@ +1095,4 @@
>          char *session_id;
>          int   res;
>  
> +        res = sd_pid_get_session (0, &session_id);

This does not make sense to me - won't this always fail because pid 0 (init) is not part of a session ?  I do agree that the previous hardcoding of "seat0" was not right, but this looks worse.
Comment 4 Marius Vollmer 2013-06-06 06:13:17 UTC
(In reply to comment #3)
> makes sense, but looking through the uses of manager->priv->seat.id, there
> are several places where it being NULL will cause problems.

Yes... I'll find them.  Some seat related functionality will stop to work, of course, but that is OK I guess.

> @@ +1095,4 @@
> >          char *session_id;
> >          int   res;
> >  
> > +        res = sd_pid_get_session (0, &session_id);
> 
> This does not make sense to me - won't this always fail because pid 0 (init)
> is not part of a session ?  I do agree that the previous hardcoding of
> "seat0" was not right, but this looks worse.

Init is pid 1. :-) Pid 0 means "current process" with sd_pid_get_session.
Comment 5 Matthias Clasen 2013-06-06 12:43:14 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > makes sense, but looking through the uses of manager->priv->seat.id, there
> > are several places where it being NULL will cause problems.
> 
> Yes... I'll find them.  Some seat related functionality will stop to work,
> of course, but that is OK I guess.
> 
> > @@ +1095,4 @@
> > >          char *session_id;
> > >          int   res;
> > >  
> > > +        res = sd_pid_get_session (0, &session_id);
> > 
> > This does not make sense to me - won't this always fail because pid 0 (init)
> > is not part of a session ?  I do agree that the previous hardcoding of
> > "seat0" was not right, but this looks worse.
> 
> Init is pid 1. :-) Pid 0 means "current process" with sd_pid_get_session.

Err, right. Makes sense then.
Comment 6 Marius Vollmer 2013-06-11 12:19:52 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > makes sense, but looking through the uses of manager->priv->seat.id, there
> > are several places where it being NULL will cause problems.
> 
> Yes... I'll find them.  Some seat related functionality will stop to work,
> of course, but that is OK I guess.

Hmm, now I remember that I have considered this already.  In fact, the original code should be safe against seat.id == NULL already, since that was always a valid state for act-user-manager.

Please double check: seat.id is only used by _can_activate_systemd_sessions and act_user_manager_activate_user_session after act_user_manager_can_switch has checked it for NULL.

seat.id is also used by _systemd_session_is_on_our_seat, but a NULL is ok there.
Comment 7 Matthias Clasen 2013-06-11 13:36:19 UTC
Looks like you are right, indeed.
Comment 8 Matthias Clasen 2013-06-30 18:22:39 UTC
This was pushed as 8d2c6d3debf295c99dd228bf56922a89304ebe93


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.