[release/6.0-rc2] Fix buffering bugs in TextFieldParser #59506
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Three bugs being fixed here: 1. The SlideCursorToStartOfBuffer method is incorrectly assuming that the buffer is filled to its end (that m_Buffer.Length = m_CharsRead). As a result, two things happen. First, we copy more data than is needed to the temporary buffer; that's just unnecessary but not harmful. But second, when it issues a read to fill the remainder of the buffer, it does so at the wrong position, both leaving zeros in the buffer that end up getting parsed as real data and losing real data from the end. 2. IncreaseBufferSize assumes m_CharsRead = m_Buffer.Length, which may not be the case. As with (1), this can result in it reading into the wrong part of the array and leaving garbage that gets processed. 3. IncreaseBufferSize is always increasing the buffer size, even if only a small portion of the buffer is used. This can then result in the whole operation failing when it repeatedly increases the buffer size and ends up failing due to ticking over the max buffer threshold. (1) doesn't exist in .NET Framework 4.8 but (2) and (3) do. (1) is easily triggered by a stream that sporadically doesn't produce all the data requested; (2) and (3) end up needing a stream that frequently produces much less than requested. This PR fixes all three.
|
Tagging subscribers to this area: @cston Issue DetailsBackport of #59487 to release/6.0-rc2 /cc @stephentoub Customer ImpactTestingRisk
|
cston
approved these changes
Sep 23, 2021
|
cc @jaredpar |
jamshedd
added
Servicing-approved
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #59487 to release/6.0-rc2
/cc @stephentoub
Customer Impact
TextFieldParser has a variety of bugs that cause it to misbehave in various ways if the underlying Stream being read from returns less data than requested. This can manifest as giving back invalid data (e.g. strings containing lots of null characters) or missing data or various kinds of exceptions about failed parsing or an inability to grow the underlying buffer.
Testing
New tests in addition to the existing ones.
Risk
It's changing VB code, which I'm not a native speaker of 😄