Skip to content

[release/6.0.1] Support async operations on unseekable files - #61410

Merged
Anipik merged 1 commit into
dotnet:release/6.0from
adamsitnik:fixNonSeekableFiles
Nov 12, 2021
Merged

[release/6.0.1] Support async operations on unseekable files#61410
Anipik merged 1 commit into
dotnet:release/6.0from
adamsitnik:fixNonSeekableFiles

Conversation

@adamsitnik

@adamsitnikadamsitnik commented Nov 10, 2021

Copy link
Copy Markdown
Member

This is a manual backport of #58434, the problem has been recently discussed for #60931 which has not been merged to 6.0 as it was simply too late.

Customer Impact

This PR actually fixes two bugs.

The first one is a bug reported by customer via email, where an async read operation performed on a FileStream opened for non-seekable device file was throwing an System.IO.IOException: "The parameter is incorrect".

It turns out that ReadFiledoc was wrong:

For an hFile that does not support byte offsets, Offset and OffsetHigh are ignored.

And OVERLAPPEDdoc was right:

This member is nonzero only when performing I/O requests on a seeking device that supports the concept of an offset (also referred to as a file pointer mechanism), such as a file. Otherwise, this member must be zero.

The second bug is #58383. To tell the long story short, File.ReadAllBytes*(string path, ...) methods were assuming that path can't point to a non-seekable file like named pipe.

Testing

The fix was verified using tests added in the PR. It has been in main for a month (#58434) and we have already been doing that for sync file handles (the issue was originally discovered and fixed by @stephentoub):

privatestaticNativeOverlappedGetNativeOverlappedForSyncHandle(SafeFileHandlehandle,longfileOffset)
{
Debug.Assert(!handle.IsAsync);
NativeOverlappedresult=default;
if(handle.CanSeek)
{
result.OffsetLow=unchecked((int)fileOffset);
result.OffsetHigh=(int)(fileOffset>>32);
}

Risk

Very low. :shipit:

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@ghost

Copy link
Copy Markdown

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

Issue Details

Customer Impact

This PR actually fixes two bugs.

The first one is a bug reported by customer via email, where an async read operation performed on a FileStream opened for non-seekable device file was throwing an System.IO.IOException: "The parameter is incorrect".

It turns out that ReadFiledoc was wrong:

For an hFile that does not support byte offsets, Offset and OffsetHigh are ignored.

And OVERLAPPEDdoc was right:

This member is nonzero only when performing I/O requests on a seeking device that supports the concept of an offset (also referred to as a file pointer mechanism), such as a file. Otherwise, this member must be zero.

The second bug is #58383. To tell the long story short, File.ReadAllBytes*(string path, ...) methods were assuming that path can't point to a non-seekable file like named pipe.

Testing

The fix was verified using tests added in the PR. It has been in main for a month (#58434) and we have already been doing that for sync file handles (the issue was originally discovered and fixed by @stephentoub):

privatestaticNativeOverlappedGetNativeOverlappedForSyncHandle(SafeFileHandlehandle,longfileOffset)
{
Debug.Assert(!handle.IsAsync);
NativeOverlappedresult=default;
if(handle.CanSeek)
{
result.OffsetLow=unchecked((int)fileOffset);
result.OffsetHigh=(int)(fileOffset>>32);
}

Risk

Very low. :shipit:

Author:adamsitnik
Assignees:-
Labels:

area-System.IO

Milestone:6.0.1

@danmoseleydanmoseley added the Servicing-consider Issue for next servicing release review label Nov 10, 2021
@jamsheddjamshedd added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 11, 2021
@jamshedd

Copy link
Copy Markdown
Member

Approved for Dec - 6.0.1

@danmoseley

Copy link
Copy Markdown
Contributor

@adamsitnik approved for 6.0.1, but if it's feasible to ask the customer to validate (presumably by picking up current main) that would be ideal.

@danmoseleydanmoseley reopened this Nov 11, 2021
@Anipik
Anipik merged commit 278057b into dotnet:release/6.0Nov 12, 2021
@adamsitnik
adamsitnik deleted the fixNonSeekableFiles branch November 12, 2021 17:36
@ghostghost locked as resolved and limited conversation to collaborators Dec 12, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IOServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@adamsitnik@jamshedd@danmoseley@Anipik@lateapexearlyspeed