From: Stephen Toub Date: Tue, 27 Jul 2021 11:40:36 +0000 (-0400) Subject: Tighten up some ArrayPool.Return usage (#56229) X-Git-Tag: accepted/tizen/unified/20220110.054933~894 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=59a5610e8b9dde056c21acf4875d77a3ca20f98d;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Tighten up some ArrayPool.Return usage (#56229) Avoid potential problems if Return were to throw an exception after having already put the array back into the pool. --- diff --git a/src/libraries/Common/src/System/Text/Json/PooledByteBufferWriter.cs b/src/libraries/Common/src/System/Text/Json/PooledByteBufferWriter.cs index 80e7bc2..8694a78 100644 --- a/src/libraries/Common/src/System/Text/Json/PooledByteBufferWriter.cs +++ b/src/libraries/Common/src/System/Text/Json/PooledByteBufferWriter.cs @@ -86,8 +86,9 @@ namespace System.Text.Json } ClearHelper(); - ArrayPool.Shared.Return(_rentedBuffer); + byte[] toReturn = _rentedBuffer; _rentedBuffer = null!; + ArrayPool.Shared.Return(toReturn); } public void Advance(int count) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs index 4acd637..73e65b3 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs @@ -188,14 +188,17 @@ namespace System.Drawing filePath.CopyTo(0, buffer, 0, filePath.Length); buffer[filePath.Length] = '\0'; + IntPtr hIcon; fixed (char* b = buffer) { - IntPtr hIcon = Interop.Shell32.ExtractAssociatedIcon(NativeMethods.NullHandleRef, b, ref index); - ArrayPool.Shared.Return(buffer); - if (hIcon != IntPtr.Zero) - { - return new Icon(hIcon, true); - } + hIcon = Interop.Shell32.ExtractAssociatedIcon(NativeMethods.NullHandleRef, b, ref index); + } + + ArrayPool.Shared.Return(buffer); + + if (hIcon != IntPtr.Zero) + { + return new Icon(hIcon, true); } return null; diff --git a/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/TranscodingReadStream.cs b/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/TranscodingReadStream.cs index 894ea8c..cc8ef87 100644 --- a/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/TranscodingReadStream.cs +++ b/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/TranscodingReadStream.cs @@ -208,17 +208,20 @@ namespace System.Net.Http.Json { _disposed = true; - Debug.Assert(_charBuffer.Array != null); - ArrayPool.Shared.Return(_charBuffer.Array); + char[]? charBuffer = _charBuffer.Array; + Debug.Assert(charBuffer != null); _charBuffer = default; + ArrayPool.Shared.Return(charBuffer); - Debug.Assert(_byteBuffer.Array != null); - ArrayPool.Shared.Return(_byteBuffer.Array); + byte[]? byteBuffer = _byteBuffer.Array; + Debug.Assert(byteBuffer != null); _byteBuffer = default; + ArrayPool.Shared.Return(byteBuffer); - Debug.Assert(_overflowBuffer.Array != null); - ArrayPool.Shared.Return(_overflowBuffer.Array); + byte[]? overflowBuffer = _overflowBuffer.Array; + Debug.Assert(overflowBuffer != null); _overflowBuffer = default; + ArrayPool.Shared.Return(overflowBuffer); _stream.Dispose(); } diff --git a/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/TranscodingWriteStream.cs b/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/TranscodingWriteStream.cs index 51c73d0..2217bae 100644 --- a/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/TranscodingWriteStream.cs +++ b/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/TranscodingWriteStream.cs @@ -133,8 +133,9 @@ namespace System.Net.Http.Json if (!_disposed) { _disposed = true; - ArrayPool.Shared.Return(_charBuffer); + char[] toReturn = _charBuffer; _charBuffer = null!; + ArrayPool.Shared.Return(toReturn); } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs index 6b1b1e0..0cd8884 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs @@ -742,8 +742,10 @@ namespace System.Net.Security if (status.ErrorCode == SecurityStatusPalErrorCode.TryAgain) { // No need to hold on the buffer any more. - ArrayPool.Shared.Return(bufferToReturn); + byte[] tmp = bufferToReturn; bufferToReturn = null; + ArrayPool.Shared.Return(tmp); + // Call WriteSingleChunk() recursively to avoid code duplication. // This should be extremely rare in cases when second renegotiation happens concurrently with Write. await WriteSingleChunk(writeAdapter, buffer).ConfigureAwait(false); @@ -788,14 +790,12 @@ namespace System.Net.Security // actually contains no decrypted or encrypted bytes private void ReturnReadBufferIfEmpty() { - if (_internalBuffer != null && _decryptedBytesCount == 0 && _internalBufferCount == 0) + if (_internalBuffer is byte[] internalBuffer && _decryptedBytesCount == 0 && _internalBufferCount == 0) { - ArrayPool.Shared.Return(_internalBuffer); _internalBuffer = null; - _internalBufferCount = 0; _internalOffset = 0; - _decryptedBytesCount = 0; _decryptedBytesOffset = 0; + ArrayPool.Shared.Return(internalBuffer); } else if (_decryptedBytesCount == 0) { diff --git a/src/libraries/System.Net.Security/tests/StressTests/SslStress/StressOperations.cs b/src/libraries/System.Net.Security/tests/StressTests/SslStress/StressOperations.cs index 8c2ec6f..eea046d 100644 --- a/src/libraries/System.Net.Security/tests/StressTests/SslStress/StressOperations.cs +++ b/src/libraries/System.Net.Security/tests/StressTests/SslStress/StressOperations.cs @@ -24,7 +24,7 @@ namespace SslStress { public struct DataSegment { - private readonly byte[] _buffer; + private byte[] _buffer; public DataSegment(int length) { @@ -37,7 +37,12 @@ namespace SslStress public Span AsSpan() => new Span(_buffer, 0, Length); public ulong Checksum => CRC.CalculateCRC(AsSpan()); - public void Return() => ArrayPool.Shared.Return(_buffer); + public void Return() + { + byte[] toReturn = _buffer; + _buffer = null; + ArrayPool.Shared.Return(toReturn); + } /// Create and populate a segment with random data public static DataSegment CreateRandom(Random random, int maxLength) diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/Compression/WebSocketDeflater.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/Compression/WebSocketDeflater.cs index f3ecf27..7d8eb83 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/Compression/WebSocketDeflater.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/Compression/WebSocketDeflater.cs @@ -35,10 +35,10 @@ namespace System.Net.WebSockets.Compression public void ReleaseBuffer() { - if (_buffer is not null) + if (_buffer is byte[] toReturn) { - ArrayPool.Shared.Return(_buffer); _buffer = null; + ArrayPool.Shared.Return(toReturn); } } @@ -70,8 +70,11 @@ namespace System.Net.WebSockets.Compression // Rent a 30% bigger buffer byte[] newBuffer = ArrayPool.Shared.Rent((int)(_buffer.Length * 1.3)); _buffer.AsSpan(0, position).CopyTo(newBuffer); - ArrayPool.Shared.Return(_buffer); + + byte[] toReturn = _buffer; _buffer = newBuffer; + + ArrayPool.Shared.Return(toReturn); } return new ReadOnlySpan(_buffer, 0, position); diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/Compression/WebSocketInflater.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/Compression/WebSocketInflater.cs index d47e646..5f580e9 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/Compression/WebSocketInflater.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/Compression/WebSocketInflater.cs @@ -111,9 +111,11 @@ namespace System.Net.WebSockets.Compression { byte[] newBuffer = ArrayPool.Shared.Rent(_available + FlushMarkerLength); _buffer.AsSpan(0, _available).CopyTo(newBuffer); - ArrayPool.Shared.Return(_buffer); + byte[] toReturn = _buffer; _buffer = newBuffer; + + ArrayPool.Shared.Return(toReturn); } FlushMarker.CopyTo(_buffer.AsSpan(_available)); @@ -202,12 +204,13 @@ namespace System.Net.WebSockets.Compression private void ReleaseBuffer() { - if (_buffer is not null) + if (_buffer is byte[] toReturn) { - ArrayPool.Shared.Return(_buffer); _buffer = null; _available = 0; _position = 0; + + ArrayPool.Shared.Return(toReturn); } } diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs index 864b742..297b4c5 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs @@ -1452,11 +1452,10 @@ namespace System.Net.WebSockets { Debug.Assert(_sendFrameAsyncLock.CurrentCount == 0, "Caller should hold the _sendFrameAsyncLock"); - byte[]? old = _sendBuffer; - if (old != null) + if (_sendBuffer is byte[] toReturn) { _sendBuffer = null; - ArrayPool.Shared.Return(old); + ArrayPool.Shared.Return(toReturn); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs index ec23d53..a82372b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs @@ -59,10 +59,11 @@ namespace System.Collections.Generic [MethodImpl(MethodImplOptions.AggressiveInlining)] public void Dispose() { - if (_arrayFromPool != null) + T[]? toReturn = _arrayFromPool; + if (toReturn != null) { - ArrayPool.Shared.Return(_arrayFromPool); _arrayFromPool = null; + ArrayPool.Shared.Return(toReturn); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs index 04f8282..a2c83ab 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs @@ -236,12 +236,17 @@ namespace System.IO.Enumeration CloseDirectoryHandle(); - if (_pathBuffer != null) - ArrayPool.Shared.Return(_pathBuffer); - _pathBuffer = null; - if (_entryBuffer != null) - ArrayPool.Shared.Return(_entryBuffer); - _entryBuffer = null; + if (_pathBuffer is char[] pathBuffer) + { + _pathBuffer = null; + ArrayPool.Shared.Return(pathBuffer); + } + + if (_entryBuffer is byte[] entryBuffer) + { + _entryBuffer = null; + ArrayPool.Shared.Return(entryBuffer); + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/File.cs b/src/libraries/System.Private.CoreLib/src/System/IO/File.cs index 668e78c..ff2a7bd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/File.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/File.cs @@ -796,8 +796,11 @@ namespace System.IO byte[] tmp = ArrayPool.Shared.Rent((int)newLength); Buffer.BlockCopy(rentedArray, 0, tmp, 0, bytesRead); - ArrayPool.Shared.Return(rentedArray); + + byte[] toReturn = rentedArray; rentedArray = tmp; + + ArrayPool.Shared.Return(toReturn); } Debug.Assert(bytesRead < rentedArray.Length); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs index 698e3bc..191106a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs @@ -568,8 +568,9 @@ namespace System.IO // the return value is the required buffer size, in TCHARs. This value includes the size of the terminating null character. if (result > buffer.Length) { - ArrayPool.Shared.Return(buffer); + char[] toReturn = buffer; buffer = ArrayPool.Shared.Rent((int)result); + ArrayPool.Shared.Return(toReturn); result = GetFinalPathNameByHandle(handle, buffer); } diff --git a/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.NonAndroid.cs b/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.NonAndroid.cs index c5ad4b0..d7e54e1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.NonAndroid.cs +++ b/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.NonAndroid.cs @@ -181,10 +181,9 @@ namespace System List? toExplore = null; // List used as a stack int bufferSize = Interop.Sys.GetReadDirRBufferSize(); - byte[]? dirBuffer = null; + byte[] dirBuffer = ArrayPool.Shared.Rent(bufferSize); try { - dirBuffer = ArrayPool.Shared.Rent(bufferSize); string currentPath = path; fixed (byte* dirBufferPtr = dirBuffer) @@ -266,8 +265,7 @@ namespace System } finally { - if (dirBuffer != null) - ArrayPool.Shared.Return(dirBuffer); + ArrayPool.Shared.Return(dirBuffer); } } diff --git a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs index 9a67b46..c4c7c05 100644 --- a/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs +++ b/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs @@ -637,7 +637,7 @@ namespace System.Security.Cryptography else { // Use ArrayPool.Shared instead of CryptoPool because the array is passed out. - byte[] rentedBuffer = ArrayPool.Shared.Rent(inputBuffer.Length); + byte[]? rentedBuffer = ArrayPool.Shared.Rent(inputBuffer.Length); int result = default; // Pin the rented buffer for security. @@ -655,7 +655,7 @@ namespace System.Security.Cryptography } ArrayPool.Shared.Return(rentedBuffer); - rentedBuffer = null!; + rentedBuffer = null; return result; } } @@ -667,7 +667,7 @@ namespace System.Security.Cryptography CheckCopyToArguments(destination, bufferSize); // Use ArrayPool.Shared instead of CryptoPool because the array is passed out. - byte[] rentedBuffer = ArrayPool.Shared.Rent(bufferSize); + byte[]? rentedBuffer = ArrayPool.Shared.Rent(bufferSize); // Pin the array for security. fixed (byte* _ = &rentedBuffer[0]) { @@ -686,7 +686,7 @@ namespace System.Security.Cryptography } } ArrayPool.Shared.Return(rentedBuffer); - rentedBuffer = null!; + rentedBuffer = null; } /// @@ -699,7 +699,7 @@ namespace System.Security.Cryptography private async Task CopyToAsyncInternal(Stream destination, int bufferSize, CancellationToken cancellationToken) { // Use ArrayPool.Shared instead of CryptoPool because the array is passed out. - byte[] rentedBuffer = ArrayPool.Shared.Rent(bufferSize); + byte[]? rentedBuffer = ArrayPool.Shared.Rent(bufferSize); // Pin the array for security. GCHandle pinHandle = GCHandle.Alloc(rentedBuffer, GCHandleType.Pinned); try @@ -717,7 +717,7 @@ namespace System.Security.Cryptography pinHandle.Free(); } ArrayPool.Shared.Return(rentedBuffer); - rentedBuffer = null!; + rentedBuffer = null; } private void CheckCopyToArguments(Stream destination, int bufferSize) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/UnixPkcs12Reader.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/UnixPkcs12Reader.cs index b227a05..8cd7abe 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/UnixPkcs12Reader.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/UnixPkcs12Reader.cs @@ -167,6 +167,7 @@ namespace Internal.Cryptography.Pal if (contentType == DecryptedSentinel) { ReadOnlyMemory content = rentedContents[i].Content; + rentedContents[i].Content = default; if (!MemoryMarshal.TryGetArray(content, out ArraySegment segment)) { @@ -174,7 +175,6 @@ namespace Internal.Cryptography.Pal } CryptoPool.Return(segment); - rentedContents[0].Content = default; } } diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.iOS/AppleCertificatePal.Pem.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.iOS/AppleCertificatePal.Pem.cs index 4fae469..73d0e7b 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.iOS/AppleCertificatePal.Pem.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.iOS/AppleCertificatePal.Pem.cs @@ -63,8 +63,9 @@ namespace Internal.Cryptography.Pal X509ContentType.Pkcs7; bool cont = derCallback(certBytes.AsSpan(0, bytesWritten), contentType); - CryptoPool.Return(certBytes, clearSize: 0); + byte[] toReturn = certBytes; certBytes = null; + CryptoPool.Return(toReturn, clearSize: 0); if (!cont) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs index 0514750..02c7149 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs @@ -32,6 +32,7 @@ namespace System.Text.Json.Serialization.Converters protected override object CreateObject(ref ReadStackFrame frame) { object[] arguments = (object[])frame.CtorArgumentState!.Arguments; + frame.CtorArgumentState.Arguments = null!; var createObject = (JsonTypeInfo.ParameterizedConstructorDelegate?)frame.JsonTypeInfo.CreateObjectWithArgs; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs index 867b74f..528ac02 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs @@ -76,8 +76,9 @@ namespace System.Text.Json.Serialization.Converters ReadPropertyValue(obj, ref state, ref tempReader, jsonPropertyInfo, useExtensionProperty); } - ArrayPool.Shared.Return(argumentState.FoundProperties!, clearArray: true); + FoundProperty[] toReturn = argumentState.FoundProperties!; argumentState.FoundProperties = null; + ArrayPool.Shared.Return(toReturn, clearArray: true); } } else @@ -133,8 +134,9 @@ namespace System.Text.Json.Serialization.Converters } } - ArrayPool.Shared.Return(argumentState.FoundPropertiesAsync!, clearArray: true); + FoundPropertyAsync[] toReturn = argumentState.FoundPropertiesAsync!; argumentState.FoundPropertiesAsync = null; + ArrayPool.Shared.Return(toReturn, clearArray: true); } } @@ -238,9 +240,10 @@ namespace System.Text.Json.Serialization.Converters argumentState.FoundProperties.CopyTo(newCache, 0); - ArrayPool.Shared.Return(argumentState.FoundProperties, clearArray: true); - + FoundProperty[] toReturn = argumentState.FoundProperties; argumentState.FoundProperties = newCache!; + + ArrayPool.Shared.Return(toReturn, clearArray: true); } argumentState.FoundProperties[argumentState.FoundPropertyCount++] = ( @@ -436,9 +439,10 @@ namespace System.Text.Json.Serialization.Converters argumentState.FoundPropertiesAsync!.CopyTo(newCache, 0); - ArrayPool.Shared.Return(argumentState.FoundPropertiesAsync!, clearArray: true); - + FoundPropertyAsync[] toReturn = argumentState.FoundPropertiesAsync!; argumentState.FoundPropertiesAsync = newCache!; + + ArrayPool.Shared.Return(toReturn, clearArray: true); } // Cache the property name and value. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadBufferState.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadBufferState.cs index 395c5f9..d909716 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadBufferState.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadBufferState.cs @@ -25,8 +25,11 @@ namespace System.Text.Json.Serialization { // Clear only what we used and return the buffer to the pool new Span(Buffer, 0, ClearMax).Clear(); - ArrayPool.Shared.Return(Buffer); + + byte[] toReturn = Buffer; Buffer = null!; + + ArrayPool.Shared.Return(toReturn); } } }