From 53d87e378b577d282f481e6a3aa3b724526f7cb6 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 9 Dec 2022 01:28:41 +0000 Subject: [PATCH 1/3] add array rental capability in RryReadPlpUnicodeChars --- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 64 +++++++++++++++---- 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs index 8217604c72..483a7be95c 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -5684,8 +5684,8 @@ private bool TryReadSqlStringValue(SqlBuffer value, byte type, int length, Encod if (isPlp) { char[] cc = null; - - if (!TryReadPlpUnicodeChars(ref cc, 0, length >> 1, stateObj, out length)) + bool buffIsRented = false; + if (!TryReadPlpUnicodeChars(ref cc, 0, length >> 1, stateObj, out length, supportRentedBuff: true, rentedBuff: ref buffIsRented)) { return false; } @@ -5697,6 +5697,16 @@ private bool TryReadSqlStringValue(SqlBuffer value, byte type, int length, Encod { s = ""; } + if (buffIsRented) + { + // do not use clearArray:true on the rented array because it can be massively larger + // than the space we've used and we would incur performance clearing memory that + // we haven't used and can't leak out information. + // clear only the length that we know we have used. + cc.AsSpan(0, length).Clear(); + ArrayPool.Shared.Return(cc, clearArray: false); + cc = null; + } } else { @@ -12512,8 +12522,9 @@ private bool TryReadPlpUnicodeCharsChunk(char[] buff, int offst, int len, TdsPar internal int ReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsParserStateObject stateObj) { int charsRead; + bool rentedBuff = false; Debug.Assert(stateObj._syncOverAsync, "Should not attempt pends in a synchronous call"); - bool result = TryReadPlpUnicodeChars(ref buff, offst, len, stateObj, out charsRead); + bool result = TryReadPlpUnicodeChars(ref buff, offst, len, stateObj, out charsRead, supportRentedBuff: false, ref rentedBuff); if (!result) { throw SQL.SynchronousCallMayNotPend(); @@ -12525,8 +12536,9 @@ internal int ReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsParserS // requested length is -1 or larger than the actual length of data. First call to this method // should be preceeded by a call to ReadPlpLength or ReadDataLength. // Returns the actual chars read. - internal bool TryReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsParserStateObject stateObj, out int totalCharsRead) + internal bool TryReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsParserStateObject stateObj, out int totalCharsRead, bool supportRentedBuff, ref bool rentedBuff) { + bool inputWasRentedBuff = rentedBuff; int charsRead = 0; int charsLeft = 0; char[] newbuf; @@ -12538,16 +12550,31 @@ internal bool TryReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsPar return true; // No data } - Debug.Assert(((ulong)stateObj._longlen != TdsEnums.SQL_PLP_NULL), - "Out of sync plp read request"); + Debug.Assert(((ulong)stateObj._longlen != TdsEnums.SQL_PLP_NULL),"Out of sync plp read request"); Debug.Assert((buff == null && offst == 0) || (buff.Length >= offst + len), "Invalid length sent to ReadPlpUnicodeChars()!"); charsLeft = len; - // If total length is known up front, allocate the whole buffer in one shot instead of realloc'ing and copying over each time - if (buff == null && stateObj._longlen != TdsEnums.SQL_PLP_UNKNOWNLEN) + // If total length is known up front, the length isn't specified as unknown + // and the caller doesn't pass int.max/2 indicating that it doesn't know the length + // allocate the whole buffer in one shot instead of realloc'ing and copying over each time + if (buff == null && stateObj._longlen != TdsEnums.SQL_PLP_UNKNOWNLEN && len < (int.MaxValue >> 1)) { - buff = new char[(int)Math.Min((int)stateObj._longlen, len)]; + if (supportRentedBuff && len < 1073741824) // 1 Gib + { + if (buff != null && rentedBuff) + { + buff.AsSpan(0, offst).Clear(); + ArrayPool.Shared.Return(buff, clearArray: false); + } + buff = ArrayPool.Shared.Rent(len); + rentedBuff = true; + } + else + { + buff = new char[(int)Math.Min((int)stateObj._longlen, len)]; + rentedBuff = false; + } } if (stateObj._longlenleft == 0) @@ -12572,11 +12599,26 @@ internal bool TryReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsPar charsRead = (int)Math.Min((stateObj._longlenleft + 1) >> 1, (ulong)charsLeft); if ((buff == null) || (buff.Length < (offst + charsRead))) { - // Grow the array - newbuf = new char[offst + charsRead]; + bool returnRentedBufferAfterCopy = rentedBuff; + if (supportRentedBuff && (offst + charsRead) < 1073741824) // 1 Gib + { + newbuf = ArrayPool.Shared.Rent(offst + charsRead); + rentedBuff = true; + } + else + { + newbuf = new char[offst + charsRead]; + rentedBuff = false; + } + if (buff != null) { Buffer.BlockCopy(buff, 0, newbuf, 0, offst * 2); + if (returnRentedBufferAfterCopy) + { + buff.AsSpan(0, offst).Clear(); + ArrayPool.Shared.Return(buff, clearArray: false); + } } buff = newbuf; } From debe78bf605573e4d6619fe07c6cf196fc3738ef Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Mon, 12 Dec 2022 20:36:37 +0000 Subject: [PATCH 2/3] remove unneeded variable --- .../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs index 483a7be95c..1491335980 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -12538,7 +12538,6 @@ internal int ReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsParserS // Returns the actual chars read. internal bool TryReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsParserStateObject stateObj, out int totalCharsRead, bool supportRentedBuff, ref bool rentedBuff) { - bool inputWasRentedBuff = rentedBuff; int charsRead = 0; int charsLeft = 0; char[] newbuf; From e845b1d009c0915b37966f64c983e7d766abaf7a Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Mon, 12 Dec 2022 20:57:25 +0000 Subject: [PATCH 3/3] remove uneeded rental buffer return check --- .../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs index 1491335980..7b700a1edd 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -12561,12 +12561,7 @@ internal bool TryReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsPar { if (supportRentedBuff && len < 1073741824) // 1 Gib { - if (buff != null && rentedBuff) - { - buff.AsSpan(0, offst).Clear(); - ArrayPool.Shared.Return(buff, clearArray: false); - } - buff = ArrayPool.Shared.Rent(len); + buff = ArrayPool.Shared.Rent((int)Math.Min((int)stateObj._longlen, len)); rentedBuff = true; } else