Skip to content

Commit

Permalink
don't verify OS handle position
Browse files Browse the repository at this point in the history
  • Loading branch information
@adamsitnik
adamsitnik committed Mar 4, 2021
1 parent e8d6f77 commit d06f175
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 77 deletions.
4 changes: 2 additions & 2 deletions src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,13 @@ public void AccessFlushesFileClosesHandle()
}
}

[Fact]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsLegacyFileStreamEnabled))]
public async Task ThrowWhenHandlePositionIsChanged_sync()
{
await ThrowWhenHandlePositionIsChanged(useAsync: false);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported), nameof(PlatformDetection.IsLegacyFileStreamEnabled))]
public async Task ThrowWhenHandlePositionIsChanged_async()
{
await ThrowWhenHandlePositionIsChanged(useAsync: true);
Expand Down
15 changes: 1 addition & 14 deletions src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ protected override void OnInit()
if (_fileHandle.ThreadPoolBinding == null)
{
// We should close the handle so that the handle is not open until SafeFileHandle GC
Debug.Assert(!_exposedHandle, "Are we closing handle that we exposed/not own, how?");
_fileHandle.Dispose();
}
}
Expand Down Expand Up @@ -145,9 +144,6 @@ private unsafe Task<int> ReadAsyncInternal(Memory<byte> destination, Cancellatio
{
long len = Length;

// Make sure we are reading from the position that we think we are
VerifyOSHandlePosition();

if (_filePosition + destination.Length > len)
{
if (_filePosition <= len)
Expand Down Expand Up @@ -273,9 +269,6 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory<byte> source, Cancella
// Make sure we set the length of the file appropriately.
long len = Length;

// Make sure we are writing to the position that we think we are
VerifyOSHandlePosition();

if (_filePosition + source.Length > len)
{
SetLengthCore(_filePosition + source.Length);
Expand Down Expand Up @@ -381,16 +374,10 @@ private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, Canc
Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed");
Debug.Assert(CanRead, "_parent.CanRead");

bool canSeek = CanSeek;
if (canSeek)
{
VerifyOSHandlePosition();
}

try
{
#pragma warning disable CA2007 // Consider calling ConfigureAwait on the awaited task
await FileStreamHelpers.AsyncModeCopyToAsync(_fileHandle, _path, canSeek, _filePosition, destination, bufferSize, cancellationToken);
await FileStreamHelpers.AsyncModeCopyToAsync(_fileHandle, _path, CanSeek, _filePosition, destination, bufferSize, cancellationToken);
#pragma warning restore CA2007 // Consider calling ConfigureAwait on the awaited task
}
finally
Expand Down
6 changes: 0 additions & 6 deletions src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,6 @@ private unsafe int ReadSpan(Span<byte> destination)

Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed");

// Make sure we are reading from the right spot
VerifyOSHandlePosition();

int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, null, out int errorCode);

if (r == -1)
Expand Down Expand Up @@ -139,9 +136,6 @@ private unsafe void WriteSpan(ReadOnlySpan<byte> source)

Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed");

// Make sure we are writing to the position that we think we are
VerifyOSHandlePosition();

int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, null, out int errorCode);

if (r == -1)
Expand Down
59 changes: 4 additions & 55 deletions src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,10 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy
private readonly bool _canSeek;
private readonly bool _isPipe; // Whether to disable async buffering code.

/// <summary>Whether the file stream's handle has been exposed.</summary>
protected bool _exposedHandle;

private long _appendStart; // When appending, prevent overwriting file.

internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access)
{
_exposedHandle = true;

InitFromHandle(handle, access, out _canSeek, out _isPipe);

// Note: Cleaner to set the following fields in ValidateAndInitFromHandle,
Expand Down Expand Up @@ -87,31 +82,16 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access
/// <summary>Gets or sets the position within the current stream</summary>
public override long Position
{
get
{
VerifyOSHandlePosition();

return _filePosition;
}
set
{
Seek(value, SeekOrigin.Begin);
}
get => _filePosition;
set => Seek(value, SeekOrigin.Begin);
}

internal sealed override string Name => _path ?? SR.IO_UnknownFileName;

internal sealed override bool IsClosed => _fileHandle.IsClosed;

internal sealed override SafeFileHandle SafeFileHandle
{
get
{
// Flushing is the responsibility of BufferedFileStreamStrategy
_exposedHandle = true;
return _fileHandle;
}
}
// Flushing is the responsibility of BufferedFileStreamStrategy
internal sealed override SafeFileHandle SafeFileHandle => _fileHandle;

// ReadByte and WriteByte methods are used only when the user has disabled buffering on purpose
// their performance is going to be horrible
Expand Down Expand Up @@ -171,9 +151,6 @@ public sealed override long Seek(long offset, SeekOrigin origin)
if (_fileHandle.IsClosed) throw Error.GetFileNotOpen();
if (!CanSeek) throw Error.GetSeekNotSupported();

// Verify that internal position is in sync with the handle
VerifyOSHandlePosition();

long oldPos = _filePosition;
long pos = SeekCore(_fileHandle, offset, origin);

Expand Down Expand Up @@ -267,7 +244,6 @@ public sealed override void SetLength(long value)
protected unsafe void SetLengthCore(long value)
{
Debug.Assert(value >= 0, "value >= 0");
VerifyOSHandlePosition();

FileStreamHelpers.SetLength(_fileHandle, _path, value);

Expand All @@ -276,32 +252,5 @@ protected unsafe void SetLengthCore(long value)
SeekCore(_fileHandle, 0, SeekOrigin.End);
}
}

/// <summary>
/// Verify that the actual position of the OS's handle equals what we expect it to.
/// This will fail if someone else moved the UnixFileStream's handle or if
/// our position updating code is incorrect.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
protected void VerifyOSHandlePosition()
{
bool verifyPosition = _exposedHandle; // in release, only verify if we've given out the handle such that someone else could be manipulating it
#if DEBUG
verifyPosition = true; // in debug, always make sure our position matches what the OS says it should be
#endif
if (verifyPosition && CanSeek)
{
long oldPos = _filePosition; // SeekCore will override the current _position, so save it now
long curPos = SeekCore(_fileHandle, 0, SeekOrigin.Current);
if (oldPos != curPos)
{
// For reads, this is non-fatal but we still could have returned corrupted
// data in some cases, so discard the internal buffer. For writes,
// this is a problem; discard the buffer and error out.

throw new IOException(SR.IO_FileStreamHandlePosition);
}
}
}
}
}

0 comments on commit d06f175

Please sign in to comment.