[release/6.0] Fix Int32 overflow bug in buffering logic

[github-actions]

Backport of #60459 to release/6.0

/cc @adamsitnik

Customer Impact

Customers who use Seek() in FileStream or BufferedStream with an offset larger than 4GB and then proceed to Read() might get the incorrect data as the buffering logic won't notice that the position has changed and the data present in the buffer should be flushed, this is due an arithmetic overflow error in the Seek implementation.

Some pseudocode to show the problem:

FileStream fs = Open("aBigFile.txt");
fs.Read(100 bytes); // fs reads 4096 bytes into the private buffer, returns first 100 bytes
fs.Seek(4GB); // fs moves the Position by 4GB, but it does not flush the buffer **(bug)**
fs.Read(100 bytes); // fs returns invalid data (bytes from 100 to 200, not 100+4GB to 100+4GB+100) from the buffer that should have been flushed 

Testing

A unit test that exercises the edge case scenario has been added. It was failing before the fix has been applied, now it's passing.

Risk

We believe it's a minimal change required to fix the bug. It's low risk as it's just removing the cast from long to int which was causing the problem.
Furthermore, this issue has been present in BufferedStream since 1.0 and it was found now that FileStream copies its logic internally.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Backport of #60459 to release/6.0

/cc @adamsitnik

Customer Impact

Testing

Risk

Author:	github-actions[bot]
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[adamsitnik]

@danmoseley since @jeffhandley is offline today, could you PTAL? The bug is serious, but it's an edge case and the change is very small.

[danmoseley]

@adamsitnik I think we should take this. Please send the usual email.

[danmoseley]

(It would meet the servicing bar, I think)

[danmoseley]

Looks like this test is too mighty for Window 7 x86 -- it's failed twice there. Not enough virtual memory?

   System.IO.FileSystem.Net5Compat.Tests: [Long Running Test] 'System.IO.Tests.FileStream_Read.NoInt32OverflowInTheBufferingLogic', Elapsed: 00:11:00
   System.IO.FileSystem.Net5Compat.Tests: [Long Running Test] 'System.IO.Tests.FileStream_Read.NoInt32OverflowInTheBufferingLogic', Elapsed: 00:13:00
['System.IO.FileSystem.Net5Compat.Tests' END OF WORK ITEM LOG: Command timed out, and was killed]

[jozkee]

@danmoseley why that error doesn't show up in main?

[danmoseley]

My guess is because we have moved some configurations post commit and that isn't ported to 6.0 yet. @aik-jahoda

[adamsitnik]

why that error doesn't show up in main

Because we are no longer testing the .NET 5 Compat implementation in main (#57735)

[danmoseley]

@adamsitnik do you have a concrete theory why we saw the hang/extreme slowness on 32bit? It would make it impossible to allocate a full 2GB array, but if the implementation is even trying to do that (I assume not) it would cause an exception. No doubt the machine has many GB memory so I would not expect paging (although that is OS decision).

[danmoseley]

I can merge this if you guys think it is ready now

[adamsitnik]

I've tried to repro it on my Windows 10 machine and I've failed.

do you have a concrete theory why we saw the hang/extreme slowness on 32bit?

It's a very simple case: just sync file IO. Was it Windows 7? In such case I would imagine that it may block instead of throwing it when runs out of disk space. But I am just guessing.

I can merge this if you guys think it is ready now

I do think it's ready to solve the immediate problem (Int32Overflow in the buffering logic)

For large files support for Win 7 x86, we might have just discovered an issue, similar like I did with WASM: #45954 (comment) That is going to require a separate investigation.

[Anipik]

whats the status of this one ? should we merge it ? tomorrow is the last day

[adamsitnik]

whats the status of this one ? should we merge it ?

It has been approved, please merge it.