Accessing the keyring in a thread make the program crash (was find_credentials segfaults)

Bug #656545 reported by Alejandro J. Cura on 2010-10-07
32
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Ubuntu Single Sign On Client
Status tracked in Trunk
Stable-1-0
High
Alejandro J. Cura
Trunk
High
Alejandro J. Cura
ubuntu-sso-client (Ubuntu)
High
Natalia Bidart
Maverick
High
Natalia Bidart
Natty
High
Natalia Bidart

Bug Description

Calling the find_credentials in the /com/ubuntu/sso/credentials DBus object makes this service crash very often.

This is caused by accessing the keyring in a thread using functions that are not thread safe.
This also affects the /credentials service since it access the keyring in a thread to store the credentials.

FIX: initialize glib threads on dbus.mainloop.glib

Minimal patch: see linked branch.

TEST CASE: this bug is very difficult to reproduce since involves some thread weirdness. Matt griffin was able to reproduce on a clean maverick install by logging in successfully into Ubuntu One using ussoc. After login, the process died with no much info.

Related branches

Alejandro J. Cura (alecu) wrote :

It looks like calling keyring_get_credentials from a thread is not safe.
I propose renaming the current one keyring_get_credentials_sync.
Also I propose not using a thread in the new find_credentials and using the async way of accessing the keyring instead.

tags: added: desktop+ sso-network u1-natty
Changed in ubuntu-sso-client:
status: New → Confirmed
importance: Undecided → High
Changed in ubuntu-sso-client:
assignee: nobody → Alejandro J. Cura (alecu)
Natalia Bidart (nataliabidart) wrote :

Performing tests with trunk code I've got:

nessita@dali:~/canonical/ubuntu-sso-client/trunk$ killall ubuntu-sso-login; DEBUG=True PYTHONPATH=. ./bin/ubuntu-sso-login
ubuntu-sso-login: no process found
Hooking up SIGHUP with handler <function sighup_handler at 0x23d47d0>.
Starting Ubuntu SSO login manager for bus 'com.ubuntu.sso'.
keyring_delete_credentials: app_name "a".
keyring_delete_credentials: app_name "a".
**
ERROR:gkr-operation.c:342:on_pending_call_notify: assertion failed: (pending == op->pending)
Segmentation fault
nessita@dali:~/canonical/ubuntu-sso-client/trunk$

Changed in ubuntu-sso-client:
status: Confirmed → In Progress
description: updated
summary: - the new find_credentials segfaults
+ Accessing the keyring in a thread make the program crash (was
+ find_credentials segfaults)
Changed in ubuntu-sso-client (Ubuntu):
status: New → In Progress
importance: Undecided → High
assignee: nobody → Alejandro J. Cura (alecu)
tags: added: u1-maverick-sru
dobey (dobey) on 2010-10-09
Changed in ubuntu-sso-client:
status: In Progress → Fix Committed
Changed in ubuntu-sso-client (Ubuntu):
assignee: Alejandro J. Cura (alecu) → Naty Bidart (nataliabidart)
Changed in ubuntu-sso-client (Ubuntu):
status: In Progress → Triaged
milestone: none → natty-alpha-1
description: updated
description: updated
description: updated
description: updated

Accepted ubuntu-sso-client into maverick-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Changed in ubuntu-sso-client (Ubuntu):
status: Triaged → Fix Committed
Changed in ubuntu-sso-client (Ubuntu Maverick):
status: New → Fix Committed
tags: added: verification-needed
Changed in ubuntu-sso-client (Ubuntu Maverick):
importance: Undecided → High
assignee: nobody → Naty Bidart (nataliabidart)
Natalia Bidart (nataliabidart) wrote :

Tested on a clean maverick install with -proposed enabled. Problem is very tricky to reproduce on maverick, but I can confirm I never got the seg fault.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package ubuntu-sso-client - 1.1.1-0ubuntu1

