Fix EncoderNLS / DecoderNLS regression in reporting error index (#25397)

Also fixes incorrect asserts in the Encoding type

Author: GrabYourPitchforks

src/System.Private.CoreLib/shared/System/Text/DecoderNLS.cs

@@ -296,9 +296,10 @@ internal int DrainLeftoverDataForGetCharCount(ReadOnlySpan<byte> bytes, out int
                     break;
             }
 
-            // Couldn't decode the buffer. Fallback the buffer instead.
+            // Couldn't decode the buffer. Fallback the buffer instead. See comment in DrainLeftoverDataForGetChars
+            // for more information on why a negative index is provided.
 
-            if (FallbackBuffer.Fallback(combinedBuffer.Slice(0, combinedBufferBytesConsumed).ToArray(), index: 0))
+            if (FallbackBuffer.Fallback(combinedBuffer.Slice(0, combinedBufferBytesConsumed).ToArray(), index: -_leftoverByteCount))
             {
                 charCount = _fallbackBuffer!.DrainRemainingDataForGetCharCount();
                 Debug.Assert(charCount >= 0, "Fallback buffer shouldn't have returned a negative char count.");
@@ -360,9 +361,13 @@ internal int DrainLeftoverDataForGetChars(ReadOnlySpan<byte> bytes, Span<char> c
                     break;
             }
 
-            // Couldn't decode the buffer. Fallback the buffer instead.
+            // Couldn't decode the buffer. Fallback the buffer instead. The fallback mechanism relies
+            // on a negative index to convey "the start of the invalid sequence was some number of
+            // bytes back before the current buffer." Since we know the invalid sequence must have
+            // started at the beginning of our leftover byte buffer, we can signal to our caller that
+            // they must backtrack that many bytes to find the real start of the invalid sequence.
 
-            if (FallbackBuffer.Fallback(combinedBuffer.Slice(0, combinedBufferBytesConsumed).ToArray(), index: 0)
+            if (FallbackBuffer.Fallback(combinedBuffer.Slice(0, combinedBufferBytesConsumed).ToArray(), index: -_leftoverByteCount)
                 && !_fallbackBuffer!.TryDrainRemainingDataForGetChars(chars, out charsWritten))
             {
                 goto DestinationTooSmall;

src/System.Private.CoreLib/shared/System/Text/EncoderNLS.cs

@@ -302,12 +302,18 @@ internal int DrainLeftoverDataForGetByteCount(ReadOnlySpan<char> chars, out int
                     }
                     else
                     {
-                        didFallback = FallbackBuffer.Fallback(_charLeftOver, secondChar, index: 0);
+                        // The fallback mechanism relies on a negative index to convey "the start of the invalid
+                        // sequence was some number of chars back before the current buffer." In this block and
+                        // in the block immediately thereafter, we know we have a single leftover high surrogate
+                        // character from a previous operation, so we provide an index of -1 to convey that the
+                        // char immediately before the current buffer was the start of the invalid sequence.
+
+                        didFallback = FallbackBuffer.Fallback(_charLeftOver, secondChar, index: -1);
                     }
                 }
                 else
                 {
-                    didFallback = FallbackBuffer.Fallback(_charLeftOver, index: 0);
+                    didFallback = FallbackBuffer.Fallback(_charLeftOver, index: -1);
                 }
 
                 // Now tally the number of bytes that would've been emitted as part of fallback.
@@ -367,7 +373,7 @@ internal bool TryDrainLeftoverDataForGetBytes(ReadOnlySpan<char> chars, Span<byt
                             break;
 
                         case OperationStatus.InvalidData:
-                            FallbackBuffer.Fallback(_charLeftOver, secondChar, index: 0);
+                            FallbackBuffer.Fallback(_charLeftOver, secondChar, index: -1); // see comment in DrainLeftoverDataForGetByteCount
                             break;
 
                         default:
@@ -377,7 +383,7 @@ internal bool TryDrainLeftoverDataForGetBytes(ReadOnlySpan<char> chars, Span<byt
                 }
                 else
                 {
-                    FallbackBuffer.Fallback(_charLeftOver, index: 0);
+                    FallbackBuffer.Fallback(_charLeftOver, index: -1); // see comment in DrainLeftoverDataForGetByteCount
                 }
             }
 

src/System.Private.CoreLib/shared/System/Text/Encoding.Internal.cs

@@ -257,7 +257,7 @@ private protected unsafe int GetByteCountWithFallback(char* pCharsOriginal, int
         private unsafe int GetByteCountWithFallback(char* pOriginalChars, int originalCharCount, int charsConsumedSoFar, EncoderNLS encoder)
         {
             Debug.Assert(encoder != null, "This code path should only be called from EncoderNLS.");
-            Debug.Assert(0 <= charsConsumedSoFar && charsConsumedSoFar < originalCharCount, "Caller should've checked this condition.");
+            Debug.Assert(0 <= charsConsumedSoFar && charsConsumedSoFar <= originalCharCount, "Caller should've checked this condition.");
 
             // First, try draining any data that already exists on the encoder instance. If we can't complete
             // that operation, there's no point to continuing down to the main workhorse methods.
@@ -523,7 +523,7 @@ private protected unsafe int GetBytesWithFallback(char* pOriginalChars, int orig
         private unsafe int GetBytesWithFallback(char* pOriginalChars, int originalCharCount, byte* pOriginalBytes, int originalByteCount, int charsConsumedSoFar, int bytesWrittenSoFar, EncoderNLS encoder)
         {
             Debug.Assert(encoder != null, "This code path should only be called from EncoderNLS.");
-            Debug.Assert(0 <= charsConsumedSoFar && charsConsumedSoFar < originalCharCount, "Caller should've checked this condition.");
+            Debug.Assert(0 <= charsConsumedSoFar && charsConsumedSoFar <= originalCharCount, "Caller should've checked this condition.");
             Debug.Assert(0 <= bytesWrittenSoFar && bytesWrittenSoFar <= originalByteCount, "Caller should've checked this condition.");
 
             // First, try draining any data that already exists on the encoder instance. If we can't complete
@@ -843,7 +843,7 @@ private protected unsafe int GetCharCountWithFallback(byte* pBytesOriginal, int
         private unsafe int GetCharCountWithFallback(byte* pOriginalBytes, int originalByteCount, int bytesConsumedSoFar, DecoderNLS decoder)
         {
             Debug.Assert(decoder != null, "This code path should only be called from DecoderNLS.");
-            Debug.Assert(0 <= bytesConsumedSoFar && bytesConsumedSoFar < originalByteCount, "Caller should've checked this condition.");
+            Debug.Assert(0 <= bytesConsumedSoFar && bytesConsumedSoFar <= originalByteCount, "Caller should've checked this condition.");
 
             // First, try draining any data that already exists on the decoder instance. If we can't complete
             // that operation, there's no point to continuing down to the main workhorse methods.
@@ -1111,7 +1111,7 @@ private protected unsafe int GetCharsWithFallback(byte* pOriginalBytes, int orig
         private protected unsafe int GetCharsWithFallback(byte* pOriginalBytes, int originalByteCount, char* pOriginalChars, int originalCharCount, int bytesConsumedSoFar, int charsWrittenSoFar, DecoderNLS decoder)
         {
             Debug.Assert(decoder != null, "This code path should only be called from DecoderNLS.");
-            Debug.Assert(0 <= bytesConsumedSoFar && bytesConsumedSoFar < originalByteCount, "Caller should've checked this condition.");
+            Debug.Assert(0 <= bytesConsumedSoFar && bytesConsumedSoFar <= originalByteCount, "Caller should've checked this condition.");
             Debug.Assert(0 <= charsWrittenSoFar && charsWrittenSoFar <= originalCharCount, "Caller should've checked this condition.");
 
             // First, try draining any data that already exists on the encoder instance. If we can't complete