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

Calling DateTime.Now causes memory corruption when machine has many language packs installed #55307

Closed
vdanche opened this issue Jun 28, 2021 · 20 comments · Fixed by #56171
Closed
Assignees
Milestone

Comments

@vdanche
Copy link
Member

vdanche commented Jun 28, 2021

Change in initialization code of TimeZoneInfo introduced by #52992 leads to AV or native heap corruption on Windows.

Original issue description

Repro steps:
Install .NET 6 prevew 6 SDK-6.0.100-preview.6.21327.1(runtime-6.0.0-preview.6.21325.7) from https://github.com/dotnet/installer from Main branch

In CLI, execute below command.
mkdir ProC#
cd ProC#
dotnet new sln -o CSCLI
cd CSCLI
dotnet new console -o CSNC21App
dotnet new classlib -o CSNS20Lib
dotnet sln add CSNC21App\CSNC21App.csproj
dotnet sln add CSNS20Lib\CSNS20Lib.csproj

Expected Result:
No any errors

Actual Result:
.NET Host stop working.
Fatal error. System.AccessViolationException: Attempted to read or write protect
ed memory. This is often an indication that other memory is corrupt.
at Interop+Kernel32.GetFileMUIPath(UInt32, System.String, Char*, UInt32 ByRef
, Char*, UInt32 ByRef, UInt64 ByRef)
at System.TimeZoneInfo.GetFileMuiPath(System.String, System.Globalization.Cul
tureInfo)
at System.TimeZoneInfo.GetCachedFileMuiPath(System.String, System.Globalizati
on.CultureInfo)
at System.TimeZoneInfo.GetLocalizedNameByMuiNativeResource(System.String, Sys
tem.Globalization.CultureInfo)
at System.TimeZoneInfo.GetLocalizedNamesByRegistryKey(Internal.Win32.Registry
Key, System.String ByRef, System.String ByRef, System.String ByRef)
at System.TimeZoneInfo.TryGetTimeZoneFromLocalMachine(System.String, System.T
imeZoneInfo ByRef, System.Exception ByRef)
at System.TimeZoneInfo.TryGetTimeZoneFromLocalMachine(System.String, Boolean,
System.TimeZoneInfo ByRef, System.Exception ByRef, CachedData)
at System.TimeZoneInfo.TryGetTimeZoneUsingId(System.String, Boolean, System.T
imeZoneInfo ByRef, System.Exception ByRef, CachedData, Boolean)
at System.TimeZoneInfo.TryGetTimeZone(System.String, Boolean, System.TimeZone
Info ByRef, System.Exception ByRef, CachedData, Boolean)
at System.TimeZoneInfo.GetLocalTimeZone(CachedData)
at System.TimeZoneInfo+CachedData.CreateLocal()
at System.TimeZoneInfo.GetDateTimeNowUtcOffsetFromUtc(System.DateTime, Boolea
n ByRef)
at System.DateTime.get_Now()
at Microsoft.DotNet.Cli.Program.Main(System.String[])

image
Problem signature:
Problem Event Name: APPCRASH
Application Name: dotnet.exe
Application Version: 6.0.21.32507
Application Timestamp: 60d615d4
Fault Module Name: coreclr.dll
Fault Module Version: 6.0.21.32507
Fault Module Timestamp: 60d60cb9
Exception Code: c0000005
Exception Offset: 00000000001cb469
OS Version: 6.1.7601.2.1.0.256.27
Locale ID: 1033
Additional Information 1: ee7a
Additional Information 2: ee7aebb493adcdf2891586b392e6e5a5
Additional Information 3: c638
Additional Information 4: c638660efa7b5668612ea728d2ef2abb

Read our privacy statement online:
http://go.microsoft.com/fwlink/?linkid=104288&clcid=0x0409

If the online privacy statement is not available, please read our privacy statement offline:
C:\Windows\system32\en-US\erofflps.txt

Note:
It's a inconsistent issue.
dotnet --info
image

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@vdanche
Copy link
Member Author

vdanche commented Jun 28, 2021

MSBuild .zip, MSbuild.dll under \sdk\6.0.100-preview.6.21327.1 has been attached.

@vdanche vdanche changed the title With net6 preview 6 SDK installed, .NET Host stop working occasionally when executing dotnet command in CLI. With net6 preview 6 SDK installed, .NET Host stop working occasionally when executing dotnet command in CLI on Win7. Jun 28, 2021
@mangod9
Copy link
Member

mangod9 commented Jun 30, 2021

Based on the exception this could be related to this change: #52992

@AntonLapounov
Copy link
Member

This is an external issue. I will follow up with component owners regarding possible workarounds.

