From e41a31b9520dd1a4dbfe8162cf49fed7052dc69f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Rozs=C3=ADval?= Date: Tue, 5 Mar 2024 14:07:01 +0100 Subject: [PATCH] [Mono.Android] Fix race condition in AndroidMessageHandler (#8753) Fixes: https://github.com/xamarin/xamarin-android/issues/8740 There is a race condition in `AndroidMessageHandler` (see https://github.com/xamarin/xamarin-android/issues/8740#issuecomment-1960331579 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. --- .../AndroidMessageHandler.cs | 74 ++++++++++--------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs b/src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs index 26645e6fdcb..e5218687ae7 100644 --- a/src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs +++ b/src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs @@ -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; @@ -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 @@ -334,7 +358,7 @@ public bool RequestNeedsAuthorization { protected override void Dispose (bool disposing) { - disposed = true; + disposed = true; base.Dispose (disposing); } @@ -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) @@ -407,6 +439,7 @@ string EncodeUrl (Uri url) internal async Task DoSendAsync (HttpRequestMessage request, CancellationToken cancellationToken) { + started = true; AssertSelf (); if (request == null) throw new ArgumentNullException (nameof (request)); @@ -1050,15 +1083,6 @@ internal Task SetupRequestInternal (HttpRequestMessage request, HttpURLConnectio internal TrustManagerFactory? ConfigureTrustManagerFactoryInternal (KeyStore? keyStore) => ConfigureTrustManagerFactory (keyStore); - void AppendEncoding (string encoding, ref List ? list) - { - if (list == null) - list = new List (); - if (list.Contains (encoding)) - return; - list.Add (encoding); - } - async Task SetupRequestInternal (HttpRequestMessage request, URLConnection conn) { if (conn == null) @@ -1080,30 +1104,8 @@ void AppendEncoding (string encoding, ref List ? list) AddHeaders (httpConnection, request.Content.Headers); AddHeaders (httpConnection, request.Headers); - List ? 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);