Bug 1050934

Created attachment 847579 [details]
File: backtrace

Created attachment 847580 [details]
File: cgroup

Created attachment 847581 [details]
File: core_backtrace

Created attachment 847582 [details]
File: dso_list

Created attachment 847583 [details]
File: environ

Created attachment 847584 [details]
File: limits

Created attachment 847585 [details]
File: maps

Created attachment 847586 [details]
File: open_fds

Created attachment 847587 [details]
File: proc_pid_status

Created attachment 847588 [details]
File: var_log_messages

** (nm-pptp-service:18984): WARNING **: Failed to initialize VPN plugin: Connection ":1.127" is not allowed to own the service "org.freedesktop.NetworkManager.pptp" due to security policies in the configuration file

1月 09 18:02:25 fab.cicku.me dbus-daemon[539]: dbus[539]: [system] Rejected send message, 3 matched rules; type="error", sender=":1.107" (uid=0 pid=16419 comm="/usr/libexec/nm-pptp-service --debug ") interface="(unset)" member="(unset)" error name="org.freedesktop.DBus.Error.UnknownMethod" requested_reply="0" destination=":1.108" (uid=0 pid=16422 comm="/sbin/pppd pty /sbin/pptp 10.6.205.202 --nolaunchp")

There were problems with D-Bus communication. They caused NMVPNPlugin constructor() to fail, and a crash/abort resulted due to destroying self-object in the constructor.

Fix the crash by reworking NMVPNPlugin initialization:
branch jk/rh1050934-vpn-plugin-init

Looks good!!

Also, we need to change the creation of the plugin. I have pushed the code to the plugins' repositories. Branches:
jk/vpnc-plugin-init
jk/openvpn-plugin-init
jk/pptp-plugin-init
jk/openconnect-plugin-init

You can test with these scratch builds (rawhide):
NetworkManager:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=6640684
NetworkManager-vpnc:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=6641334

And jk/openswan-plugin-init too.

Thinking about this again, I'd say that the patch in libnm-glib must be changed so that the constructor still operates as before, but in error case, it must not unref @self, instead it must set an internal GError instance.

initable_init() in error case just returns FALSE and the internal GError instance.

In other words, initable_init() should not do anything required for the object to function properly, so that old plugins can get away with creating the object with g_object_new().


This way, old plugins using g_object_new() will still work. If an error happens, they are doomed -- but that would not have been any different before this patch.
New plugins instead use g_initable_new() so that the can fail gracefully.

The patches to all the plugins look good to me.

Only thing, I would change

- g_warning ("%s", error->message);
+ g_warning ("Error initiating VPN plugin: %s", error->message);

Note that the abort-on-finalize-during-construction behavior was reverted later, and the current version of glib in rawhide (glib2-2.39.92-1.fc21) just prints a warning in this case (in addition to the warning that it already printed for returning NULL from construct.)

And as Thomas says, requiring the use of the GInitable API is an API break, so we shouldn't do that.

Yeah, we can't quite break the API yet.  There are external plugins (iodine, ssh, etc) that we should try to keep working.

Thank you everyone above, I just read about this...

Created attachment 1129820 [details]
[patch] jk/rh1050934-vpn-plugin-init

I attach here Jirka's patch jk/rh1050934-vpn-plugin-init for historical reference. It applies on commit a1e89b4d29b97695f21e0070888f7910f7fe7a87.


I think the patch is wrong, because it requires users to call g_initable_init(), which is an API break.