Fix bounds checking in PagedCharBuffer and related code
#5354
Conversation
|
/to @pranavkm since you're familiar w/ this code |
| } | ||
|
|
||
| [Fact] | ||
| public async Task FlushAsync_WritesContentToInner() |
There was a problem hiding this comment.
This looks identical to the previous test. What am I missing here?
There was a problem hiding this comment.
Could add the new assertion to the previous test. But they're really looking for unrelated things in the same scenario. Could change the previous test's name and merge the two. LMK if you feel strongly...
There was a problem hiding this comment.
It's fine. I just wasn't sure if this was a copy-paste error
|
|
||
| public override void Return(char[] buffer, bool clearArray = false) | ||
| { | ||
| Returned.Add(buffer); |
There was a problem hiding this comment.
Not using Returned in new test that uses this. But seemed better to be consistent w/ the other test ArrayPool<T> here.
| // Imitate ArrayPool<char>.Shared as it falls back from its default char[1024] Bucket to the next one. | ||
| var bufferSource = new Mock<ICharBufferSource>(); | ||
| bufferSource | ||
| .SetupSequence(s => s.Rent(PagedCharBuffer.PageSize)) |
| var expected = Enumerable.Repeat('d', 2 * PagedCharBuffer.PageSize); | ||
| var laterString = new string('d', PagedCharBuffer.PageSize); | ||
|
|
||
| // Act (loop w/in second Append(string) call). |
There was a problem hiding this comment.
I'll spell out "within" which is what I meant w/in this comment.
| // Arrange | ||
| // Imitate ArrayPool<char>.Shared as it falls back from its default char[1024] Bucket to the next one. | ||
| var bufferSource = new Mock<ICharBufferSource>(); | ||
| bufferSource |
There was a problem hiding this comment.
Could you also add a test for the reverse sequence i.e.
.Returns(new char[2 * PagedCharBuffer.PageSize])
.Returns(new char[PagedCharBuffer.PageSize]);
- #5347 - inconsistent bounds checking caused problems after `ArrayPool<char>` fell back to `new char[2048]` - would fail a `Debug` assertion in Debug builds and loop endlessly in Release builds - change to `CacheTagHelper+CharBufferHtmlContent` is for correctness only - always uses a `CharArrayBufferSource` and that returns arrays of the exact size requested
dougbu
force-pushed
the
dougbu/bounds.fix.5347
branch
from
41aea40 to
2629717
Compare
|
🆙📅 |
ArrayPool<char>fell back tonew char[2048]Debugassertion in Debug builds and loop endlessly in Release buildsCacheTagHelper+CharBufferHtmlContentis for correctness onlyCharArrayBufferSourceand that returns arrays of the exact size requested