Runtime: Bug: string.CompareOrdinal reads beyond the string

[rikimaru0345]

Hi,

it seems the string.CompareOrdinal() method on .NET Framework 4.7.2 has a bug.
I found this while investigating a bug report in my library (Ceras) and I also posted an overview of what I found there:

https://github.com/rikimaru0345/Ceras/issues/62#issuecomment-521343657

The method compares data after the end of the string(s).
Maybe I'm doing something wrong?

I got the other string from _utf8Encoding.GetString(...) (an instance from new UTF8Encoding(false, true);).

Looking at the two strings in the memory view shows that they're perfectly identical, so I think it should return 0.

Let me know if I should just copy the whole comment into this post.

[rikimaru0345]

It seems EqualsHelper (the internal function used by string.Equals(object) has a very similar bug and also includes 2 additional bytes after the strings end.
I updated my comment in the Ceras repo accordingly.

Not sure if I should report that as a separate bug or not. They're two completely distinct methods after all...

[0xd4d]

All strings have an extra 0x0000 (16-bit) value after the real string data. The first string in your picture doesn't have this value in the "Memory 1" hex viewer, the 2nd "Memory 2" hex viewer shows that the 2nd string has it. How did you create the 1st string?

[rikimaru0345]

Ah, I see, I got the first string from var str = _utf8Encoding.GetString(buffer, offset, length);
with the encoding created by UTF8Encoding _utf8Encoding = new UTF8Encoding(false, true);.

[0xd4d]

Seems like they're not null-terminated by encoding.GetString().

[stephentoub]

@rikimaru0345, can you please share a small repro of the problem?

Also, you mention .NET 4.7.2; does the problem also repro on .NET Core?

[rikimaru0345]

@stephentoub
Should be no problem, I'll prepare something and post it here soon!
I test .NET Core as well and report back on that.

[rikimaru0345]

Repro project here: https://github.com/rikimaru0345/BugRepro

• Run the console program "CerasDictionaryIssue"
• Before Console.WriteLine("exception should have been triggered"); will appear, you'll get an exception here:
  

In the screenshot you can see that, there's no custom code between the call to GetString and the part where I check for \0.

You can then just move the execution cursor around the exception (or comment it out), and continue. After that you'll eventually end up here:

Just set a breakpoint there like I did.

There must either be a \0 in the string (but there isn't) or the compare functions have to be changed.

The project includes all needed code (a full copy of the ceras project) and is set to .NET Framework.

I'll work on changing the project to .net core now and then test it there... 😄 👍

[rikimaru0345]

Same thing in netcoreapp2.2 (test project) + netstandard2.0 (ceras):

I've updated the repro project so it can be ran with the new csproj (<TargetFrameworks>net47;netcoreapp2.2;</TargetFrameworks>)

edit:
Does anyone know if I could workaround this by simply overwriting the char after the last with 0? like strPtr[length] = '\0';? Or in other words, is that byte possibly part of another object?

I tried to follow the code but It ends in internal static extern string FastAllocateString(int length);

[dbolin]

This isn't a bug in the runtime - this is a bug in your unsafe code.

When deserializing, the first element of the allocated bool[] was at 0x0000014646ae8970
The first byte of string created by GetString() was at 0x0000014646ae8a4c

In ReinterpretFormatter.cs:210, 240 bytes were copied to 0x0000014646ae8970 - ending at 0x0000014646ae8a60, overlapping with where the string is later allocated.

[rikimaru0345]

This isn't a bug in the runtime - this is a bug in your unsafe code.

When deserializing, the first element of the allocated bool[] was at 0x0000014646ae8970
The first byte of string created by GetString() was at 0x0000014646ae8a4c

In ReinterpretFormatter.cs:210, 240 bytes were copied to 0x0000014646ae8970 - ending at 0x0000014646ae8a60, overlapping with where the string is later allocated.

Wow, I'll check it out, thanks!

[rikimaru0345]

It was indeed a bug in calculating the size of bool.
Marhsal.SizeOf(typeof(bool)) reports 4 bytes, Unsafe.SizeOf<bool>() reports 1 byte.