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 782940 - Rework some of the SoupAuthNegotiate internals
Rework some of the SoupAuthNegotiate internals
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: Misc
2.58.x
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
: 782239 (view as bug list)
Depends on:
Blocks: 783780
 
 
Reported: 2017-05-22 07:34 UTC by Tomas Popela
Modified: 2017-06-22 13:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (6.77 KB, patch)
2017-05-22 07:34 UTC, Tomas Popela
none Details | Review
Address Dan's suggestions (8.58 KB, patch)
2017-05-29 08:23 UTC, Tomas Popela
none Details | Review
Proposed patch 3 (8.60 KB, patch)
2017-06-19 15:22 UTC, Tomas Popela
none Details | Review
Proposed patch 3 (8.97 KB, patch)
2017-06-20 11:15 UTC, Tomas Popela
reviewed Details | Review

Description Tomas Popela 2017-05-22 07:34:13 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.
Comment 1 Tomas Popela 2017-05-22 07:34:52 UTC
Created attachment 352340 [details] [review]
Proposed patch
Comment 2 Tomas Popela 2017-05-22 08:00:26 UTC
*** Bug 782239 has been marked as a duplicate of this bug. ***
Comment 3 Dan Winship 2017-05-26 12:49:54 UTC
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().
Comment 4 Tomas Popela 2017-05-29 08:21:55 UTC
(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.
Comment 5 Tomas Popela 2017-05-29 08:23:03 UTC
Created attachment 352754 [details] [review]
Address Dan's suggestions
Comment 6 Dan Winship 2017-06-19 13:45:06 UTC
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.
Comment 7 Tomas Popela 2017-06-19 14:51:55 UTC
(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.
Comment 8 Tomas Popela 2017-06-19 15:22:40 UTC
Created attachment 354047 [details] [review]
Proposed patch 3

Address Dan's review comments.
Comment 9 Dan Winship 2017-06-19 15:53:04 UTC
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.
Comment 10 Tomas Popela 2017-06-20 11:15:26 UTC
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 11 Dan Winship 2017-06-21 17:02:20 UTC
Comment on attachment 354091 [details] [review]
Proposed patch 3

ok. seems right I think
Comment 12 Tomas Popela 2017-06-22 13:55:08 UTC
Fixed with commit 05a88c3a in the master branch.