Fix bad usage of ArrayPool in TdsParserStateObject

[bartonjs]

If TdsParserStateObject.TryReadString or
TdsParserStateObject.TryReadStringWithEncoding hit the growth case they
would rent an array, save it into a field, use the array, return the array to the pool,
but keep it assigned to the field and continue using it.

Since other writes to _bTmp use fresh arrays in an instance-cached-growth
pattern, this change restores these two methods to that same approach, rather
than renting to a local buffer and not renting into a field.

[Wraith2]

I'd noticed that I've introduced this problem and have a local branch to fiddle with a fix for it. I don't think the _bTmp variable needs to be used here at all since the entire contents are converted to a string at the end of the method.

[bartonjs]

I don't think the _bTmp variable needs to be used here at all

Yeah, it does seem like it could be something like

buf = _bTmp;

if (buf is not big enough)
{
    buf = rentedBuf = Rent(big enough);
}

do stuff

if (rentedBuf != null)
{
    Return(rentedBuf);
}

That seems like a reasonable alternative to pursue, but the short term goal is to just eliminate where the array is being used after being returned to the pool.

[Wraith2]

Sure, I've no argument against the revert for clearly broken behaviour, I should have been more careful.

I've been considering trying to remove that temp buffer entirely since it's never cleared and there's no telling what might get left in there, That's a future PR though.

I am curious how you found it though. I'm guessing you've got an analyser for arraypool usage so you can audit the crypto stuff?

[bartonjs]

I am curious how you found it though. I'm guessing you've got an analyser for arraypool usage so you can audit the crypto stuff?

Exhaustive iterative analysis utilizing a speculative prediction via moderately trained wetware.

(I looked at literally everything that used ArrayPool to see what sorts of things might go wrong 😄)

[Wraith2]

AKA the hard way 😁 . Well thanks for finding and fixing.

[danmoseley]

would this help? https://github.com/dotnet/coreclr/issues/9885

[Wraith2]

would this help? dotnet/coreclr#9885

Perhaps but only if we can catch the error during testing. Looking at the code the problem is reasonably easy to spot which is why I thought there might be an analyser. I think it should be quite rare to correctly assign a rented buffer to an instance variable which is what goes wrong here.

I've writtenref struct StackArrayPoolRental<T> and class HeapArrayPoolRental<T> disposable classes to help me not make mistakes with rentals elsewhere, I'm assuming there was some reason for this sort of thing not to be in the BCL.

[divega]

I've writtenref struct StackArrayPoolRental and class HeapArrayPoolRental disposable classes to help me not make mistakes with rentals elsewhere, I'm assuming there was some reason for this sort of thing not to be in the BCL.

@Wraith2 you may want to raise this question in a new issue.