Inconsistencies and memory problems in TreeModel API

[felinira]

The current TreeIter implementation has memory safety issues so even just using a TreeStore is unsafe if you keep any TreeIters around anywhere.

The reason is that the model handles memory management and frees the TreeIter as soon as it is invalidated (for TreeStore this is luckily only when you delete a node, because it has GTK_TREE_MODEL_ITERS_PERSIST). If you delete a node from a TreeStore and still have an iter around you have a use-after-free situation. Safely using this API is basically impossible unless you copy the TreeIter.

This first part of the issue is solved by #495.

There is a second part of the problem though: As TreeStore for example actually stores real pointers inside the TreeIter, reusing it still leads to use-after-free. So actually the API must only return borrowed references and not implement clone or copy to make it impossible to store TreeIters anywhere if you are not the model. I think they are not really in use right now from rust as its main use is by gtk views from C code. So it's probably safe to change it.

There is also an issue with the TreeModel interface that could be fixed along the way: iter_next and iter_previous return bools. Actually they should return TreeIters (or &TreeIters as discussed above). The C interface works like this: You pass a TreeIter pointer and the model is supposed to change the pointer to the next/previous TreeIter. The return value indicates that this swap has happened / there is a next/previous iter.

[sdroege]

Repeating my comment from the PR:

For the general memory unsafety here, can you reproduce a invalid memory access or similar with just using the existing Rust bindings around this? That would be bad and would require further investigation there, but also there's limits to what we can do. See gtk-rs/gtk3-rs#333

And for

There is a second part of the problem though: As TreeStore for example actually stores real pointers inside the TreeIter, reusing it still leads to use-after-free. So actually the API must only return borrowed references and not implement clone or copy to make it impossible to store TreeIters anywhere if you are not the model. I think they are not really in use right now from rust as its main use is by gtk views from C code. So it's probably safe to change it.

that might mean that the Rust type for the TreeIter should actually have a lifetime parameter. Which unfortunately means that the whole bindings for anything using an iter would have to be manually implemented.

[miek]

Repeating my comment from the PR:

For the general memory unsafety here, can you reproduce a invalid memory access or similar with just using the existing Rust bindings around this? That would be bad and would require further investigation there, but also there's limits to what we can do. See gtk-rs/gtk3-rs#333

Here's an example adapted from the tree_view example:

use gtk::prelude::*;
use gtk::{
    ApplicationWindow, CellRendererText,
    TreeStore, TreeView, TreeViewColumn,
};

fn append_text_column(tree: &TreeView) {
    let column = TreeViewColumn::new();
    let cell = CellRendererText::new();

    column.pack_start(&cell, true);
    column.add_attribute(&cell, "text", 0);
    tree.append_column(&column);
}

fn build_ui(application: &gtk::Application) {
    let window = ApplicationWindow::new(application);

    window.set_title(Some("TreeView example"));

    // left pane
    let tree = TreeView::new();
    let store = TreeStore::new(&[String::static_type()]);

    tree.set_model(Some(&store));
    tree.set_headers_visible(false);
    append_text_column(&tree);

    let iter = store.insert_with_values(None, None, &[(0, &"Hello")]);

    //
    let iter2 = store.iter_from_string("0").unwrap();
    store.remove(&iter2);
    //

    for i in 0..10 {
        let iter = store.insert_with_values(None, None, &[(0, &format!("Hello {}", i))]);

        for _ in 0..i {
            store.insert_with_values(Some(&iter), None, &[(0, &"I'm a child node")]);
        }
    }

    //
    store.remove(&iter);
    //

    window.set_child(Some(&tree));
    window.show();
}

fn main() {
    let application = gtk::Application::new(
        Some("com.github.gtk-rs.examples.treeview"),
        Default::default(),
    );

    application.connect_activate(build_ui);

    application.run();
}

This adds a node then attempts to remove it twice, and segfaults fairly reliably for me. When calling remove Gtk does clear the fields in the GtkTreeIter to make it invalid, but keeping a copy gets around that.

I wonder whether GtkTreeStore should be incrementing stamp on any node removal, instead of only incrementing it on clear? That would allow it to detect an old iter.

---

The reason is that the model handles memory management and frees the TreeIter as soon as it is invalidated (for TreeStore this is luckily only when you delete a node, because it has GTK_TREE_MODEL_ITERS_PERSIST). If you delete a node from a TreeStore and still have an iter around you have a use-after-free situation. Safely using this API is basically impossible unless you copy the TreeIter.

I'm not sure that that is correct. As far as I can tell from looking through GtkTreeView and GtkTreeStore in C, the user is always expected to allocate the GtkTreeIter and only ever passes a pointer to the model. Here's an example: https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gtk/gtktreeview.c#L11566-L11583. The model does initialise & modify the fields within the GtkTreeIter, but never allocates or frees the struct itself.