MemoryExtensions ToUpper / ToLower don't properly support overlapping buffers

Per the devdoc on MemoryExtensions.ToUpper / ToLower and similar methods, these APIs are supposed to behave correctly if the source and destination buffers partially overlap.

/// <remarks>If the source and destinations overlap, this method behaves as if the original values are in
/// a temporary location before the destination is overwritten.</remarks>

However, I don't think any shipped version of ToUpper or ToLower ever had this behavior. At least, the most recent version just blasts forward without checking to see if the buffers are overlapped.

Span<char> chars = new char[] { 'a', 'b', 'c', 'd' };
((ReadOnlySpan<char>)chars).Slice(0, 2).ToUpperInvariant(chars.Slice(1, 2));
Console.WriteLine(chars.ToString()); // prints "aAAd", not "aABd"

We should update the documentation to reflect the actual behavior or change the implementation to satisfy the documented behavior, whichever is appropriate.

Hmm, that's strange. These tests would have failed if overlapping spans don't work:
https://github.com/dotnet/corefx/blob/db7daabdb592da062d8a80f27bbad1178c364530/src/Common/tests/Tests/System/StringTests.cs#L5185-L5214

FYI, here is when we added the comment + tests: dotnet/corefx@3131eef (as part of dotnet/corefx#27193)

// prints "aAAd", not "aABd"

It prints "aABd" for me (.NET Core 3.0).

Span<char> chars = new char[] { 'a', 'b', 'c', 'd' };
((ReadOnlySpan<char>)chars).Slice(0, 2).ToUpperInvariant(chars.Slice(1, 2));
Console.WriteLine(chars.ToString()); // aABd

It turns out the above example just happens to work on .NET Core 3.0 due to recent changes like dotnet/coreclr#20275 since the input is small.

On .NET Core 2.1, the above example would fail (i.e. print aAAd).

We do not support overlapping spans for ToUpper/ToLower and hence we should change the documentation.

For example, a test like this fails:

[Fact]
public static void ToUpperOverlappingLarger()
{
    var expectedSource = new char[4] { 'a', 'A', 'B', 'C' };
    var expectedDestination = new char[4] { 'A', 'B', 'C', 'D' };
    var expectedWhole = new char[5] { 'a', 'A', 'B', 'C', 'D' };

    char[] a = { 'a', 'b', 'c', 'd', 'e' };

    string s1 = new string(a, 0, 4);                
    Assert.Equal(expectedDestination, s1.ToUpperInvariant().ToArray());

    var source = new ReadOnlySpan<char>(a, 0, 4);
    var destination = a.AsSpan(1, 4);
    Assert.Equal(source.Length, source.ToUpperInvariant(destination));
    Assert.Equal(expectedWhole, a.ToArray());   // Char[] ['a', 'A', 'B', 'B', 'D']
    Assert.Equal(expectedDestination, destination.ToArray()); // Char[] ['A', 'B', 'B', 'D']
    Assert.Equal(expectedSource, source.ToArray()); // Char[] ['a', 'A', 'B', 'B']
}

Can overlapping be determined reliably? If it can be, it should be part of argument validation.

@GSPP

@GSPP Yes. There is an Overlaps extension method in MemoryExtensions:

https://github.com/dotnet/corefx/blob/f078da3a4ddcb99cbb26b59fb67f59fdfb200548/src/Common/src/CoreLib/System/MemoryExtensions.cs#L1461-L1465

@jkotas

Can overlapping be determined reliably? If it can be, it should be part of argument validation.

We should definitely consider it. However, one thing we should measure is potential performance regression that might be introduced due to this check, especially for operations on small payloads. Given that its going through TextInfo anyway, it may not matter all that much. We did recently change the methods being called to statics to avoid the overhead: dotnet/coreclr#17890

cc @jkotas, @tarekgh

owever, one thing we should measure is potential performance regression that might be introduced due to this check, especially for operations on small payloads

This is very cheap check. I am not worried about its cost.

@ahsonkhan

We should definitely consider it.

@ahsonkhan do you mean check and throw exception?

do you mean check and throw exception?

Yes.

@adamsitnik

@adamsitnik, are you still planning on working on this issue?

@ahsonkhan

@ahsonkhan I was hoping to do it when I was flying from Seattle to Europe, but I did not. I have unassigned myself

@GSPP

@GSPP, @ektrah, @MarcoRossignoli, are any of you interested in working on this issue?

Some specs methods to fix

public static int ToLower(this ReadOnlySpan<char> source, Span<char> destination, CultureInfo culture)
public static int ToLowerInvariant(this ReadOnlySpan<char> source, Span<char> destination)
public static int ToUpper(this ReadOnlySpan<char> source, Span<char> destination, CultureInfo culture)
public static int ToUpperInvariant(this ReadOnlySpan<char> source, Span<char> destination)

Should throw internal static void ThrowNotSupportedException(ExceptionResource resource) with new ExceptionResource.NotSupported_SpanOverlappedOperation ?
Error message Operation on overlapped buffers not supported?
Tests on corefx System.Memory close to https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/Memory/IndexAndRange.cs#L38?

Some specs methods to fix

Yes.

Should throw internal static void ThrowNotSupportedException(ExceptionResource resource) with new ExceptionResource.NotSupported_SpanOverlappedOperation ?
Error message Operation on overlapped buffers not supported?

I would throw ArgumentException (or maybe InvalidOperationException, but probably not NotSupportedException). The message seems fine to me.
Maybe This operation is invalid on overlapping buffers.

Tests on corefx System.Memory close to https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/Memory/IndexAndRange.cs#L38?

Most of the ToLower/ToUpper tests live next to the StringTests so that would probably be better.
https://github.com/dotnet/corefx/blob/42ecf717478504fd7cc072f3586c6edd1ba57daa/src/Common/tests/Tests/System/StringTests.cs#L5040-L5053

I would also be OK with them being in a new file next to since they are specifically about spans/overlapping (which isn't relevant in the string case):
https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/ReadOnlySpan/Overlaps.cs

Let me know if you are planning to submit a fix by Monday, because we want to get the fix in before 6/26. Otherwise, just give me a heads up and I will try to address this issue myself.

Thanks.

I'll work to have a fix by monday, if something unexpected will happen I'll tell you asap.

Fixed in dotnet/corefx#38778 / dotnet/corefx#38782

Navigation Menu

Repository navigation

MemoryExtensions ToUpper / ToLower don't properly support overlapping buffers #28494

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

MemoryExtensions ToUpper / ToLower don't properly support overlapping buffers #28494

Description

Activity

ahsonkhan commented on Jan 29, 2019

ahsonkhan commented on Jan 29, 2019

ahsonkhan commented on Feb 14, 2019

GSPP commented on Feb 15, 2019

ektrah commented on Feb 15, 2019

ahsonkhan commented on Feb 15, 2019

jkotas commented on Feb 15, 2019

MarcoRossignoli commented on Feb 27, 2019

ahsonkhan commented on Feb 27, 2019

ahsonkhan commented on Jun 15, 2019

adamsitnik commented on Jun 17, 2019

ahsonkhan commented on Jun 18, 2019

MarcoRossignoli commented on Jun 19, 2019

ahsonkhan commented on Jun 22, 2019

MarcoRossignoli commented on Jun 22, 2019

ahsonkhan commented on Jun 25, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions