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

ARROW-7040: [C#] - System.Memory Span.CopyTo - Crashes #6122

Closed
wants to merge 9 commits into from
Closed
7 changes: 7 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,10 @@ site/
**/*.Rcheck/
**/.Rhistory
.Rproj.user
/.vs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to change the .gitignore file with this change. Can you explain why this is being proposed here?

If there is a C# specific pattern that needs to be ignored (like .vs), there is a csharp/.gitignore file for the C# specific patterns - but it already has .vs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you click "Open in Visual Studio" on github - the root folder is opened and folders are created automatically... Any one else that does that will probably see those folders get created as well - so its just annoying to see those all the time..

/matlab/out/
/java/gandiva/out/
/go/arrow/math/_lib/out/
/go/arrow/memory/_lib/out/
/java/adapter/orc/out/

5 changes: 3 additions & 2 deletions csharp/src/Apache.Arrow/Apache.Arrow.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,11 @@
</EmbeddedResource>
</ItemGroup>


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) inserting this blank line is unnecessary.

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.3'">
<Compile Remove="Extensions\StreamExtensions.netcoreapp2.1.cs" />
<Compile Remove="**/*.netcoreapp2.1.cs" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp2.1'">
<Compile Remove="Extensions\StreamExtensions.netstandard.cs" />
<Compile Remove="**/*.netstandard1.3.cs" />
</ItemGroup>
</Project>
46 changes: 2 additions & 44 deletions csharp/src/Apache.Arrow/ArrowBuffer.Builder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ namespace Apache.Arrow
{
public partial struct ArrowBuffer
{
public class Builder<T>
public partial class Builder<T>
where T : struct
{
private const int DefaultCapacity = 8;
Expand Down Expand Up @@ -59,14 +59,6 @@ public Builder<T> Append(T value)
return this;
}

public Builder<T> Append(ReadOnlySpan<T> source)
{
EnsureCapacity(source.Length);
source.CopyTo(Span.Slice(Length, source.Length));
Length += source.Length;
return this;
}

public Builder<T> AppendRange(IEnumerable<T> values)
{
if (values != null)
Expand Down Expand Up @@ -100,23 +92,7 @@ public Builder<T> Clear()
Length = 0;
return this;
}

public ArrowBuffer Build(MemoryAllocator allocator = default)
{
int currentBytesLength = Length * _size;
int bufferLength = checked((int)BitUtility.RoundUpToMultipleOf64(currentBytesLength));

var memoryAllocator = allocator ?? MemoryAllocator.Default.Value;
var memoryOwner = memoryAllocator.Allocate(bufferLength);

if (memoryOwner != null)
{
Memory.Slice(0, currentBytesLength).CopyTo(memoryOwner.Memory);
}

return new ArrowBuffer(memoryOwner);
}


private void EnsureCapacity(int n)
{
var length = checked(Length + n);
Expand All @@ -129,24 +105,6 @@ private void EnsureCapacity(int n)
Reallocate(capacity);
}
}

private void Reallocate(int length)
{
if (length < 0)
{
throw new ArgumentOutOfRangeException(nameof(length));
}

if (length != 0)
{
var memory = new Memory<byte>(new byte[length]);
Memory.CopyTo(memory);

Memory = memory;
}
}

}

}
}
70 changes: 70 additions & 0 deletions csharp/src/Apache.Arrow/ArrowBuffer.Builder.netcoreapp2.1.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Licensed to the Apache Software Foundation (ASF) under one or more
// contributor license agreements. See the NOTICE file distributed with
// this work for additional information regarding copyright ownership.
// The ASF licenses this file to You under the Apache License, Version 2.0
// (the "License"); you may not use this file except in compliance with
// the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

using Apache.Arrow.Memory;
using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;

namespace Apache.Arrow
{
public partial struct ArrowBuffer
{
public partial class Builder<T>
where T : struct
{

public Builder<T> Append(ReadOnlySpan<T> source)
{
EnsureCapacity(source.Length);
source.CopyTo(Span.Slice(Length, source.Length));
Length += source.Length;
return this;
}

public ArrowBuffer Build(MemoryAllocator allocator = default)
{
int currentBytesLength = Length * _size;
int bufferLength = checked((int)BitUtility.RoundUpToMultipleOf64(currentBytesLength));

var memoryAllocator = allocator ?? MemoryAllocator.Default.Value;
var memoryOwner = memoryAllocator.Allocate(bufferLength);

if (memoryOwner != null)
{
Memory.Slice(0, currentBytesLength).CopyTo(memoryOwner.Memory);
}

return new ArrowBuffer(memoryOwner);
}

private void Reallocate(int length)
{
if (length < 0)
{
throw new ArgumentOutOfRangeException(nameof(length));
}

if (length != 0)
{
var memory = new Memory<byte>(new byte[length]);
Memory.CopyTo(memory);

Memory = memory;
}
}
}
}
}
70 changes: 70 additions & 0 deletions csharp/src/Apache.Arrow/ArrowBuffer.Builder.netstandard1.3.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Licensed to the Apache Software Foundation (ASF) under one or more
// contributor license agreements. See the NOTICE file distributed with
// this work for additional information regarding copyright ownership.
// The ASF licenses this file to You under the Apache License, Version 2.0
// (the "License"); you may not use this file except in compliance with
// the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

using Apache.Arrow.Memory;
using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;

