Skip to content

Commit

Permalink
[Mono.Android] Fix race condition in AndroidMessageHandler (#8753)
Browse files Browse the repository at this point in the history
Fixes: #8740

There is a race condition in `AndroidMessageHandler` (see #8740 (comment) 
for more information). 

The solution is fairly simple: we should determine the decompression strategy only during the setup, when `AutomaticDecompression` is set. 
We don't need to do any more work during the request.

To prevent changing the decompression strategy while there are requests mid-flight, the setter will only accept values until the first request 
is sent. This is exactly what the `SocketsHttpHandler` does. I didn't want to touch any code unrelated to the race condition in this PR so if we 
want to prevent other properties from being mutated while HTTP requests are being sent, we should do it in a follow-up PR.
  • Loading branch information
simonrozsival authored Mar 5, 2024
1 parent aea89e1 commit e41a31b
Showing 1 changed file with 38 additions and 36 deletions.
74 changes: 38 additions & 36 deletions src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,14 @@ public void Reset ()
DecompressionMethods _decompressionMethods;

bool disposed;
bool started;

// Now all hail Java developers! Get this... HttpURLClient defaults to accepting AND
// uncompressing the gzip content encoding UNLESS you set the Accept-Encoding header to ANY
// value. So if we set it to 'gzip' below we WILL get gzipped stream but HttpURLClient will NOT
// uncompress it any longer, doh. And they don't support 'deflate' so we need to handle it ourselves.
bool decompress_here;
bool decompress_here => _acceptEncoding is not null && _acceptEncoding != IDENTITY_ENCODING;
string? _acceptEncoding;

public bool SupportsAutomaticDecompression => true;
public bool SupportsProxy => true;
Expand All @@ -152,7 +154,29 @@ public void Reset ()
public DecompressionMethods AutomaticDecompression
{
get => _decompressionMethods;
set => _decompressionMethods = value;
set
{
CheckDisposedOrStarted ();

_decompressionMethods = value;
_acceptEncoding = null;

if (value == DecompressionMethods.None) {
_acceptEncoding = IDENTITY_ENCODING;
} else {
if ((value & DecompressionMethods.GZip) != 0) {
_acceptEncoding = GZIP_ENCODING;
}

if ((value & DecompressionMethods.Deflate) != 0) {
_acceptEncoding = _acceptEncoding is null ? DEFLATE_ENCODING : $"{_acceptEncoding}, {DEFLATE_ENCODING}";
}

if ((value & DecompressionMethods.Brotli) != 0) {
_acceptEncoding = _acceptEncoding is null ? BROTLI_ENCODING : $"{_acceptEncoding}, {BROTLI_ENCODING}";
}
}
}
}

public CookieContainer CookieContainer
Expand Down Expand Up @@ -334,7 +358,7 @@ public bool RequestNeedsAuthorization {

protected override void Dispose (bool disposing)
{
disposed = true;
disposed = true;

base.Dispose (disposing);
}
Expand All @@ -346,6 +370,14 @@ protected void AssertSelf ()
throw new ObjectDisposedException (nameof (AndroidMessageHandler));
}

void CheckDisposedOrStarted ()
{
AssertSelf ();
if (started) {
throw new InvalidOperationException ("The handler has already started sending requests");
}
}

string EncodeUrl (Uri url)
{
if (url == null)
Expand Down Expand Up @@ -407,6 +439,7 @@ string EncodeUrl (Uri url)

internal async Task <HttpResponseMessage> DoSendAsync (HttpRequestMessage request, CancellationToken cancellationToken)
{
started = true;
AssertSelf ();
if (request == null)
throw new ArgumentNullException (nameof (request));
Expand Down Expand Up @@ -1050,15 +1083,6 @@ internal Task SetupRequestInternal (HttpRequestMessage request, HttpURLConnectio
internal TrustManagerFactory? ConfigureTrustManagerFactoryInternal (KeyStore? keyStore)
=> ConfigureTrustManagerFactory (keyStore);

void AppendEncoding (string encoding, ref List <string>? list)
{
if (list == null)
list = new List <string> ();
if (list.Contains (encoding))
return;
list.Add (encoding);
}

async Task <HttpURLConnection> SetupRequestInternal (HttpRequestMessage request, URLConnection conn)
{
if (conn == null)
Expand All @@ -1080,30 +1104,8 @@ void AppendEncoding (string encoding, ref List <string>? list)
AddHeaders (httpConnection, request.Content.Headers);
AddHeaders (httpConnection, request.Headers);

List <string>? accept_encoding = null;

decompress_here = false;
if (AutomaticDecompression == DecompressionMethods.None) {
AppendEncoding (IDENTITY_ENCODING, ref accept_encoding); // Turns off compression for the Java client
} else {
if ((AutomaticDecompression & DecompressionMethods.GZip) != 0) {
AppendEncoding (GZIP_ENCODING, ref accept_encoding);
decompress_here = true;
}

if ((AutomaticDecompression & DecompressionMethods.Deflate) != 0) {
AppendEncoding (DEFLATE_ENCODING, ref accept_encoding);
decompress_here = true;
}

if ((AutomaticDecompression & DecompressionMethods.Brotli) != 0) {
AppendEncoding (BROTLI_ENCODING, ref accept_encoding);
decompress_here = true;
}
}

if (accept_encoding?.Count > 0)
httpConnection.SetRequestProperty ("Accept-Encoding", String.Join (",", accept_encoding));
if (_acceptEncoding is not null)
httpConnection.SetRequestProperty ("Accept-Encoding", _acceptEncoding);

if (UseCookies && CookieContainer != null && request.RequestUri is not null) {
string cookieHeaderValue = CookieContainer.GetCookieHeader (request.RequestUri);
Expand Down

0 comments on commit e41a31b

Please sign in to comment.