Comment 7 for bug 1750013

Revision history for this message
Dan Streetman (ddstreet) wrote :

> when SSH ends, for example, a Release event is sent
> through dbus and systemd-logind captures it, in the function manager_message_handler().
>
> From there, the function session_remove_fifo() is called. That point is our "bootstrap"
> to add the closing session on gc

ok i see that path, dbus Release -> remove fifo, however i don't think it's appropriate to put the add_to_gc into remove_fifo; because, remove_fifo is also called from session_free()..consider this code:

        session_remove_fifo(s);

        free(s->state_file);
        free(s);
}

so...that call to session_remove_fifo() previously only removed the fifo. but with your patch, it's also adding 's' back onto the gc queue...but then immediately freeing s!!! When the gc queue is processed, this code:

        while ((session = m->session_gc_queue)) {
                LIST_REMOVE(Session, gc_queue, m->session_gc_queue, session);
                session->in_gc_queue = false;

will dereference 's' ('session' in the gc handler), accessing freed memory.

Since you're trying to use the dbus Release notification to add the session to the gc, wouldn't it be better to simply update the dbus Release handler to remove the fifo *and* add the session to the gc, from there? not modify the remove_fifo function?