@AntonLapounov AntonLapounov changed the title With net6 preview 6 SDK installed, .NET Host stop working occasionally when executing dotnet command in CLI on Win7. Calling DateTime.Now causes memory corruption when machine has many language packs installed Jul 8, 2021
@AntonLapounov AntonLapounov transferred this issue from dotnet/sdk Jul 8, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Globalization untriaged New issue has not been triaged by the area owner labels Jul 8, 2021
@ghost
Copy link

ghost commented Jul 8, 2021

Tagging subscribers to this area: @tarekgh, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

Machine OS: win7 SP1

Repro steps:
Install .NET 6 prevew 6 SDK-6.0.100-preview.6.21327.1(runtime-6.0.0-preview.6.21325.7) from https://github.com/dotnet/installer from Main branch

In CLI, execute below command.
mkdir ProC#
cd ProC#
dotnet new sln -o CSCLI
cd CSCLI
dotnet new console -o CSNC21App
dotnet new classlib -o CSNS20Lib
dotnet sln add CSNC21App\CSNC21App.csproj
dotnet sln add CSNS20Lib\CSNS20Lib.csproj

Expected Result:
No any errors

Actual Result:
.NET Host stop working.
Fatal error. System.AccessViolationException: Attempted to read or write protect
ed memory. This is often an indication that other memory is corrupt.
at Interop+Kernel32.GetFileMUIPath(UInt32, System.String, Char*, UInt32 ByRef
, Char*, UInt32 ByRef, UInt64 ByRef)
at System.TimeZoneInfo.GetFileMuiPath(System.String, System.Globalization.Cul
tureInfo)
at System.TimeZoneInfo.GetCachedFileMuiPath(System.String, System.Globalizati
on.CultureInfo)
at System.TimeZoneInfo.GetLocalizedNameByMuiNativeResource(System.String, Sys
tem.Globalization.CultureInfo)
at System.TimeZoneInfo.GetLocalizedNamesByRegistryKey(Internal.Win32.Registry
Key, System.String ByRef, System.String ByRef, System.String ByRef)
at System.TimeZoneInfo.TryGetTimeZoneFromLocalMachine(System.String, System.T
imeZoneInfo ByRef, System.Exception ByRef)
at System.TimeZoneInfo.TryGetTimeZoneFromLocalMachine(System.String, Boolean,
System.TimeZoneInfo ByRef, System.Exception ByRef, CachedData)
at System.TimeZoneInfo.TryGetTimeZoneUsingId(System.String, Boolean, System.T
imeZoneInfo ByRef, System.Exception ByRef, CachedData, Boolean)
at System.TimeZoneInfo.TryGetTimeZone(System.String, Boolean, System.TimeZone
Info ByRef, System.Exception ByRef, CachedData, Boolean)
at System.TimeZoneInfo.GetLocalTimeZone(CachedData)
at System.TimeZoneInfo+CachedData.CreateLocal()
at System.TimeZoneInfo.GetDateTimeNowUtcOffsetFromUtc(System.DateTime, Boolea
n ByRef)
at System.DateTime.get_Now()
at Microsoft.DotNet.Cli.Program.Main(System.String[])

image
Problem signature:
Problem Event Name: APPCRASH
Application Name: dotnet.exe
Application Version: 6.0.21.32507
Application Timestamp: 60d615d4
Fault Module Name: coreclr.dll
Fault Module Version: 6.0.21.32507
Fault Module Timestamp: 60d60cb9
Exception Code: c0000005
Exception Offset: 00000000001cb469
OS Version: 6.1.7601.2.1.0.256.27
Locale ID: 1033
Additional Information 1: ee7a
Additional Information 2: ee7aebb493adcdf2891586b392e6e5a5
Additional Information 3: c638
Additional Information 4: c638660efa7b5668612ea728d2ef2abb

Read our privacy statement online:
http://go.microsoft.com/fwlink/?linkid=104288&clcid=0x0409

If the online privacy statement is not available, please read our privacy statement offline:
C:\Windows\system32\en-US\erofflps.txt

Note:
It's a inconsistent issue.
dotnet --info
image

Author: vdanche
Assignees: -
Labels:

area-System.Globalization, untriaged

Milestone: -

@ghost
Copy link

ghost commented Jul 8, 2021

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Machine OS: win7 SP1

Repro steps:
Install .NET 6 prevew 6 SDK-6.0.100-preview.6.21327.1(runtime-6.0.0-preview.6.21325.7) from https://github.com/dotnet/installer from Main branch

