Fix TPA map hash calculation. by viewizard · Pull Request #38016 · dotnet/runtime

viewizard · 2020-06-17T09:29:49Z

The point of issue is "the Turkish-I Problem". After locale changed, towupper() provide another result for "i" and different hash are calculated in case if file name have "i" letter.

Fix #37910

CC @alpencolt @jkotas @HJLeee

Dotnet-GitSync-Bot · 2020-06-17T09:29:52Z

I couldn't figure out the best area label to add to this PR. Please help me learn by adding exactly one area label.

viewizard · 2020-06-17T11:36:57Z

I see some tests failed with

Assert failure(PID 2340 [0x00000924], Thread: 3012 [0x0bc4]): CONTRACT VIOLATION by SString::SString at "F:\workspace\_work\1\s\src\coreclr\src\inc\sstring.inl" @ 231

THROWS called in a NOTHROW region.

                        CONTRACT in SString::SString at "F:\workspace\_work\1\s\src\coreclr\src\inc\sstring.inl" @ 231
VIOLATED-->  CONTRACT in SHash<class BINDER_SPACE::SimpleNameToFileNameMapTraits>::AddOrReplace at "F:\workspace\_work\1\s\src\coreclr\src\inc\shash.inl" @ 587
                        CONTRACT in SHash<class BINDER_SPACE::SimpleNameToFileNameMapTraits>::AddOrReplace at "F:\workspace\_work\1\s\src\coreclr\src\inc\shash.inl" @ 143
                        CONTRACT in CorHost2::CreateAppDomainWithManager at "F:\workspace\_work\1\s\src\coreclr\src\vm\corhost.cpp" @ 573

this PR should be fixed first.

src/coreclr/src/binder/inc/applicationcontext.hpp

ghost · 2020-06-17T12:48:34Z

Tagging subscribers to this area: @vitek-karas
Notify danmosemsft if you want to be subscribed.

The point of issue is "the Turkish-I Problem". After locale changed, towupper() provide another result for "i" and different hash are calculated in case if file name have "i" letter.

vitek-karas · 2020-06-17T13:59:09Z

Could you please add a test for this - maybe just the Turkish i. Probably into this suite: https://github.com/dotnet/runtime/tree/master/src/libraries/System.Runtime.Loader/tests

viewizard · 2020-06-17T16:21:30Z

I believe, I could create test, but I need more time, since I am not really familiar with this test suite.

viewizard · 2020-06-23T14:48:18Z

@vitek-karas I found, that the only way I could guaranteed test internal TPA map for consistent state is

#ifdef _DEBUG
            if (pTpaEntry == nullptr)
            {
                auto CheckAllEntriesBySimpleName = [&](SimpleNameToFileNameMapEntry &element)
                {
                    _ASSERTE(0 != wcscmp(element.m_wszSimpleName, simpleName.GetUnicode()));
                };
                tpaMap->ForEach(CheckAllEntriesBySimpleName);
            }
#endif //_DEBUG

code block after line:

runtime/src/coreclr/src/binder/assemblybinder.cpp

Line 1066 in 97e553f

    
           const SimpleNameToFileNameMapEntry *pTpaEntry = tpaMap->LookupPtr(simpleName.GetUnicode());

With this changes in CoreCLR we could use simple test with setlocale call in https://github.com/dotnet/runtime/tree/master/src/libraries/System.Runtime.Loader/tests
but the problem is - CI should test Libs with CoreCLR Debug build in order to make this work.
Is the any way I could provide Libs test for CoreCLR Debug build?

vitek-karas · 2020-06-23T14:52:11Z

Can't we write a repro based on the issue #37910?
Having internal consistency check in CLR is nice, but it's typically not how we test fixes. After all we do the fix to mitigate a problem observed on all builds (Debug or Release), so the test should validate that the fix works there as well.

viewizard · 2020-06-23T16:12:17Z

Can't we write a repro based on the issue #37910?

This could be tested by direct dotnet application start. Something like simple application:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp5.0</TargetFramework>
  </PropertyGroup>
</Project>

using System;
using System.Runtime.InteropServices;

namespace Test
{
    public class Program
    {
        private const string LibcLibrary = "libc";
        private const string TRLocale = "tr_TR.UTF-8";

        [DllImport(LibcLibrary, EntryPoint = "setlocale")]
        public static extern IntPtr setlocale(int category, [MarshalAs(UnmanagedType.LPStr)] string locale);

        static int Main(string[] args)
        {
            IntPtr res = setlocale(6 /*LC_ALL*/, TRLocale);
            if (TRLocale != Marshal.PtrToStringAnsi(res))
            {
                return 1;
            }
            Console.WriteLine(Marshal.PtrToStringAnsi(res)); // unhandled exception at this call
            
            return 0;
        }
    }
}