namespace Apache.Arrow
{
public partial struct ArrowBuffer
{
public partial class Builder<T>
where T : struct
{

public Builder<T> Append(ReadOnlySpan<T> source)
{
EnsureCapacity(source.Length);
source.CopyToFix(Span.Slice(Length, source.Length));
Length += source.Length;
return this;
}

public ArrowBuffer Build(MemoryAllocator allocator = default)
{
int currentBytesLength = Length * _size;
int bufferLength = checked((int)BitUtility.RoundUpToMultipleOf64(currentBytesLength));

var memoryAllocator = allocator ?? MemoryAllocator.Default.Value;
var memoryOwner = memoryAllocator.Allocate(bufferLength);

if (memoryOwner != null)
{
Memory.Slice(0, currentBytesLength).CopyToFix(memoryOwner.Memory);
}

return new ArrowBuffer(memoryOwner);
}

private void Reallocate(int length)
{
if (length < 0)
{
throw new ArgumentOutOfRangeException(nameof(length));
}

if (length != 0)
{
var memory = new Memory<byte>(new byte[length]);
Memory.CopyToFix(memory);

Memory = memory;
}
}
}
}
}
5 changes: 4 additions & 1 deletion csharp/src/Apache.Arrow/Extensions/SpanExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,19 @@
// limitations under the License.

using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace Apache.Arrow
{
public static class SpanExtensions
public static partial class SpanExtensions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) double spaces between public and static

{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Span<T> CastTo<T>(this Span<byte> span)
where T: struct =>
MemoryMarshal.Cast<byte, T>(span);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static ReadOnlySpan<T> CastTo<T>(this ReadOnlySpan<byte> span)
where T: struct =>
MemoryMarshal.Cast<byte, T>(span);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Licensed to the Apache Software Foundation (ASF) under one or more
// contributor license agreements. See the NOTICE file distributed with
// this work for additional information regarding copyright ownership.
// The ASF licenses this file to You under the Apache License, Version 2.0
// (the "License"); you may not use this file except in compliance with
// the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

using System;
using System.Runtime.CompilerServices;

namespace Apache.Arrow
{
// TODO: remove all CopyToFix methods after a fix has been released for: https://github.com/dotnet/coreclr/issues/27590
public static partial class SpanExtensions
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void CopyToFix<T>(this ReadOnlySpan<T> source, Span<T> target)
{
CopyToFix(source, 0, target, 0, source.Length);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void CopyToFix<T>(this ReadOnlySpan<T> source, T[] target)
{
CopyToFix(source, 0, target, 0, source.Length);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void CopyToFix<T>(this Span<T> source, Span<T> target)
{
CopyToFix(source, 0, target, 0, source.Length);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void CopyToFix<T>(this Memory<T> source, Memory<T> target)
{
CopyToFix(source.Span, 0, target.Span, 0, source.Length);
}


[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void CopyToFix<T>(ReadOnlySpan<T> source, int sourceOffset, Span<T> target, int targetOffset, int length)
{
for (int i = 0; i < length; ++i)
{
target[targetOffset + i] = source[sourceOffset + i];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we guaranteed that target and source will never overlap in memory?

If they do, this is not going to work correctly. If the beginning of target is at the end of source, this code will mutate the end of source before it reads the last elements of source. Thus the wrong data will be copied to target.

}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void CopyToFix<T>(ReadOnlySpan<T> source, int sourceOffset, T[] target, int targetOffset, int length)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this method? Won't the above method that takes Span<T> target be enough, since T[] is implicitly convertible to Span<T>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might no longer be needed (I removed the usage in flat buffers) - I created different overloads to get the compilation to work when I did a global replace of Span.CopyTo - If i created it, it was being used at some point for compliation

{
for (int i = 0; i < length; ++i)
{
target[targetOffset + i] = source[sourceOffset + i];
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public static int Read(this Stream stream, Memory<byte> buffer)
try
{
int result = stream.Read(sharedBuffer, 0, buffer.Length);
new Span<byte>(sharedBuffer, 0, result).CopyTo(buffer.Span);
new Span<byte>(sharedBuffer, 0, result).CopyToFix(buffer.Span);
return result;
}
finally
Expand All @@ -63,7 +63,7 @@ async ValueTask<int> FinishReadAsync(Task<int> readTask, byte[] localBuffer, Mem
try
{
int result = await readTask.ConfigureAwait(false);
new Span<byte>(localBuffer, 0, result).CopyTo(localDestination.Span);
new Span<byte>(localBuffer, 0, result).CopyToFix(localDestination.Span);
return result;
}
finally
Expand All @@ -83,7 +83,7 @@ public static ValueTask WriteAsync(this Stream stream, ReadOnlyMemory<byte> buff
else
{
byte[] sharedBuffer = ArrayPool<byte>.Shared.Rent(buffer.Length);
buffer.Span.CopyTo(sharedBuffer);
buffer.Span.CopyToFix(sharedBuffer);
return FinishWriteAsync(stream.WriteAsync(sharedBuffer, 0, buffer.Length, cancellationToken), sharedBuffer);
}
}
Expand Down
1 change: 1 addition & 0 deletions csharp/src/Apache.Arrow/Flatbuf/FlatBuffers/ByteBuffer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text;
using Apache.Arrow;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert this change as well?


#if ENABLE_SPAN_T
using System.Buffers.Binary;
Expand Down