Re: [Tracker] Checking out some miner code while trying to figure out a crash report..



On 12/11/09 13:34, Philip Van Hoof wrote:
Hey Martyn, Carlos,

Hi Philip.

I saw this email while I was in China and thought I would wait until I have coffee+processing power in my head to look at it :)

So here we go:

In libinotify_monitor_event_cb :

Line ~922:

        other_file = NULL;

        if (!str1) {
                str1 = g_file_get_path (file);
        }

        if (other_file) {
                str2 = g_file_get_path (other_file);
        } else {
                str2 = NULL;
        }

Isn't in this code the variable other_file always NULL?

Yes. This code has changed over the past year or so I think. In the past, I think we could look up cached events and get the other file - the reason this doesn't crash is because the other_file = NULL was always there IIRC and since, it looks like the code to retrieve it has been removed.

We should just set str2 = NULL with other_file = NULL here I guess.

next Line ~975:

                data = g_hash_table_lookup (monitor->private->event_pairs,
                                            GUINT_TO_POINTER (cookie));

                if (!data) {
                        data = event_data_new (file, event_type);

                        g_hash_table_insert (monitor->private->event_pairs,
                                             GUINT_TO_POINTER (cookie),
                                             data);
                } else {
                        other_file = data->file;
                }

So ok, other_file can be assigned to data->file. But ...

Yea.

Then at line ~1083:


                } else if (other_file) {
                        g_signal_emit (monitor,
                                       signals[ITEM_MOVED], 0,
                                       file,
                                       other_file,
                                       is_directory,
                                       TRUE);
                        
                        if (is_directory) {
                                tracker_monitor_update (monitor, file, other_file);
                        }

                        g_hash_table_remove (monitor->private->event_pairs,
                                             GUINT_TO_POINTER (cookie));
                }


So let's look at that tracker_monitor_update:

OK.

Line ~1415:


static void
tracker_monitor_update (TrackerMonitor *monitor,
                        GFile          *old_file,
                        GFile          *new_file)
{
        gpointer file_monitor;

        file_monitor = g_hash_table_lookup (monitor->private->monitors, old_file);

        if (!file_monitor) {
                gchar *path;

                path = g_file_get_path (old_file);
                g_warning ("No monitor was found for directory:'%s'", path);
                g_free (path);

                return;
        }

        /* Replace key in monitors hashtable */
        g_hash_table_steal (monitor->private->monitors, old_file);
        g_hash_table_insert (monitor->private->monitors, new_file, file_monitor);
}


So let's look at that monitor->private->monitors's GDestroyNotify-s

Yep.

Line ~250:

        /* Create monitors table for this module */
        priv->monitors =
                g_hash_table_new_full (g_file_hash,
                                       (GEqualFunc) g_file_equal,
                                       (GDestroyNotify) g_object_unref,
                                       (GDestroyNotify) libinotify_monitor_cancel);


This means that the key (new_file = other_file at the caller) is
unreferenced whenever g_hash_table_insert overwrites the slot in the
hashtable.

Hmm, I think Carlos updated this code and wanted to avoid the memory creation/free. But actually it looks broken for 3 reasons too, not just the one you mentioned.

    1. Inserting a GFile into a hash table without incrementing a
    reference causes a reference count problem and leads to using memory
    which is no longer valid.

    2. Using the same INotifyHandle* for a different directory name is
    broken and that's what we were doing when moving directories.

    3. We should actually recursively be removing all old monitors and
    re-adding monitors for the new sub-directory names.

Well spotted Philip.

Doesn't that mean that it should be like this instead?

g_hash_table_insert (monitor->private->monitors,
                      g_object_ref (new_file),
                      file_monitor);

Yes it should.

Except, I am going to just call remove/add APIs.

Otherwise will data->file become unreferenced (dangling pointer)
whenever either the hashtable is destroyed or the key is replaced with a
GFile that is g_file_equal.

I'm just wondering because this would explain a bug report by Tshepang.

It is quite likely. Some of his testing involves moving kernel source directories too, so I would not be surprised.

Thanks again for tracking this down, nicely done!

I have committed a fix for this now.

--
Regards,
Martyn



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]