Race condition in app_indicator_init() causes application crash

Bug #1122596 reported by John Vert on 2013-02-12
16
This bug affects 3 people
Affects Status Importance Assigned to Milestone
libappindicator
Fix Released
Medium
Charles Kerr
libappindicator (Ubuntu)
Undecided
Unassigned
Precise
High
Brian Murray

Bug Description

app_indicator_init() in app-indicator.c calls g_bus_get() before setting self->priv. This creates a race condition where the bus_creation() callback can execute while self->priv still contains garbage. Suggested fix is to set self->priv before calling g_bus_get().

diff -ruN libappindicator-0.4.92/src/app-indicator.c libappindicator-0.4.92.new/src/app-indicator.c
--- libappindicator-0.4.92/src/app-indicator.c 2012-03-21 11:11:43.967367303 -0700
+++ libappindicator-0.4.92.new/src/app-indicator.c 2013-02-07 13:51:54.773720789 -0800
@@ -611,6 +611,8 @@
  priv->sec_activate_target = NULL;
  priv->sec_activate_enabled = FALSE;

+ self->priv = priv; // Needs to be set BEFORE calling g_bus_get so our handler can read it.
+
  /* Start getting the session bus */
  g_object_ref(self); /* ref for the bus creation callback */
  g_bus_get(G_BUS_TYPE_SESSION, NULL, bus_creation, self);
@@ -618,8 +620,6 @@
  g_signal_connect(G_OBJECT(gtk_icon_theme_get_default()),
   "changed", G_CALLBACK(theme_changed_cb), self);

- self->priv = priv;
-
  return;
 }

ProblemType: Bug
DistroRelease: Ubuntu 12.04
Package: libappindicator1 (not installed)
ProcVersionSignature: Ubuntu 3.2.0-32.51-generic 3.2.30
Uname: Linux 3.2.0-32-generic x86_64
NonfreeKernelModules: nvidia
ApportVersion: 2.0.1-0ubuntu17.1
Architecture: amd64
Date: Mon Feb 11 17:20:25 2013
InstallationMedia: Ubuntu 12.04.1 LTS "Precise Pangolin" - Release amd64 (20121016)
MarkForUpload: True
ProcEnviron:
 LC_CTYPE=en_US.UTF-8
 TERM=xterm
 PATH=(custom, no user)
 LANG=en_US.UTF-8
 SHELL=/bin/zsh
SourcePackage: libappindicator
UpgradeStatus: No upgrade log present (probably fresh install)

[Impact]
This bug was the #2 cause of crashes in the Steam client. I believe this meets the SRU criteria: "Bugs which do not fit under above categories, but (1) have an obviously safe patch and (2) affect an application rather than critical infrastructure packages (like X.org or the kernel)."

[Test Case]
As this is a race condition it is difficult to reproduce. However we have many crashdumps with a SIGSEGV referencing a garbage self->priv pointer in bus_creation(). After applying this fix, the crashes stopped.

[Regression Potential]
Low

Related branches

Charles Kerr (charlesk) wrote :

Looks right. Thanks John!

Changed in libappindicator:
status: New → In Progress
assignee: nobody → Charles Kerr (charlesk)
importance: Undecided → Medium
John Vert (johnv) on 2013-02-14
description: updated
Changed in libappindicator:
status: In Progress → Fix Committed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libappindicator - 12.10.1daily13.02.15-0ubuntu1

---------------
libappindicator (12.10.1daily13.02.15-0ubuntu1) raring; urgency=low

  [ Charles Kerr ]
  * Race condition in app_indicator_init() causes application crash (LP:
    #1122596)

  [ Iain Lane ]
  * [raring] Python appindicators broken by the latest libappindicator
    update (12.10.1daily13.02.13-0ubuntu1) with "ERROR:root:Could not
    find any typelib for AppIndicator3" (LP: #1124941)

  [ John Vert ]
  * Race condition in app_indicator_init() causes application crash (LP:
    #1122596)

  [ Automatic PS uploader ]
  * Automatic snapshot from revision 253
 -- Automatic PS uploader <email address hidden> Fri, 15 Feb 2013 07:46:54 +0000

Changed in libappindicator (Ubuntu):
status: New → Fix Released
Changed in libappindicator (Ubuntu Precise):
status: New → Triaged
importance: Undecided → High
Changed in libappindicator (Ubuntu Precise):
assignee: nobody → Brian Murray (brian-murray)
status: Triaged → In Progress

Hello John, or anyone else affected,

Accepted libappindicator into precise-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/libappindicator/0.4.92-0ubuntu1.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in libappindicator (Ubuntu Precise):
status: In Progress → Fix Committed
tags: added: verification-needed

The fix for this bug has been awaiting testing feedback in the -proposed repository for precise for more than 90 days. Please test this fix and update the bug appropriately with the results. In the event that the fix for this bug is still not verified 15 days from now, the package will be removed from the -proposed repository.

tags: added: removal-candidate
Brian Murray (brian-murray) wrote :

discussion in #ubuntu-devel regarding the crash to look for:

16:08 < charles> bdmurray: it would show up as
                 dereferencing a NULL pointer, the
                 self->priv field
16:08 < bdmurray> charles: would the crash be filed about
                  some other package though?
16:08 < charles> bdmurray, likely candidates for where
                 this would happen would be
16:09 < charles> app-indicator.c, bus_creation(), NULL
                 dereference on app->priv->connection
16:10 < charles> and much less likely, in
                 app-indicator.c, theme_changed_cb(), in
                 "if (priv->dbus_registration != 0)"
16:10 < charles> wrt showing up in a different package...
                 hmm
16:13 < charles> bdmurray, I guess it's possible. If so,
                 the stacktrace would show the levels
                 app_indicator_init() -> bus_creation()
                 -> crash
16:13 < charles> bdmurray: if you're trying to eliminate
                 candidate tickets -- if those aren't in
                 the stacktrace, it's not #1122596
16:14 < charles> bdmurray: is this helpful? I'm not sure
                 that I'm answering the right question :-)

Brian Murray (brian-murray) wrote :

I checked the apport duplicates database and didn't find any crashes with this information.

Bartosz Kosiorek (gang65) wrote :

After applying patch from precise-proposed the problem with crashes has gone. Thanks!

tags: added: verification-done
removed: verification-needed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libappindicator - 0.4.92-0ubuntu1.1

---------------
libappindicator (0.4.92-0ubuntu1.1) precise; urgency=low

  * Fix race condition in app_indicator_init() (LP: #1122596)
 -- John Vert <email address hidden> Tue, 12 Feb 2013 10:53:08 -0800

Changed in libappindicator (Ubuntu Precise):
status: Fix Committed → Fix Released

The verification of the Stable Release Update for libappindicator has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.

Charles Kerr (charlesk) on 2014-03-13
Changed in libappindicator:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers