Fix NVDA crash when formatting a timestamp of a log entry under some versions of Universal CRT #12250
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ountered under particular versions of Universal CRT.
|
This still needs some work - marking as a draft for now. |
|
@lukaszgo1 - this fix looks good to me, what more work needs to be done? I saw wxPython has this fix implemented - but unreleased. Maybe all we need is a comment referencing that this hack can go when wx is updated |
|
Critical CC: @RobinD42
From: Sean Budd ***@***.***>
Sent: Monday, April 5, 2021 10:21 PM
To: nvaccess/nvda ***@***.***>
Cc: Subscribed ***@***.***>
Subject: Re: [nvaccess/nvda] Backport of `InitLocale` from Wx Python 4.1.2 to workaround crash under some versions of Windows 10 (#12250)
@lukaszgo1 <https://github.com/lukaszgo1> - this fix looks good to me, what more work needs to be done?
I saw wxPython has this fix implemented - but unreleased. Maybe all we need is a comment referencing that this hack can go when wx is updated
wxWidgets/Phoenix@ <wxWidgets/Phoenix@040c59f> 040c59f
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#12250 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4AXEEHPDAFCLMRDY2AEX3THKK3VANCNFSM42CVVELQ> .
|
|
I suggest to make it conditional. The |
…PI calls to avoid crashes under Server 2019 and Windows 10 1809
See test results for failed build of commit fd500a08c3 |
- Remove star imports - Remove duplicate definition of openProcess
…e unused imports.
NVDA is Windows only, and it checks for the Python version on startup failing to run under any other version than py 3.8. |
|
@seanbudd Sorry for the delay. As I've explained in the updated PR description backporting this method has not been enough to fix this crash. Now this is ready for review. |
lukaszgo1
changed the title
InitLocale from Wx Python 4.1.2 to workaround crash under some versions of Windows 10
seanbudd
requested changes
Apr 8, 2021
|
@seanbudd All your changes are now addressed. |
seanbudd
approved these changes
Apr 8, 2021
There was a problem hiding this comment.
Thank you @lukaszgo1 , and thanks @CyrilleB79 and @kvark128 for testing and investigating
Merged
7 tasks
feerrenrut
pushed a commit
that referenced
this pull request
Apr 21, 2021
* Use FILETIME from winKernel in objidl and SAPI4 internal driver rather than defining it in every file. Follow up of PR #12250:
lukaszgo1
added a commit
to lukaszgo1/nvda
that referenced
this pull request
Aug 19, 2021
… no longer needed since we're now setting Python's locale to a correct Win32 ones.
This was referenced Aug 19, 2021
Merged
lukaszgo1
added a commit
to lukaszgo1/nvda
that referenced
this pull request
Aug 22, 2021
…as it is no longer needed since we're now setting Python's locale to a correct Win32 ones." This reverts commit d2491a4.
seanbudd
pushed a commit
that referenced
this pull request
Aug 26, 2021
…itializing language. (#12753) Fix-up of #12214 Follow up to #12250 Fixes root cause of #12160 Summary of the issue: After NVDA sets its interface language it also attempts to set Python locale to the current language. Before PR #12214 we were just passing the current language code to locale.setlocale and catching all possible exceptions. Unfortunately Python setlocale is pretty useless on Windows as it tries to normalize the locale name according to the Unix standards (see Python issue 37945. When we were using wxPython older than 4.1 this was generally not an issue (except for some specific locales under a particular configurations such as one described in #12160 ) since wxPython was setting locale to a valid one so the problem remained unnoticed. This broke with wxPython 4.1 and the fix was attempted in #12214 but it still was setting Python locale to Unix locales. This is causing two problems: We are Windows only so it is important to set locales to something reasonable If the locale that is unknown to Windows is set some versions of CRT assumes that the code page of such locale is Utf8 which causes a crash when using time.localtime (that was the underlying cause of Crash of the latest alpha NVDA builds on a system with Russian locale #12160) Description of how this pull request fixes the issue: Since Python locale.setlocale and locale.getlocale cannot be trusted on Windows the approach I took avoids all the normalization they offer by creating a valid Windows locale string using getLocaleInfoEx in a form which Python no longer normalizes. This string is then passed to locale.setlocale. The new functionality has pretty solid unit test coverage, previous unit tests for languageHandler are also updated to account for these changes. As suggested during the review this PR also consolidates all NLS constants we use when retrieving various locale related information from Windows into an enum in languageHandler. The old constants are deprecated and would no longer be available in 2022.1. Specific fix for #12160 from #12250 cannot be removed because even though we're setting only complete locales i.e. with a code page and for languages such as Russian or German_Switzerland this crash would no longer occur NVDA supports some locales such as Hindi for which code page is indeed Utf8 and they would still cause a crash. Testing strategy: Manual testing: On Windows 7, Server 2012, Windows 10 1809 (on which I was previously able to reproduce #12160 ) and latest preview of Windows 11 started NVDA with both specific language set in preferences and with the language set to default. Made sure that when 'user default' is selected appropriate language is chosen and locale (as retrieved by locale.setlocale(locale.LC_ALL)) is set into a locale of a form englishLanguageName_englishCountryName.ANSICodePage. Repeated these tests with some languages known to cause troubles in the past: Aragonese - locale set to Windows locale German_Switzerland and Hindi - made sure that Crash of the latest alpha NVDA builds on a system with Russian locale #12160 has not been reintroduced Executed unit tests on all above mentioned versions of Windows made sure they pass.
5 tasks
michaelDCurran
pushed a commit
that referenced
this pull request
Nov 21, 2021
…er after abandoned #10089) (#12958) Fixes #10044 Supersedes #10089 Summary of the issue: It can sometimes be useful to specify NVDA's language from the CLI. One use case which has been mentioned in #10044 is when language has been accidentally changed to a foreign one. This can also be useful for developers (it would certainly be a welcome addition for me when working on #12250 and #12753). Description of how this pull request fixes the issue: Add a new --lang command line parameter. It accepts either "Windows" or the usual "en" / "de_CH" codes, with a little tolerance regarding to case and dashes. As discussed during the review in #10089 the parameter is treated as a temporary override (i.e. it does not affect configuration at all and is effective only until NVDA is restarted). As a bonus this PR adds unit tests for languageHandler.normalizeLanguage and languageHandler.getAvailableLanguages (tests for the former were necessary to confirm it provides us with normalization we need, and since I had to split the latter one into two functions added tests made it possible to confirm that current behavior has not changed).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Fixes #12160
Related to https://bugs.python.org/issue36792
Summary of the issue:
As described in This Python bug report (many thanks to @kvark128 for his research) under some versions of Universal CRT Python can crash when formatting time with
time.localtimeand locale is set to a language which contains underscore it its name.Unfortunately Python developers decided not to workaround this bug on their side since this crash is fixed in more recent version of the Universal CRT. Sadly the version containing this bug is present in Windows 10 1809 which is a base version for Server 2019 and Windows 10 LTSC 2019 both are supported until 2028 and we should support them too if possible. For users this manifests in a crash of NVDA since default log formatter attempts to format current time with
time.localtimewhen writing anything to the log.Description of how this pull request fixes the issue:
wx.app. While this is not necessary any longer whentime.localtimeis not used we are managing locale ourselves inlanguageHandlerso it makes sense to do it just in one place.time.localtimeis not used any more. I've provided an alternative implementation which relies on win32 API calls to convert time_t to a string representation.While at it I've also removed some unused imports from
logHandlerand cleaned up star imports fromwinKernel.Testing strategy:
On both Windows 7 and Windows 10 1809 in Polish started NVDA with the following language settings:
Confirmed there are no crashes whereas under Windows 10 1809 it was impossible to start NVDA both when set to user default and when set to de_CH before fix from this pr.
In theory it should be possible to create an unit test for this (AppVeyor machines are on Server 2019 after all) annoyingly I am unable to reproduce this crash from the Python interpreter. I'll keep trying but this should not block this PR.
Known issues with pull request:
formatTimemethod only for Windows versions on which UCRT has this bug, but since I am not sure in which version of Windows 10 it started nor on which it has been fixed it seemed safer to always use this new approach.time.localtimeis not used anywhere in our code it is theoretically possible it is still used in one of our dependencies or somewhere in standard Python library. This is very hard to track down so unless someone reports a crash I see no way to detect such cases.Change log entry:
Bug fixes:
It is once again possible to use NVDA in a languages containing underscores in the locale name such as de_CH on Windows 10 1803 and 1809.
Please note that this crash occurred also in 2020.4 for these languages hence the change log entry.
Code Review Checklist:
This checklist is a reminder of things commonly forgotten in a new PR.
Authors, please do a self-review and confirm you have considered the following items.
Mark items you have considered by checking them.
You can do this when editing the Pull request description with an x:
[ ]becomes[x].You can also check the checkboxes after the PR is created.