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

[Martyn Russell <martyn lanedo com>]

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