In CLI, execute below command.
mkdir ProC#
cd ProC#
dotnet new sln -o CSCLI
cd CSCLI
dotnet new console -o CSNC21App
dotnet new classlib -o CSNS20Lib
dotnet sln add CSNC21App\CSNC21App.csproj
dotnet sln add CSNS20Lib\CSNS20Lib.csproj

Expected Result:
No any errors

Actual Result:
.NET Host stop working.
Fatal error. System.AccessViolationException: Attempted to read or write protect
ed memory. This is often an indication that other memory is corrupt.
at Interop+Kernel32.GetFileMUIPath(UInt32, System.String, Char*, UInt32 ByRef
, Char*, UInt32 ByRef, UInt64 ByRef)
at System.TimeZoneInfo.GetFileMuiPath(System.String, System.Globalization.Cul
tureInfo)
at System.TimeZoneInfo.GetCachedFileMuiPath(System.String, System.Globalizati
on.CultureInfo)
at System.TimeZoneInfo.GetLocalizedNameByMuiNativeResource(System.String, Sys
tem.Globalization.CultureInfo)
at System.TimeZoneInfo.GetLocalizedNamesByRegistryKey(Internal.Win32.Registry
Key, System.String ByRef, System.String ByRef, System.String ByRef)
at System.TimeZoneInfo.TryGetTimeZoneFromLocalMachine(System.String, System.T
imeZoneInfo ByRef, System.Exception ByRef)
at System.TimeZoneInfo.TryGetTimeZoneFromLocalMachine(System.String, Boolean,
System.TimeZoneInfo ByRef, System.Exception ByRef, CachedData)
at System.TimeZoneInfo.TryGetTimeZoneUsingId(System.String, Boolean, System.T
imeZoneInfo ByRef, System.Exception ByRef, CachedData, Boolean)
at System.TimeZoneInfo.TryGetTimeZone(System.String, Boolean, System.TimeZone
Info ByRef, System.Exception ByRef, CachedData, Boolean)
at System.TimeZoneInfo.GetLocalTimeZone(CachedData)
at System.TimeZoneInfo+CachedData.CreateLocal()
at System.TimeZoneInfo.GetDateTimeNowUtcOffsetFromUtc(System.DateTime, Boolea
n ByRef)
at System.DateTime.get_Now()
at Microsoft.DotNet.Cli.Program.Main(System.String[])

image
Problem signature:
Problem Event Name: APPCRASH
Application Name: dotnet.exe
Application Version: 6.0.21.32507
Application Timestamp: 60d615d4
Fault Module Name: coreclr.dll
Fault Module Version: 6.0.21.32507
Fault Module Timestamp: 60d60cb9
Exception Code: c0000005
Exception Offset: 00000000001cb469
OS Version: 6.1.7601.2.1.0.256.27
Locale ID: 1033
Additional Information 1: ee7a
Additional Information 2: ee7aebb493adcdf2891586b392e6e5a5
Additional Information 3: c638
Additional Information 4: c638660efa7b5668612ea728d2ef2abb

Read our privacy statement online:
http://go.microsoft.com/fwlink/?linkid=104288&clcid=0x0409

If the online privacy statement is not available, please read our privacy statement offline:
C:\Windows\system32\en-US\erofflps.txt

Note:
It's a inconsistent issue.
dotnet --info
image

Author: vdanche
Assignees: -
Labels:

area-System.Runtime, untriaged

Milestone: -

@AntonLapounov
Copy link
Member

@mattjohnsonpint @tarekgh @stephentoub
The GetFileMUIPath(MUI_USE_INSTALLED_LANGUAGES, ...) call introduced in #52992 leads to AV or native heap corruption and should be avoided. Potential options here:

  • Revert Allow TimeZoneInfo display names to use any of the installed Windows languages #52992.
  • Call GetFileMUIPath(MUI_USE_SEARCH_ALL_LANGUAGES, ...) instead. My understanding is that may start using tzres.dll.mui files under the System32 directory that are present but not officially 'installed'.
  • Enumerate available UI languages using EnumUILanguages and for each suitable candidate language call GetFileMUIPath(MUI_USE_SEARCH_ALL_LANGUAGES, ..., candidate, ...) to check whether we have the tzres.dll.mui resource file for that language.

Note that EnumUILanguages may enumerate either 'installed' or 'installed and licensed' languages. That means some languages may be installed, but not licensed to use.

@tarekgh
Copy link
Member

tarekgh commented Jul 8, 2021

@AntonLapounov

leads to AV or native heap corruption and should be avoided.

Do you have more info why this causing heap corruption? I can look at that later but curious if you already did any investigation.

