After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 607505 - double free crash in get_session_command_for_file()
double free crash in get_session_command_for_file()
Status: RESOLVED FIXED
Product: gdm
Classification: Core
Component: general
2.29.x
Other Linux
: Normal critical
: ---
Assigned To: GDM maintainers
GDM maintainers
Depends on:
Blocks:
 
 
Reported: 2010-01-19 23:12 UTC by Sebastien Bacher
Modified: 2010-01-20 21:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
don't free the exec variable twice (438 bytes, patch)
2010-01-19 23:13 UTC, Sebastien Bacher
rejected Details | Review

Description Sebastien Bacher 2010-01-19 23:12:21 UTC
The bug has been opene don https://bugs.launchpad.net/ubuntu/+source/gdm/+bug/505051

"This causes the login process to hang indefinitely. Cancel does nothing...

Here's what gets logged in /var/log/gdm/:0-slave.log:
*** glibc detected *** /usr/lib/gdm/gdm-simple-slave: double free or corruption (fasttop): 0x09a61c88 ***
======= Backtrace: =========
/lib/tls/i686/cmov/libc.so.6[0x440ff1]
/lib/tls/i686/cmov/libc.so.6[0x4426f2]
/lib/tls/i686/cmov/libc.so.6(cfree+0x6d)[0x44579d]
/lib/libglib-2.0.so.0(g_free+0x36)[0xbd8196]
/usr/lib/gdm/gdm-simple-slave[0x8056ac5] get_session_command_for_file
/usr/lib/gdm/gdm-simple-slave[0x8056d23] get_fallback_session_name
/usr/lib/gdm/gdm-simple-slave[0x805851d] gdm_session_direct_defaults_changed
/usr/lib/gdm/gdm-simple-slave[0x8058709] gdm_session_direct_setup_for_user
/usr/lib/libgobject-2.0.so.0(g_cclosure_marshal_VOID__STRING+0x88)[0xa251c8]
/usr/lib/libgobject-2.0.so.0(g_closure_invoke+0x1b2)[0xa18072]
/usr/lib/libgobject-2.0.so.0[0xa2d7a8]
/usr/lib/libgobject-2.0.so.0(g_signal_emit_valist+0x7bd)[0xa2eb2d]
/usr/lib/libgobject-2.0.so.0(g_signal_emit+0x26)[0xa2efb6]
/usr/lib/gdm/gdm-simple-slave[0x804f44a] greeter_server_message_handler
/lib/libdbus-1.so.3[0x3b9f13]
/lib/libdbus-1.so.3(dbus_connection_dispatch+0x3cc)[0x3accec]
/usr/lib/libdbus-glib-1.so.2[0x60075d]
/lib/libglib-2.0.so.0(g_main_context_dispatch+0x1f8)[0xbcfe88]
/lib/libglib-2.0.so.0[0xbd3730]
/lib/libglib-2.0.so.0(g_main_loop_run+0x1bf)[0xbd3b9f]
/usr/lib/gdm/gdm-simple-slave[0x804dbc6]
/lib/tls/i686/cmov/libc.so.6(__libc_start_main+0xe6)[0x3ecb56]
/usr/lib/gdm/gdm-simple-slave[0x804d7e1]

I have added the decoded symbol names from the ddeb. From this you can see that get_session_command_for_file likely causes the double free. Looking at that code I noticed this:

static gboolean
get_session_command_for_file (const char *file,
                              char **command)
{
        GKeyFile *key_file;
        GError *error;
        char *full_path;
        char *exec;
        gboolean ret;
        gboolean res;

        exec = NULL;
...
        exec = g_key_file_get_string (key_file,
                                      G_KEY_FILE_DESKTOP_GROUP,
                                      G_KEY_FILE_DESKTOP_KEY_TRY_EXEC,
                                      NULL);
        if (exec != NULL) {
                res = is_prog_in_path (exec);
                g_free (exec);

                if (! res) {
                        g_debug ("GdmSessionDirect: Command not found: %s",
                                 G_KEY_FILE_DESKTOP_KEY_TRY_EXEC);
                        goto out;
                }
        }
...
out:
        g_free (exec);

        return ret;
}

So exec gets g_free'd twice if there is a TryExec key and is_prog_in_path fails."
Comment 1 Sebastien Bacher 2010-01-19 23:13:26 UTC
Created attachment 151797 [details] [review]
don't free the exec variable twice
Comment 2 Ray Strode [halfline] 2010-01-20 19:01:55 UTC
Review of attachment 151797 [details] [review]:

::: gdm-2.29.5/daemon/gdm-session-direct.c
@@ +573,2 @@
         g_free (exec);
+out:

I don't think this is right, there's a number of other exit points where the free should happen.

In fact, looking at the code, it seems like the exec variable is getting abused for checking tryexec. That's probably okay, but I think we need to nullify it when we're done abusing it.
Comment 3 Ray Strode [halfline] 2010-01-20 19:04:32 UTC
I pushed 

6b28faeac82d3310c3c142187069d10b513cbf97 

Does that work okay for you?
Comment 4 Sebastien Bacher 2010-01-20 21:54:03 UTC
> I don't think this is right, there's a number of other exit points where the
free should happen.

The variable is used only twice in this function so the suggested change was doing the job, yours is another way and working fine too, thanks!