Fix string.strlen

[benaadams]

Fixes #22393

What's the best approach for adding a coreclr test for this? (e.g. the code from #22393); as the coreclr tests are a little different than corefx.

/cc @jkotas @stephentoub

[stephentoub]

The object couldn't be moved after the AsPointer check?

[benaadams]

string.strlen uses byte* so its fixed data; for public use I don't think this method offers any guarantees for reading out of bounds over unfixed managed data.

[benaadams]

e.g. the issue is that string.strlen calls this with length int.MaxValue; if the reads are not aligned then reading the Vector can cross a page boundary into a non-allocated page; creating a segfault, even if the part of the Vector in the page contains the null terminator.

[omariom]

The method is used by public API https://source.dot.net/#System.Private.CoreLib/shared/System/SpanHelpers.Byte.cs,253f89d386c4359c,references

[benaadams]

Yes, but the public api uses don't suggest you can use a length larger than the memory allocated for the array/span. This method will not read passed the length passed in.

The issue is because strlen is looking for a null terminator in an unknown length of memory; without this change it can page fault into non-process memory; with this change it can't (unless the string doesn't have a null terminator)

[benaadams]

Specifically

coreclr/src/System.Private.CoreLib/shared/System/String.cs

Lines 658 to 659 in e78994d

	// IndexOf processes memory in aligned chunks, and thus it won't crash even if it accesses memory beyond the null terminator.
	int length = SpanHelpers.IndexOf(ref *ptr, (byte)'\0', int.MaxValue);

[omariom]

Aah..

[jkotas]

Right, we had two options on how to implement strlen: Either reuse IndexOf and make sure that it handles the int.MaxValue buffer size as unknown buffer size, or create a separate implementation for it that would be almost identical to IndexOf. We have chosen the first one.

[benaadams]

coreclr-ci failure in eventpipeandetw_eventpipeandetw_eventpipeandetw_cmd https://github.com/dotnet/coreclr/issues/19302#issuecomment-460358752

[jkotas]

What's the best approach for adding a coreclr test for this?

It should be possible to write platform-neutral test for this using System.IO.MemoryMappedFiles. It can live in either CoreCLR or CoreFX. I think our default choice would be CoreFX since it is an API test.

[benaadams]

Probably why I couldn't get the char version wcslen to work #21730; likely needs to be tweaked in similar way

[benaadams]

@dotnet-bot test Windows_NT x64 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Windows_NT x64 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Windows_NT x64 Checked jitnox86hwintrinsic

@dotnet-bot test Windows_NT x86 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Windows_NT x86 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Windows_NT x86 Checked jitnox86hwintrinsic

@dotnet-bot test Ubuntu x64 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Ubuntu x64 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Ubuntu x64 Checked jitnox86hwintrinsic

[benaadams]

Ubuntu x64 Checked Build and Test (Jit - TieredCompilation=0 EnableIncompleteISAClass=1 EnableAVX=0)

and

Ubuntu x64 Checked Build and Test (Jit - TieredCompilation=0 EnableIncompleteISAClass=1 FeatureSIMD=0)

Failed with:

14:15:29   starg_ref -> /mnt/j/workspace/dotnet_coreclr/master/checked_ubuntu_prtest/bin/tests/Linux.x64.Checked/JIT/IL_Conformance/Old/Conformance_Base/starg_ref/starg_ref.exe
14:15:30   stind_i4 -> /mnt/j/workspace/dotnet_coreclr/master/checked_ubuntu_prtest/bin/tests/Linux.x64.Checked/JIT/IL_Conformance/Old/Conformance_Base/stind_i4/stind_i4.exe
14:15:30   stind_i8 -> /mnt/j/workspace/dotnet_coreclr/master/checked_ubuntu_prtest/bin/tests/Linux.x64.Checked/JIT/IL_Conformance/Old/Conformance_Base/stind_i8/stind_i8.exe
14:15:30 
MSBUILD : error MSB4166: Child node "3" exited prematurely. Shutting down. Diagnostic information may be found in files in "/tmp/" and will be named MSBuild_*.failure.txt. This location can be changed by setting the MSBUILDDEBUGPATH environment variable to a different directory.
14:15:30 
/mnt/j/workspace/dotnet_coreclr/master/checked_ubuntu_prtest/tests/dir.traversal.targets(25,5): error : (No message specified) [/mnt/j/workspace/dotnet_coreclr/master/checked_ubuntu_prtest/tests/src/dirs.proj]
14:15:30 /mnt/j/workspace/dotnet_coreclr/master/checked_ubuntu_prtest/Tools/IL.targets(49,5): warning MSB5021: Terminating the task executable "sh" and its child processes because the build was canceled. [/mnt/j/workspace/dotnet_coreclr/master/checked_ubuntu_prtest/tests/src/JIT/IL_Conformance/Old/Conformance_Base/stind_r8.ilproj]
14:15:30 
14:15:30 Build FAILED.

Don't think its related?

[BruceForstall]

This change causes x86/x64 JitStress=2 tests to fail: https://github.com/dotnet/coreclr/issues/22436. I'm going to disable the tests for now with #22438, but an alternative is to back this change out until the JIT is fixed.

[benaadams]

Causes an access volition https://github.com/dotnet/coreclr/issues/22393 without this change :-/

[jkotas]

Right, going from intermittent AV to JIT-stress failure is an improvement. We should suppress the JIT stress failures until the JIT is fixed as @BruceForstall has done.