GNOME Bugzilla – Bug 674961
ShellNetworkAgent crash when connecting to VPN
Last modified: 2012-05-07 20:09:17 UTC
See https://bugzilla.redhat.com/show_bug.cgi?id=739315#c31 and later (note that everything before comment 31 is a different bug, but abrt is being stupid and insisting on merging the two bugs together). I can't reproduce, but I think the chain of events is: - VPNRequestHandler._readStdoutOldStyle() calls this._dataStdout.read_line_async() - User clicks OK, which results in this._agent.respond(), getting called, which causes ShellNetworkAgent to remove the request from the hash table - this._dataStdout.read_line_async() invokes its callback, which calls this._vpnChildProcessLineOldStyle(), which calls this._agent.set_password(), which crashes because the request isn't found I made a scratch build with this patch and pointed to it from the fedora bug, so we should have confirmation of whether or not this fixes it soon... The second patch tries to avoid future crashes of this sort...
Created attachment 212971 [details] [review] networkAgent: cancel any pending async I/O when destroying an operation _readStdoutOldStyle() or _readStdoutNewStyle() may have an async read pending when the dialog is destroyed. Cancel it and then discard the result.
Created attachment 212972 [details] [review] shell-network-agent: don't crash if a request isn't found If a request isn't found in shell_network_agent_set_password() or shell_network_agent_respond(), then g_return_if_fail() rather than crashing. OTOH, if a request is not found in shell_network_agent_cancel_get_secrets(), then just ignore it silently, since that could legitimately happen.
Uhm... Patches are ok, but I'm not sure I understand fully what happened in this bug. I'm assuming this is an old-style plugin (wasn't vpnc ported? bah), so .respond is only called when the process dies. This happens after we send QUIT\n\n to it, which in turn happens when have finished reading everything. The plugin should send nothing after we send QUIT\n\n, and dataStdout.read_line_utf8_finish() should return null, because reading got EOF.
Review of attachment 212972 [details] [review]: (This is ok for sure anyway)
Review of attachment 212971 [details] [review]: This one is ok too, although I'm not sure how it interacts with sending DONE\n\n - there's the risk the plugin just keeps waiting if we cancel from NM.
(In reply to comment #3) > Uhm... Patches are ok, but I'm not sure I understand fully what happened in > this bug. > I'm assuming this is an old-style plugin (wasn't vpnc ported? bah) vpnc and openconnect were both mentioned. vpnc is new, openconnect is old. (And there are actually a bunch of wifi password crashes mixed in with the abrt dups too, although they're all from f16.) > so .respond > is only called when the process dies. mmm... yeah, I was mixing up old and new paths, thinking respond() would be called from _onOk, but obviously that's not getting called in this case... Ah... nm-openconnect-auth-dialog doesn't wait for QUIT before exiting. So _vpnChildFinished() might get called before processing all the output. So that explains the openconnect crash. (And the g_return_if_fail() will prevent the crash, but it won't make things work, because we'll end up not sending some of the secrets with the response.) For the wifi crashes... lots of them mention there being multiple password prompts... so the problem there is that NM doesn't guarantee that there's only one outstanding request for a given set of secrets at any time, but our current hash table key is only unique-per-secret, not unique-per-request. So if you have two requests for the same secret, then the second one will crash, because its request will get removed from the hash table when the first one completes. That might be the problem with the vpnc crash too... I can't find any way for set_password() to get called after respond() in the newStylePlugin path.
Attachment 212972 [details] pushed as 8befcb9 - shell-network-agent: don't crash if a request isn't found
Created attachment 213001 [details] [review] NetworkAgent: disallow multiple requests for the same connection/setting It doesn't make sense to have multiple requests for the same connection/setting combination at the same time, since we would be asking the user twice for the same password. Instead, report cancellation to NetworkManager if this happens. Note that does make sense to have multiple requests in sequence though (they could have different flags), but this is not affected. What about this one? At least, it prevents getting two requests for the same request id.
*** Bug 673103 has been marked as a duplicate of this bug. ***
Comment on attachment 213001 [details] [review] NetworkAgent: disallow multiple requests for the same connection/setting >+ /* Keep pointers available even if request is freed */ >+ self = request->self; >+ request_id = request->request_id; If request is freed then request->self will be unreffed and request->request_id will be freed. >+ /* We already have a request pending for this (connection, setting) >+ Cancel it before starting the new one. >+ This will also free the request structure and associated resources. >+ */ /* comment should * be like * this */ > ShellAgentRequest *request = g_hash_table_lookup (priv->requests, request_id); >- GError *error; >+ g_free (request_id); another nitpick: blank line between the declarations and the g_free.
(In reply to comment #10) > (From update of attachment 213001 [details] [review]) > >+ /* Keep pointers available even if request is freed */ > >+ self = request->self; > >+ request_id = request->request_id; > > If request is freed then request->self will be unreffed and request->request_id > will be freed. request->self is hopefully kept alive by who ever calls the method request_id OTOH not actually freed with the request, rather it is freed by the hash table. But yeah, it's not actually needed. Updated patch soon...
Created attachment 213144 [details] [review] NetworkAgent: disallow multiple requests for the same connection/setting It doesn't make sense to have multiple requests for the same connection/setting combination at the same time, since we would be asking the user twice for the same password. Instead, report cancellation to NetworkManager if this happens. Note that does make sense to have multiple requests in sequence though (they could have different flags), but this is not affected.
Comment on attachment 213144 [details] [review] NetworkAgent: disallow multiple requests for the same connection/setting yeah, i think this should work
Attachment 213144 [details] pushed as e6087ef - NetworkAgent: disallow multiple requests for the same connection/setting