---------------
ubuntu-sso-client (1.1.1-0ubuntu1) natty; urgency=low

  * New upstream release (1.1.1):

    [ Alejandro J. Cura <email address hidden> ]
      * Call the dbus mainloop thread init (fixes LP: #656545).

    [ Natalia B. Bidart <email address hidden> ]
      * Added the new dbus iface to the executable. Deprecated a few Dbus
      constants.

    [ Natalia B. Bidart <email address hidden> ]
      * Adding a new DBus iface to manage credentials with flexible API (LP:
      #653113).

    [ Natalia B. Bidart <email address hidden> ]
      * Replace twisted gtk reactor with the standard gtk mainloop. (LP:
      #655327).

    [ Natalia B. Bidart <email address hidden> ]
      * The TC browser will not be shown until the TOS page has been loaded. A
      spinner is shown in the mean time (LP: #620456).
      * The TC browser does not allow link navigation other than the TOS page
      (LP: #616961).

    [ Natalia B. Bidart <email address hidden> ]
      * Terms and conditions label is now translatable. Added a button to be
      able to browse the TC (LP: 654534).

    [ Natalia B. Bidart <email address hidden> ]
      * Moved logic from DBus SSOCredentials module into a self-contained, DBus
      agnostic module called 'credentials' (LP: #653113).

    [ Natalia B. Bidart <email address hidden> ]
      * Isolating SSO login processor into a separated module (LP: #637204).

    [ Natalia B. Bidart <email address hidden> ]
      * Cleanup code and modules to prepare the code for the DBus API refactor.

    [ Natalia B. Bidart <email address hidden> ]
      * Made entries name to be unicode to avoid a faulty capitalization when
      using turkish locale (LP: #652939).

    [ Natalia B. Bidart <email address hidden> ]
      * Workaround for LP: #467397 (when using turkish locale, decimal module
      import fails).

    [ Natalia B. Bidart <email address hidden> ]
      * Added encoding declaration on every file. Fixed error passing from GUI
      in GTK signals (LP: #652318).

    [ Natalia B. Bidart <email address hidden> ]
      * Avoiding calling a callback twice for buttons (LP: #652248).

    [ Natalia B. Bidart <email address hidden> ]
      * Removed unneeded assert sentences and replaced with warning logging
      when necessary (LP: #649317).

  * New upstream release (1.1.0):

    [ Natalia B. Bidart <email address hidden> ]
      * Avoiding having the main window freezing when pinging the U1 server
      (LP: #645162).

    [ Natalia B. Bidart <email address hidden> ]
      * Do not create the login keyring, use default instead (LP: #639690).

    [ Natalia B. Bidart <email address hidden> ]
      * Avoid duplicating logging messages for module "main" (LP: #638981).

    [ Natalia B. Bidart <email address hidden> ]
      * Delay import of webkit to be able to build on non-graphical envs (LP:
      #628956).
 -- Natalia Bidart (nessita) <email address hidden> Mon, 11 Oct 2010 10:22:28 -0300

Changed in ubuntu-sso-client (Ubuntu Natty):
status: Fix Committed → Fix Released
tags: added: verification-done
removed: verification-needed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package ubuntu-sso-client - 1.0.4-0ubuntu1

---------------
ubuntu-sso-client (1.0.4-0ubuntu1) maverick-proposed; urgency=low

  * New upstream release:

  [ Alejandro J. Cura <email address hidden> ]
    * Replace twisted gtk reactor with the standard gtk mainloop. (LP: #655327).

  [ Alejandro J. Cura <email address hidden> ]
    * Call the dbus mainloop thread init (fixes LP: #656545).

  * Adding .bzr-builddeb/default.conf as per Michael Vog (mvo) request.

  * Adding dpkg (>= 1.15.7.2) as Pre-Depends (fixes LP: #658768).

  * Adding gnome-keyring as dep since python-gnomekeyring doesn't install it.
 -- Natalia Bidart (nessita) <email address hidden> Tue, 12 Oct 2010 10:07:55 -0300

Changed in ubuntu-sso-client (Ubuntu Maverick):
status: Fix Committed → Fix Released
Natalia Bidart (nataliabidart) wrote :

We have a recent case of segfault within ubuntu-sso-client for maverick release, so I'll be reopening this bug.

Changed in ubuntu-sso-client (Ubuntu Maverick):
status: Fix Released → Triaged
Changed in ubuntu-sso-client (Ubuntu Natty):
status: Fix Released → Triaged
Martin Pitt (pitti) wrote :

Accepted ubuntu-sso-client into maverick-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Changed in ubuntu-sso-client (Ubuntu Maverick):
status: Triaged → Fix Committed
tags: removed: verification-done
tags: added: verification-needed
Joshua Blount (jblount) wrote :

I was able to consistently reproduce this bug before, but with -proposed it's fixed now for me.

Martin Pitt (pitti) on 2010-11-12
tags: added: verification-done
removed: verification-needed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package ubuntu-sso-client - 1.0.7-0ubuntu1

---------------
ubuntu-sso-client (1.0.7-0ubuntu1) maverick-proposed; urgency=low

  * New upstream release (1.0.6, 1.0.7):

    [ Natalia B. Bidart <email address hidden> ]
      * Added a new DBus signal UserNotValidated to indicate when a user is
      registered but not validated (LP: #667899).
      * Added new workflow so email validation is requested if necessary.
      * The verify email page should be always built, not only on registration.

    [ Alejandro J. Cura <email address hidden> ]
      * Store credentials on the keyring *only* from the main thread (LP:
      #656545).

  * New upstream release (1.0.5):

    [ Natalia B. Bidart <email address hidden> ]

      * Credentials are removed if the pinging to the server fails or any
      other exception occurs (LP: #660516).
 -- Natalia Bidart (nessita) <email address hidden> Thu, 04 Nov 2010 09:21:00 -0300

Changed in ubuntu-sso-client (Ubuntu Maverick):
status: Fix Committed → Fix Released
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package ubuntu-sso-client - 1.1.5-0ubuntu1

---------------
ubuntu-sso-client (1.1.5-0ubuntu1) natty; urgency=low

  * New upstream release (1.1.5):

    [ Natalia B. Bidart <email address hidden> ]
      * Use "org.freedesktop.secrets" dbus service instead of
      "org.gnome.keyring" (LP: #683088).

  * New upstream release (1.1.4):

    [ Natalia B. Bidart <email address hidden> ]
      * Added a gtk.Notebook to ensure proper window resize at startup
        (LP: #682669).
      * Enabled window resizing to be more user friendly.
      * Remove outdated references to gnome keyring from docstrings.

  * New upstream release (1.1.3):

    [ Natalia B. Bidart <email address hidden> ]
      * Make UI more friendly to resizes and big fonts (LP: #627496).
      * Splitting GUI code out of backend (LP: #677518).
      * Credentials can now be stored using a DBus called (LP: #680253).
      * Status from SSO server is now case sensitive (LP: #653165).
      * Credentials should not be cleared if the ping wasn't made due to empty
        ping url (LP: #676679).

    [ Rodney Dawes <email address hidden> ]
     * Add the utils sub-package to the packages declaration so it installs
     (LP: #680593).

    [ Alejandro J. Cura <email address hidden> ]
      * Fully async keyring access thru DBus. Drops dependency with
      gnomekeyring (LP: #656545).
 -- Natalia Bidart (nessita) <email address hidden> Tue, 30 Nov 2010 10:22:11 -0300

Changed in ubuntu-sso-client (Ubuntu Natty):
status: Triaged → Fix Released
tags: added: testcase
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers