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 777737 - g_file_monitor_directory returns NULL and doesn't provide error
g_file_monitor_directory returns NULL and doesn't provide error
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: client module
git master
Other Linux
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2017-01-25 12:31 UTC by Ondrej Holy
Modified: 2017-11-10 13:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
network: Check variable before dereferencing (1.12 KB, patch)
2017-01-25 12:32 UTC, Ondrej Holy
committed Details | Review
client: Propagate error from create_mount_tracker_proxy (2.77 KB, patch)
2017-01-25 14:33 UTC, Ondrej Holy
committed Details | Review

Description Ondrej Holy 2017-01-25 12:31:45 UTC
It seems that g_file_monitor_directory may return NULL and doesn't provide any error in some cases. It should not happen and I don't really have any idea why it happens.

As a consequence, network backend sometimes crashes probably because of dereferencing NULL error variable, see:
https://retrace.fedoraproject.org/faf/problems/?component_names=gvfs&binary_names=%2Fusr%2Flibexec%2Fgvfsd-network
https://bugzilla.redhat.com/show_bug.cgi?id=1402350
https://bugzilla.redhat.com/show_bug.cgi?id=1391518
https://bugzilla.redhat.com/show_bug.cgi?id=1191298
Comment 1 Ondrej Holy 2017-01-25 12:32:34 UTC
Created attachment 344210 [details] [review]
network: Check variable before dereferencing

It seems that the error variable may be NULL at this point in
some cases as per the bug reports, but I don't really have any
idea why. Let's check the error variable before dereferencing
and see if it helps to reduce the number of bug reports...
Comment 2 Ondrej Holy 2017-01-25 12:35:11 UTC
Comment on attachment 344210 [details] [review]
network: Check variable before dereferencing

Attachment 344210 [details] pushed as d659151 - network: Check variable before dereferencing
Comment 3 Bastien Nocera 2017-01-25 12:55:23 UTC
I don't see why we'd want to do that instead of fixing the actual problem, I'd revert it.

The daemon is monitoring non-local files, so this would be implemented in gdaemonfile.c in gvfs.

And it looks like _g_daemon_vfs_get_mount_info_sync() will happily return NULL without setting an error:
  proxy = create_mount_tracker_proxy ();
  if (proxy == NULL)
    return NULL;
Comment 4 Ondrej Holy 2017-01-25 13:15:29 UTC
(In reply to Bastien Nocera from comment #3)
> I don't see why we'd want to do that instead of fixing the actual problem,
> I'd revert it.

It is not instead of fixing the problem, it is just workaround before we find the real cause. I haven't seen any obvious place where it could fail. We can revert it once we fix it...

> The daemon is monitoring non-local files, so this would be implemented in
> gdaemonfile.c in gvfs.

Yes, that's why I filed the bug against gvfs client :-)

> And it looks like _g_daemon_vfs_get_mount_info_sync() will happily return
> NULL without setting an error:
>   proxy = create_mount_tracker_proxy ();
>   if (proxy == NULL)
>     return NULL;

Good catch! I will prepare patch for it...
Comment 5 Ondrej Holy 2017-01-25 14:33:24 UTC
Created attachment 344231 [details] [review]
client: Propagate error from create_mount_tracker_proxy

Prevent GDaemonFile methods failures without error being set.

But still I am not sure that it fails on this place, because create_mount_tracker_proxy contains g_warning and I guess it should be in var_log_messages, but it isn't...
Comment 6 Bastien Nocera 2017-01-25 14:35:00 UTC
Is var_log_messages something other than /var/log/messages? Because that's not used on recent distros.
Comment 7 Bastien Nocera 2017-01-25 14:36:27 UTC
Review of attachment 344231 [details] [review]:

Looks good.
Comment 8 Ondrej Holy 2017-01-25 14:49:09 UTC
(In reply to Bastien Nocera from comment #6)
> Is var_log_messages something other than /var/log/messages? Because that's
> not used on recent distros.

I suppose so, because this file is also attached in downstream bug reports from abrt for recent Fedora versions, which doesn't use that file... but I will check with abrt people.
Comment 9 Ondrej Holy 2017-01-26 09:05:11 UTC
Comment on attachment 344231 [details] [review]
client: Propagate error from create_mount_tracker_proxy

Attachment 344231 [details] pushed as 6a62f4a - client: Propagate error from create_mount_tracker_proxy
Comment 10 Ondrej Holy 2017-01-26 09:05:42 UTC
(In reply to Ondrej Holy from comment #8)
> (In reply to Bastien Nocera from comment #6)
> > Is var_log_messages something other than /var/log/messages? Because that's
> > not used on recent distros.
> 
> I suppose so, because this file is also attached in downstream bug reports
> from abrt for recent Fedora versions, which doesn't use that file... but I
> will check with abrt people.

They confirmed that it should contain journalctl output, see:
https://github.com/abrt/abrt/blob/master/src/plugins/ccpp_event.conf#L32

I've made a test for sure and it really contains the warnings, so we are looking for something else here... :-/ any other tip? :-P
Comment 11 Ondrej Holy 2017-11-10 13:30:50 UTC
I've found the warning in "syslog" file finally, so attachment 344231 [details] [review] should be the proper fix.