Invalid reads in keyboard plugin

Bug #658777 reported by Chris Coulson on 2010-10-11
22
This bug affects 3 people
Affects Status Importance Assigned to Milestone
gnome-settings-daemon (Ubuntu)
Medium
Chris Coulson
Maverick
Medium
Chris Coulson

Bug Description

This is split from comment 16 on bug 630239:

==4294== Invalid read of size 1
==4294== at 0x4C29732: strcmp (mc_replace_strmem.c:426)
==4294== by 0x640AD28: g_str_equal (gstring.c:115)
==4294== by 0x63D7E6A: g_hash_table_insert_internal (ghash.c:401)
==4294== by 0x13BE70E5: popup_menu_set_group (gsd-keyboard-xkb.c:376)
==4294== by 0x13BE7C84: apply_xkb_settings (gsd-keyboard-xkb.c:543)
==4294== by 0x13BE8377: gsd_keyboard_xkb_init (gsd-keyboard-xkb.c:1123)
==4294== by 0x13BE651A: start_keyboard_idle_cb (gsd-keyboard-manager.c:399)
==4294== by 0x63E67E1: g_main_context_dispatch (gmain.c:2119)
==4294== by 0x63EA747: g_main_context_iterate (gmain.c:2750)
==4294== by 0x63EAC54: g_main_loop_run (gmain.c:2958)
==4294== by 0x4F7AA46: gtk_main (gtkmain.c:1237)
==4294== by 0x404299: main (main.c:502)
==4294== Address 0x1aa7d3a1 is 1 bytes inside a block of size 4 free'd
==4294== at 0x4C27D71: free (vg_replace_malloc.c:366)
==4294== by 0x13BE7104: popup_menu_set_group (gsd-keyboard-xkb.c:392)
==4294== by 0x13BE7C84: apply_xkb_settings (gsd-keyboard-xkb.c:543)
==4294== by 0x13BE8377: gsd_keyboard_xkb_init (gsd-keyboard-xkb.c:1123)
==4294== by 0x13BE651A: start_keyboard_idle_cb (gsd-keyboard-manager.c:399)
==4294== by 0x63E67E1: g_main_context_dispatch (gmain.c:2119)
==4294== by 0x63EA747: g_main_context_iterate (gmain.c:2750)
==4294== by 0x63EAC54: g_main_loop_run (gmain.c:2958)
==4294== by 0x4F7AA46: gtk_main (gtkmain.c:1237)
==4294== by 0x404299: main (main.c:502)

It is most likely the key for that particular node pointing to free'd
memory.

This is happening here in popup_menu_set_group:

                for (g = 0; g < g_strv_length (shortnames);g++) {
                        gpointer pcounter = NULL;
                        gchar *prev_layout_name = NULL;
                        int counter = 0;

                        if (g < g_strv_length (shortnames)) {
                                if (xkl_engine_get_features (engine) &
                                    XKLF_MULTIPLE_LAYOUTS_SUPPORTED) {
                                        gchar *longname = (gchar *) g_slist_nth_data (current_kbd_config.layouts_variants, g);
                                        gchar *variant_name;
                                        if (!gkbd_keyboard_config_split_items (longname, &lname, &variant_name))
                                                /* just in case */
                                                lname = longname;

                                        /* make it freeable */
                                        lname = g_strdup (lname);

                                        if (shortnames != NULL) {
                                                gchar *shortname = shortnames[g];
                                                if (shortname != NULL && *shortname != '\0') {
                                                        /* drop the long name */
                                                        g_free (lname);
                                                        lname = g_strdup (shortname);
                                                }
                                        }
                                } else {
                                        lname = g_strdup (longnames[g]);
                                }
                        }
                        if (lname == NULL)
                                lname = g_strdup ("");

                        /* Process layouts with repeating description */
                        if (g_hash_table_lookup_extended (ln2cnt_map, lname, (gpointer *) & prev_layout_name, &pcounter)) {
                                /* "next" same description */
                                counter = GPOINTER_TO_INT (pcounter);
                                guide = "XXX1";
                        }
1--> g_hash_table_insert (ln2cnt_map, lname, GINT_TO_POINTER (counter+1));

                        if (st->group == g) {
                                if (counter > 0) {
                                        gchar appendix[10] = "";
                                        gint utf8length;
                                        gunichar cidx;
                                        /* Unicode subscript 2, 3, 4 */
                                        cidx = 0x2081 + counter;
                                        utf8length = g_unichar_to_utf8 (cidx, appendix);
                                        appendix[utf8length] = '\0';
                                        layout_name = g_strconcat (lname, appendix, NULL);
                                } else {
                                        layout_name = g_strdup(lname);
                                }
                        }
2--> g_free(lname);
                }

So, at 2), the key "lname" is being free'd after it was previously
inserted in to the hash table (1), leaving a hash table full of nodes
with keys pointing to free'd memory regions.

This needs fixing, as it's likely to trigger crashes for some people when switching layouts

affects: ubuntu → gnome-settings-daemon (Ubuntu)
Changed in gnome-settings-daemon (Ubuntu):
assignee: nobody → Chris Coulson (chrisccoulson)
importance: Undecided → Medium
status: New → Triaged
Changed in gnome-settings-daemon (Ubuntu Maverick):
status: New → Triaged
importance: Undecided → Medium
assignee: nobody → Chris Coulson (chrisccoulson)
assignee: Chris Coulson (chrisccoulson) → nobody
milestone: none → maverick-updates
assignee: nobody → Chris Coulson (chrisccoulson)

Accepted gnome-settings-daemon 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 gnome-settings-daemon (Ubuntu Maverick):
status: Triaged → Fix Committed
tags: added: verification-needed
Sebastien Bacher (seb128) wrote :

confirming that the update fixes the errors, I get the invalid read with the maverick version and not after installing the upgrade, the new version works fine and I didn't notice any issue running it

Jean-Baptiste Lallement (jibel) wrote :

Thanks for testing, I'm marking as verification-done

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

This bug was fixed in the package gnome-settings-daemon - 2.32.0-0ubuntu3

---------------
gnome-settings-daemon (2.32.0-0ubuntu3) maverick-proposed; urgency=low

  * Fix LP: #625793 - Multiple Keyboard Layouts unusable: continuously
    changes layout + 100% CPU usage. Don't call xkl_engine_lock_group in
    response to XKB events, as XkbLockGroup generates another event
    - update debian/patches/06_use_application_indicator.patch
  * Fix LP: #658777 - In popup_menu_set_group() - after adding new entries to
    the hash table, don't free the keys else we end up with a hash table full
    of keys pointing to invalid memory. Instead, create the hash table with
    g_hash_table_new_full, and have the keys freed when the hash table is
    destroyed
    - update debian/patches/06_use_application_indicator.patch
 -- Chris Coulson <email address hidden> Tue, 12 Oct 2010 11:03:40 +0100

Changed in gnome-settings-daemon (Ubuntu Maverick):
status: Fix Committed → Fix Released
Martin Pitt (pitti) wrote :

Copied to natty.

Changed in gnome-settings-daemon (Ubuntu):
status: Triaged → Fix Released
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