viewizard@totoro:~/Desktop/test$ dotnet build -c Release
Microsoft (R) Build Engine version 16.7.0-preview-20309-02+d6862eb21 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
  You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
  Test -> /home/viewizard/Desktop/test/bin/Release/netcoreapp5.0/Test.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:01.45
viewizard@totoro:~/Desktop/test$ dotnet /home/viewizard/Desktop/test/bin/Release/netcoreapp5.0/Test.dll
Unhandled exception. System.IO.FileNotFoundException: Could not load file or assembly 'System.Threading, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.

File name: 'System.Threading, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'
   at System.Console.<get_Out>g__EnsureInitialized|25_0()
   at System.Console.get_Out()
   at System.Console.WriteLine(String value)
   at Test.Program.Main(String[] args)
Aborted (core dumped)

No exception thrown if patch applied, or if TRLocale is en_US.UTF-8, ru_RU.UTF-8 or any other without Turkish-I.
But since this part of CoreCLR work with another logic during xunit test, AssemblyBinder::BindByTpaList() will be never called at Console.WriteLine() after setlocale with simple name already in TPA map but with different hash, so, no exception will be thrown.

jkotas · 2020-06-23T16:16:29Z

You are right that the xunit harness under libraries does not make it easy to test.

Writing this as a standalone test under coreclr should work better, e.g. you can start with something like this
jkotas@d8b59c9

viewizard · 2020-06-25T15:29:00Z

I added test. Please note, Linux must have tr_TR.UTF-8 locale installed/generated in system, or test will fail at setlocale call.

jkotas

Thank you!

jkotas · 2020-06-25T16:05:27Z

Linux must have tr_TR.UTF-8 locale installed/generated in system

Would it be better for the test to succeed when this locale is not installed in the system?

viewizard · 2020-06-25T16:23:04Z

I am not sure about this, since in this case test could never warn you about regression in code. With current implementation, you at least will see, that some tests are failed due to system environment.

BruceForstall · 2020-06-26T16:03:35Z

As predicted above, this test is failing on Linux, in all outerloop jobs.

Unfortunately, this test was added as a "Pri-1" test, but the outerloop / Pri-1 tests were not triggered as part of this PR. In the future, authors and code reviewers, please pay attention to that.

BruceForstall · 2020-06-26T16:05:53Z

Actually, the test only runs on Linux, so maybe it only passes on arm32/arm64 Linux runs, but no other?

viewizard · 2020-06-26T16:46:28Z

Actually, the test only runs on Linux, so maybe it only passes on arm32/arm64 Linux runs, but no other?

Was tested on amd64 too.
Error message:

...
Failed! tr_TR.UTF-8 locale was not found in system!
...

#38016 (comment)
I mentioned, that tr_TR.UTF-8 locale must be installed/generated in system.

jkotas reviewed Jun 17, 2020

View reviewed changes

src/coreclr/src/binder/inc/applicationcontext.hpp Outdated Show resolved Hide resolved

jkotas reviewed Jun 17, 2020

View reviewed changes

src/coreclr/src/binder/inc/applicationcontext.hpp Outdated Show resolved Hide resolved

jkotas added the area-AssemblyLoader-coreclr label Jun 17, 2020

Fix TPA map hash calculation.

974a4ac

The point of issue is "the Turkish-I Problem". After locale changed, towupper() provide another result for "i" and different hash are calculated in case if file name have "i" letter.

Regression test for #37910

6b9c2cb

jkotas approved these changes Jun 25, 2020

View reviewed changes

jkotas merged commit 4d561f4 into dotnet:master Jun 25, 2020

BruceForstall mentioned this pull request Jun 26, 2020

Test failed: Loader/binding/assemblies/assemblybugs/37910/Ii/Ii.sh #38452

Closed

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Conversation

viewizard commented Jun 17, 2020

Uh oh!

Dotnet-GitSync-Bot commented Jun 17, 2020

Uh oh!

viewizard commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Jun 17, 2020

Uh oh!

vitek-karas commented Jun 17, 2020

Uh oh!

viewizard commented Jun 17, 2020

Uh oh!

viewizard commented Jun 23, 2020

Uh oh!

vitek-karas commented Jun 23, 2020

Uh oh!

viewizard commented Jun 23, 2020

Uh oh!

jkotas commented Jun 23, 2020

Uh oh!

viewizard commented Jun 25, 2020

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jun 25, 2020

Uh oh!

viewizard commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BruceForstall commented Jun 26, 2020

Uh oh!

BruceForstall commented Jun 26, 2020

Uh oh!

viewizard commented Jun 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viewizard commented Jun 17, 2020 •

edited

Loading

viewizard commented Jun 25, 2020 •

edited

Loading