Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RandomAccess.Write throws System.IO.IOException: The parameter is incorrect. #108322

Closed
miroslavp opened this issue Sep 27, 2024 · 7 comments · Fixed by #108380
Closed

RandomAccess.Write throws System.IO.IOException: The parameter is incorrect. #108322

miroslavp opened this issue Sep 27, 2024 · 7 comments · Fixed by #108380
Assignees
Labels
area-System.IO bug in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@miroslavp
Copy link

miroslavp commented Sep 27, 2024

Description

RandomAccess.Write(SafeFileHandle handle, IReadOnlyList<ReadOnlyMemory<byte>> buffers, long fileOffset) throws IOException on Windows platform when the bytes in the buffers list exceed Int32.MaxValue

Here's the relevant stack trace

System.IO.IOException: The parameter is incorrect.
 at System.IO.RandomAccess.WriteAtOffset(SafeFileHandle handle, ReadOnlySpan`1 buffer, Int64 fileOffset)
 at System.IO.RandomAccess.WriteGatherAtOffset(SafeFileHandle handle, IReadOnlyList`1 buffers, Int64 fileOffset)

I suspect the problem is in the bytesWritten variable, because it is int and it overflows

internal static void WriteGatherAtOffset(SafeFileHandle handle, IReadOnlyList<ReadOnlyMemory<byte>> buffers, long fileOffset)
{
// WriteFileGather does not support sync handles, so we just call WriteFile in a loop
int bytesWritten = 0;
int buffersCount = buffers.Count;
for (int i = 0; i < buffersCount; i++)
{
ReadOnlySpan<byte> span = buffers[i].Span;
WriteAtOffset(handle, span, fileOffset + bytesWritten);
bytesWritten += span.Length;
}
}

Reproduction Steps

Just call RandomAccess.Write(handle, buffers, 0) on Windows machine where buffers parameter contains multiple ReadOnlyMemory<byte> buffers that collectively are larger than 2GB

Expected behavior

It should save the buffers to the file and not throw IOException

Actual behavior

throws System.IO.IOException: The parameter is incorrect.

Regression?

Haven't tried it on .NET 6

Known Workarounds

I guess you can split the buffers in multiple smaller lists and call the method multiple times

Configuration

Which version of .NET is the code running on?
.NET 7

What OS and version, and what distro if applicable?
Windows 10

What is the architecture (x64, x86, ARM, ARM64)?
x64

Do you know whether it is specific to that configuration?
I believe the problem is in the Windows implementation only

Other information

No response

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Sep 27, 2024
Copy link
Contributor

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

@KalleOlaviNiemitalo
Copy link

This is not a regression, because RandomAccess.Write was added in .NET 6.0.0, where it already uses the wrong type in int bytesWritten:

internal static void WriteGatherAtOffset(SafeFileHandle handle, IReadOnlyList<ReadOnlyMemory<byte>> buffers, long fileOffset)
{
// WriteFileGather does not support sync handles, so we just call WriteFile in a loop
int bytesWritten = 0;
int buffersCount = buffers.Count;
for (int i = 0; i < buffersCount; i++)
{
ReadOnlySpan<byte> span = buffers[i].Span;
WriteAtOffset(handle, span, fileOffset + bytesWritten);
bytesWritten += span.Length;
}
}

RandomAccess.Read does not have a similar bug; there, RandomAccess.ReadScatterAtOffset correctly uses long total.

@KalleOlaviNiemitalo
Copy link

I think the buggy WriteGatherAtOffset can also silently write to the wrong offset and thus corrupt the file, if the fileOffset parameter is already greater than Int32.MaxValue; then the sum fileOffset + bytesWritten won't be negative and won't cause IOException.

@adamsitnik
Copy link
Member

@miroslavp thank you for a detailed bug report. You are right, it's a bug. Since you have found the place that requires the fix, would you like to send a PR with a fix?

@adamsitnik adamsitnik added bug and removed untriaged New issue has not been triaged by the area owner labels Sep 27, 2024
@adamsitnik adamsitnik added this to the 10.0.0 milestone Sep 27, 2024
@NicoAvanzDev
Copy link
Contributor

I've opened a pull request with the fix for this issue. Let me know if it's ok for you guys. Thanks you all !

@miroslavp
Copy link
Author

@miroslavp thank you for a detailed bug report. You are right, it's a bug. Since you have found the place that requires the fix, would you like to send a PR with a fix?

Thank you, but I prefer not to. I don't feel confident enough to do that. If you guys can fix it, that would be awesome.

@NicoAvanzDev
Copy link
Contributor

NicoAvanzDev commented Sep 30, 2024

@miroslavp thank you for a detailed bug report. You are right, it's a bug. Since you have found the place that requires the fix, would you like to send a PR with a fix?

Thank you, but I prefer not to. I don't feel confident enough to do that. If you guys can fix it, that would be awesome.

I've created a PR with the fix and performed testing on it #108380.

Hope this can be appreciated ! Thanks guys for your job !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO bug in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
4 participants