GNOME Bugzilla – Bug 782940
Rework some of the SoupAuthNegotiate internals
Last modified: 2017-06-22 13:55:08 UTC
There are several problems with the current state. The main problem is that the SoupMessage can outlive the SoupAuthNegotiate object and if the 'got_headers' or 'finished' SoupMessage signals handlers are active then they could be called with invalid SoupAuthNegotiate object. To avoid that use the g_signal_connect_data() and increase the reference on the SoupAuthNegotiate object. Also the whole concept of how we are working with the SoupAuthNegotiateConnectionState is wrong. When the connection state is created it's saved to the private structure and then accessed from there. The problem is that another state for different message could be created in the mean time and that one would overwrite the currently set (or if one would be freed it would erase the currently set one). Instead introduce a hash table with active messages and their states.
Created attachment 352340 [details] [review] Proposed patch
*** Bug 782239 has been marked as a duplicate of this bug. ***
Connection states aren't per-message, they're per connection. (Hence the name.) Trying to treat that as per-message too is going to run into problems because messages can move between connections when they're re-sent. (And in some cases a message might get sent on a connection, get back a 401, and then the response to the auth request gets sent on a *different* message. Or at least, that can happen in the NTLM code. Maybe that doesn't work in the Negotiate code because of bugs.) SoupAuthNegotiatePrivate->conn_state is clearly broken though. That can only work if there's only a single negotiate auth pending at a time. The fix might be to expose SoupConnectionAuth's get_connection_state_for_message() so that you can call it from check_server_response().
(In reply to Dan Winship from comment #3) > Connection states aren't per-message, they're per connection. (Hence the > name.) Trying to treat that as per-message too is going to run into problems > because messages can move between connections when they're re-sent. (And in > some cases a message might get sent on a connection, get back a 401, and > then the response to the auth request gets sent on a *different* message. Or > at least, that can happen in the NTLM code. Maybe that doesn't work in the > Negotiate code because of bugs.) > > SoupAuthNegotiatePrivate->conn_state is clearly broken though. That can only > work if there's only a single negotiate auth pending at a time. > > The fix might be to expose SoupConnectionAuth's > get_connection_state_for_message() so that you can call it from > check_server_response(). Thank you Dan for the explanation and for the suggestion! I will attach a reworked patch.
Created attachment 352754 [details] [review] Address Dan's suggestions
Comment on attachment 352754 [details] [review] Address Dan's suggestions >- /* Register the callbacks just once */ That suggests there's some problem with possibly connecting the signal handler multiple times, which the new code doesn't seem to deal with. Possibly, if the GSS mechanism involves multiple rounds, then this would get called a second time after a signal handler had already been connected in the previous round? >+/** >+ * soup_connection_auth_get_connection_state_for_message: >+ * @auth: a #SoupConnectionAuth >+ * @msg: a #SoupMessage >+ * >+ * Returns an associated connection state object for the given @auth and @msg. Should mention that this is only useful from within implementations of SoupConnectionAuth subclasses.
(In reply to Dan Winship from comment #6) > >- /* Register the callbacks just once */ > > That suggests there's some problem with possibly connecting the signal > handler multiple times, which the new code doesn't seem to deal with. > > Possibly, if the GSS mechanism involves multiple rounds, then this would get > called a second time after a signal handler had already been connected in > the previous round? You are right, that this could possibly happen and needs to be addressed. I will rework the patch. I remember that when I originally wrote the implementation one Red Hat internal site triggered this behavior, but something changed there as it's not doing it anymore.
Created attachment 354047 [details] [review] Proposed patch 3 Address Dan's review comments.
Comment on attachment 354047 [details] [review] Proposed patch 3 > priv->message_finished_signal_id = id; sorry, I should have been clearer before. This still has the same problem as originally, where you're trying to store per-message state on the SoupConnectionAuth when there isn't necessarily a 1-to-1 mapping between auths and messages. You need to keep track *on the SoupMessage* of whether you already connected to its signal. Eg, with g_object_set_data() or something.
Created attachment 354091 [details] [review] Proposed patch 3 Implement Dan's suggestion to avoid possibly connecting to the SoupMessage's 'got_headers' signal multiple times.
Comment on attachment 354091 [details] [review] Proposed patch 3 ok. seems right I think
Fixed with commit 05a88c3a in the master branch.