@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label Jul 8, 2021
@tarekgh tarekgh added this to the 6.0.0 milestone Jul 8, 2021
@AntonLapounov
Copy link
Member

@tarekgh I have sent you OS bug number on email.

@tarekgh tarekgh self-assigned this Jul 8, 2021
@AntonLapounov
Copy link
Member

Another reason to revert #52992/#53119 is that it was not a correct fix in the first place. Since TimeZoneInfo instances are cached, the first used UI culture for a given TimeZoneInfo is stuck forever unless you call TimeZoneInfo.ClearCachedData():

using System;
using System.Globalization;

CultureInfo.CurrentUICulture = CultureInfo.GetCultureInfo("es-MX");
// Outputs Spanish name: "Hora estándar del Pacífico"
Console.WriteLine(TimeZoneInfo.Local.StandardName);

CultureInfo.CurrentUICulture = CultureInfo.GetCultureInfo("en-US");
// Still outputs Spanish name: "Hora estándar del Pacífico"
Console.WriteLine(TimeZoneInfo.Local.StandardName);

@mattjohnsonpint
Copy link
Contributor

@AntonLapounov - that one is tracked by #50311.

@AntonLapounov
Copy link
Member

Great that we already have it filed. However, #50311 is not scheduled for 6.0 and I am not sure that the new Windows behavior:

Thus at any given time, the cache might be in an inconsistent state - having entries belonging to more than one language.

is better than just sticking to English for the properties in question. Though the new behavior removed the difference between Windows and Linux/macOS, I would prefer implementing a correct fix.

@tarekgh
Copy link
Member

tarekgh commented Jul 9, 2021

is better than just sticking to English for the properties in question. Though the new behavior removed the difference between Windows and Linux/macOS, I would prefer implementing a correct fix.

I don't think sticking to English is right. Usually, these properties are used in the UI and not returning them in the right language would be a concern. I think if someone requested these properties with a language that is not matching the cached language, we can re-read that value in that language (and would be ok to pay the cost for that scenario as I am not expecting this is main-stream scenario).

We need to scope the work for what we can do in .NET 6.0 and what we can do later. note, that what @mattjohnsonpint did is a step in enhancing the whole story and it is better than none.

Now, I am questioning if we can depend on ICU for that all the time (when we have ICU) and avoid calling Windows for such localized names.

@AntonLapounov
Copy link
Member

I meant sticking to the preferred UI language set by the OS (not necessary English) as we used to do before #52992/#53119.

I think if someone requested these properties with a language that is not matching the cached language, we can re-read that value in that language (and would be ok to pay the cost for that scenario as I am not expecting this is main-stream scenario).

That makes sense.

@mattjohnsonpint
Copy link
Contributor

Sorry for the delayed response. In general, the reason I was keen on having the display names not strictly tied to the default Windows UI language is because there was already logic in the Unix implementation that uses the CurrentUICulture. It doesn't make sense that the behavior is different on Windows than on Unix.

Also, this has generally been a "gotcha" when localizing websites going way back to early ASP.NET - the only recourse being to use some other copy of the display names, such as provided by my TimeZoneNames library (see mattjohnsonpint/TimeZoneNames#50 and S.O. issues linked therein). It's a big step in the right direction to allow .NET to use whatever language packs are installed. It would be even better to have an option to use the ICU display names on Windows, but I believe that should be either opt-in to avoid conflict with existing behavior or very loudly announced with clear opt-out strategy.

As for the memory corruption - I understand that calling DateTime.Now uses the local time zone, and thus would go through these API calls when first loading that time zone into the cache. (All the more reason to not cache the display names.) But I'm still not certain why the memory would be corrupted. Is the underlying Win32 API broken? Or did I use it incorrectly?

@mattjohnsonpint
Copy link
Contributor

Great that we already have it filed. However, #50311 is not scheduled for 6.0

Well, there is probably time to do that. Personally, I'd rather complete that for 6.0 than punt it to later. @tarekgh, ok if I submit a PR for that one?

@tarekgh
Copy link
Member

tarekgh commented Jul 13, 2021

@mattjohnsonpint I would like to fix this issue before we do anything for #50311. let's sync up offline.

@vdanche
Copy link
Member Author

vdanche commented Jul 19, 2021

We occurred the same issue on August service update SDK: 3.1.118(runtime:3.1.18) on win-arm64.
image

@davidfowl
Copy link
Member

@vdanche I'm not sure it's the same issue, the callstack is different.

@vdanche
Copy link
Member Author

vdanche commented Jul 19, 2021

@vdanche I'm not sure it's the same issue, the callstack is different.

Thanks davidfowl, we file a new bug to track #55900
.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 22, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants