Opened 5 years ago

Last modified 5 years ago

#250 testing defect

Smalltak/X is reading Windows Registry only in ASCII but registry is UTF16

Reported by: patrik.svestka@… Owned by:
Priority: major Milestone:
Component: default Keywords:
Cc: Also affects CVS HEAD (eXept version): no

Description (last modified by patrik.svestka@…)

Windows registry is in Unicode (produces UTF-16LE with BOM files). Smalltalk/X.

If you want to read values from registry is supports it via method

Win32OperatingSystem::RegistryEntry >> valueNamed:

However, there this currently works in ASCII which is insufficient as Windows Registry is in Unicode.

There was apparently an attempt to read it in unicode:

#define xxUSE_UNICODE
#ifdef USE_UNICODE
# define xRegQueryValueEx  RegQueryValueExW
# define CHAR             short
#else
# define RegQueryValueEx  RegQueryValueExA
# define CHAR             char
#endif

...

#ifdef USE_UNICODE
		    retVal = __MKU16STRING(dataBuffer ? dataBuffer : quickData.smallDataBuffer);
#else
		    retVal = __MKSTRING(dataBuffer ? dataBuffer : quickData.smallDataBuffer);
#endif

...

#ifdef USE_UNICODE
			    s = __MKU16STRING(cp0); __ArrayInstPtr(stringArray)->a_element[i] = s; __STORE(stringArray, s);
#else
			    s = __MKSTRING(cp0); __ArrayInstPtr(stringArray)->a_element[i] = s; __STORE(stringArray, s);
#endif

When the unicode code is turned on mostly rubbish is produced - some chineese characters.

For example - finding windows version:

key := Win32OperatingSystem::RegistryEntry key:'HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion'.
     key valueNamed: 'BuildLab'.

Should produce something like this:

'15063.rs2_release.170317-1834'

If the xx are removed and the Unicode version is used then it produces something like this:

'㔱㘰⸳獲弲敲敬獡⹥㜱㌰㜱ㄭ㌸4se.170317-1834'

As you can see there is a partial result but maybe some buffer is overwritten?

Also the value aValueName can't be unicode, which is permitted in Windows Registry, as it is defined as __isStringLike(aValueName).

Attachments (17)

libbasic_fix_1_of_1_rev_6bd6624bd026_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (21.5 KB ) - added by patrik.svestka@… 5 years ago.
Patch to make read from Windows registry unicode (UTF-16)
regression_fix_1_of_1_rev_46d264c34d05_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (104.0 KB ) - added by patrik.svestka@… 5 years ago.
Tests for the patch
regression_fix_1_of_1_rev_2ae8f9cdee71_Issue__252__Smalltak_X_is_writing_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (11.4 KB ) - added by patrik.svestka@… 5 years ago.
tests
libbasic_fix_1_of_1_rev_b1ce004d9025_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (21.1 KB ) - added by patrik.svestka@… 5 years ago.
The actual fixes
libbasic_fix_1_of_1_rev_4debcdcaf264_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (21.6 KB ) - added by patrik.svestka@… 5 years ago.
registry reading patch
regression_fix_1_of_1_rev_4d2e6776489f_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (123.3 KB ) - added by patrik.svestka@… 5 years ago.
Tests for read-only
regression_fix_1_of_1_rev_6b07361f12c6_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (108.9 KB ) - added by patrik.svestka@… 5 years ago.
Removing duplicate tests - after renaming them they somehow stayed …
libbasic_fix_1_of_2_rev_4debcdcaf264_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (21.6 KB ) - added by patrik.svestka@… 5 years ago.
First patch
libbasic_fix_2_of_2_rev_7637a4b5ecc9_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (2.8 KB ) - added by patrik.svestka@… 5 years ago.
Second patch
libbasic_fix_1_of_1_rev_76c9eea92a48_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (18.0 KB ) - added by patrik.svestka@… 5 years ago.
UTF16 reading from registry re-fix
regression_fix_1_of_1_rev_8e0b72418799_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (1.4 KB ) - added by patrik.svestka@… 5 years ago.
Tests for unicode reading
regression_fix_1_of_1_rev_01c8cc076b08_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (18.1 KB ) - added by patrik.svestka@… 5 years ago.
additiona tests for index methods
libbasic_fix_1_of_1_rev_a023e524e56e_Issue__250__Smalltak_X_is_reading_Windows_Registry_only_in_ASCII_but_registry_is_UTF16.patch (1018 bytes ) - added by patrik.svestka@… 5 years ago.
comment fix
libbasic_issue250_patches.7z (8.2 KB ) - added by patrik.svestka@… 5 years ago.
regression_tests_issue250_Win32OperatingSystemTest.7z (13.7 KB ) - added by patrik.svestka@… 5 years ago.
regression_tests_issue250_Win32OperatingSystemTest_v2.7z (14.1 KB ) - added by patrik.svestka@… 5 years ago.
regression tests
libbasic_fix_1_of_1_rev_ecd8104acb9a_Issue__250_User_input_checks___raise_an_error_when_the_method_parameter_does_not_fit_the_requirements.patch (3.0 KB ) - added by patrik.svestka@… 5 years ago.

Download all attachments as: .zip

Change History (33)

comment:1 by patrik.svestka@…, 5 years ago

Description: modified (diff)

comment:2 by patrik.svestka@…, 5 years ago

I have fixed (made a unicode version) of each read registry function. I have added also tests that show that the changes are working.

I have also added some additional comments - most of them taken from MSDN and some commented out debugging messages.

The ASCII version of the functions will be removed when all the functions that change registries (#252) will be unicode too.

Last edited 5 years ago by patrik.svestka@… (previous) (diff)

by patrik.svestka@…, 5 years ago

Patch to make read from Windows registry unicode (UTF-16)

comment:3 by patrik.svestka@…, 5 years ago

Status: newtesting

comment:4 by jan vrany, 5 years ago

Status: testingneeds_work

As for libbasic_fix_1_of_1_rev_6bd6624bd026...patch:

-     a remote computers registry."
-
-    |newEntry remoteHandle errorNumber|
-
+     a remote computers registry.
+     Note: The registry key must be form a predefined list defined by Microsoft."
+
+    |aHostName newEntry remoteHandle errorNumber|
+
+    aHostName := hostName notEmptyOrNil ifTrue:[hostName asUnicode16StringZ].
+    

A local name aHostName is not very goor. It suggest that there's a class HostName and that the variable is of that type - at least this is a long-term tradition in Smalltalk code. I'd suggest renaming it to - say - hostNameUtf16z or hostNameW or somethibg like that.


-    if (__isExternalAddressLike(__INST(handle)) && __isStringLike(hostName)) {
+#ifdef USE_UNICODE
+#undef RegConnectRegistry
+#endif
+#ifdef USE_UNICODE
+  #define RegConnectRegistry  RegConnectRegistryW
+#else
+  #define RegConnectRegistry  RegConnectRegistryA
+#endif

#undef RegConnectRegistry always - because otherwise you'd have the warning in case USE_UNICODE is undef'd...


+ */    
+
+    if (__isExternalAddressLike(__INST(handle)) && __isUnicode16String(aHostName)) {
         myKey = (HKEY)__externalAddressVal(__INST(handle));
-        if ((_retVal = RegConnectRegistryA(__stringVal(hostName), myKey, &remoteKey)) == ERROR_SUCCESS) {
+        if ((_retVal = RegConnectRegistry(__unicode16StringVal(aHostName), myKey, &remoteKey)) == ERROR_SUCCESS) {
             remoteHandle = __MKEXTERNALADDRESS(remoteKey);
         } else {
             if ((_retVal != ERROR_PATH_NOT_FOUND)

To be honest, I'd prefer using RegConnectRegistryW() directly rather than indirectly with the
help of #define above. Here you test for passwd string (aHostName) being actually a Unicode 16 string and if so, you MUST use RegConnectRegistryW(). More over, this code would break when USE_UNICODE is not defined. I'd be happy if this method would be unicode-only, i.e., ignoring USE_UNICODE. Would make things much simpler.


Rest is similar. Actually, after seeing the code, I'd just dropped support for ASCII and always worked using Unicode functions...

Makes sense?

comment:5 by patrik.svestka@…, 5 years ago

It makes perfect sense. I have fixed all the issues above and even found a nasty bug that propagated only under certain conditions.

I'm including a patch which includes all fixes && tests that also already include some tests for the writing part from sister ticket #252.

Last edited 5 years ago by patrik.svestka@… (previous) (diff)

comment:6 by patrik.svestka@…, 5 years ago

Status: needs_worktesting

comment:7 by patrik.svestka@…, 5 years ago

Sigh, I have found another bug so I had to re-patch it.

I have also restructred the tests' commit so there are only tests for registry read in the patch.

by patrik.svestka@…, 5 years ago

Removing duplicate tests - after renaming them they somehow stayed ...

comment:8 by patrik.svestka@…, 5 years ago

Today I have found one more issue:
I'm including the patches.

in reply to:  8 comment:9 by jan vrany, 5 years ago

Replying to Patrik Svestka:

Today I have found one more issue:
I'm including the patches.

Regarding libbasic_fix_1_of_2_rev_4debcdcaf264_Issue...:

+ * note: This actually means that the if the path fits within 255 chacaters you could get another 255 characters for the key itself.
+ *       This could help if you are having issues with the registry path lenght.
+ */
+    wchar_t nameBuffer[256];  // 256 is due to Key name limit (including path)
+    DWORD nameSize = sizeof(nameBuffer);
+
+ /* lpClass (classNameBuffer):
+  * A pointer to a buffer that receives the user-defined class of the enumerated subkey. 
+  */
+    wchar_t classNameBuffer[256];
+    DWORD classNameSize = sizeof(classNameBuffer);

I think there's bug in DWORD nameSize = sizeof(nameBuffer);. sizeof() gives you thw the size of type in *bytes*, whereas - according to MSDN - RegEnumKeyExW()'s lpcchName (in/out) parameter is in *characters*. So I'd suggest something like DWORD nameSize = sizeof(nameBuffer) / sizeof(wchar_t);.

This is a repeating problem thorough the patch.

comment:10 by patrik.svestka@…, 5 years ago

Great catch, I've missed that - I'll do complete review of the functions.

Last edited 5 years ago by patrik.svestka@… (previous) (diff)

comment:11 by patrik.svestka@…, 5 years ago

Here a the newest patches.

comment:12 by patrik.svestka@…, 5 years ago

adding a tiny patch to fix comments at method valueNamed:

comment:13 by patrik.svestka@…, 5 years ago

Rebasing all the patches to current HEAD at BB. For both patches and the tests.

by patrik.svestka@…, 5 years ago

comment:14 by jan vrany, 5 years ago

I applied latest patches and tun tests. The Win32OperatingSystemTest >> testReadRemoteKeyOnHost consistently fails.

Backtrace:

[OsError]: OperatingSystem error - 53 (ERROR_OTHER: 53) in process NewSystemBrowser [82]

OSErrorHolder >> reportError  {8036352} [11]
Win32OperatingSystem::RegistryEntry >> remoteKeyOnHost: 'localhost' {8040448} [54]
RegressionTests::Win32OperatingSystemTest >> testReadRemoteKeyOnHost  {8044544} [11]
RegressionTests::Win32OperatingSystemTest(Object) >> perform: #testReadRemoteKeyOnHost {8048640} [32]
RegressionTests::Win32OperatingSystemTest(TestCase) >> performTest {8052736} [2]
[] in TestCase>>runCase >> value {8056832} [10]
Block >> ensure: [] in TestCase>>runCase {8060928} [12]
Block >> sunitEnsure: [] in TestCase>>runCase {8065024} [3]
RegressionTests::Win32OperatingSystemTest(TestCase) >> runCase  {8069120} [11]
[] in TestCase>>debug >> value {8073216} [10]
Block >> on:do: TestFailure [] in TestCase>>debug {8077312} [11]
Block >> sunitOn:do: TestFailure [] in TestCase>>debug {8081408} [3]
[] in TestCase>>debug >> value  {8085504} [14]
Block >> ensure: [] in TestCase>>debug {8089600} [12]
Block >> sunitEnsure: [] in TestCase>>debug {8093696} [3]
RegressionTests::Win32OperatingSystemTest(TestCase) >> debug  {8097792} [37]
[] in Tools::TestRunnerMini>>debug >> value {8101888} [24]
Block >> ifCurtailed: [] in Tools::TestRunnerMini>>debug {8105984} [14]
[] in Tools::TestRunnerMini>>debug >> value: RegressionTests::Win32OperatingSystemTest>>#testReadRemoteKeyOnHost {8110080} [25]
Array >> from:to:do: 1 1 [] in Tools::TestRunnerMini>>debug {8114176} [7]
OrderedCollection >> do: [] in Tools::TestRunnerMini>>debug {8118272} [4]
[] in Tools::TestRunnerMini>>debug >> value  {8122368} [22]
Block >> ensure: [] in Tools::TestRunnerMini>>debug {8126464} [12]
[] in Tools::TestRunnerMini>>debug >> value  {8130560} [47]
Tools::TestRunnerEmbedded(Tools::TestRunnerMini) >> debug  {8134656} [51]
Tools::TestRunnerEmbedded(Object) >> perform: #debug {8138752} [32]
Tools::TestRunnerEmbedded(Object) >> perform:withOptionalArgument: #debug true {8142848} [8]
MessageSend >> value: true {8146944} [4]
ButtonController >> performAction  {8151040} [35]
ButtonController >> buttonRelease:x:y: 1 45 16 {8155136} [37]
ButtonController(Object) >> perform:withArguments: #buttonRelease:x:y: #(1 45 16) {8159232} [169]
Button(DisplaySurface) >> dispatchEvent:type:arguments:withFocusOn:delegate: WindowEvent::ButtonReleaseEvent(#buttonRelease:x:y: view: Button args: #(1 45 16)) #buttonRelease:x:y: #(1 45 16) a Tools::CodeView2::TextView true {8163328} [238]
Button(DisplaySurface) >> dispatchEvent:withFocusOn:delegate: WindowEvent::ButtonReleaseEvent(#buttonRelease:x:y: view: Button args: #(1 45 16)) a Tools::CodeView2::TextView true {8167424} [5]
[] in WindowGroup>>processEventsWithModalGroup: >> value  {8171520} [221]
Block >> on:do:ensure: LastEventQuery (private in WindowGroup) [] in WindowGroup>>processEventsWithModalGroup: [] in WindowGroup>>processEventsWithModalGroup: {8175616} [15]
WindowGroup >> processEventsWithModalGroup: nil {8179712} [232]
[] in WindowGroup>>eventLoopWhile:onLeave: >> value  {8183808} [152]
SignalSet >> handle:do: [] in WindowGroup>>eventLoopWhile:onLeave: [] in WindowGroup>>eventLoopWhile:onLeave: {8187904} [14]
[] in WindowGroup>>eventLoopWhile:onLeave: >> value  {8192000} [81]
Block >> ensure: [] in WindowGroup>>startupWith: (optimized) {8196096} [12]
[] in WindowGroup>>eventLoopWhile:onLeave: >> value {8200192} [184]
WindowGroup::WindowGroupQuery class(Notification class) >> answer:do: WindowGroup(NewSystemBrowser) [] in WindowGroup>>eventLoopWhile:onLeave: {8204288} [10]
[] in WindowGroup::WindowGroupQuery class>>answer:do: >> value {8208384} [4]
Block >> ensure: [] in WindowGroup::WindowGroupQuery class>>answer:do: (optimized) {8212480} [12]
WindowGroup::WindowGroupQuery class >> answer:do: WindowGroup(NewSystemBrowser) [] in WindowGroup>>eventLoopWhile:onLeave: {8216576} [5]
WindowGroup >> eventLoopWhile:onLeave: [] in WindowGroup>>startupWith: (optimized) [] in WindowGroup>>startupWith: (optimized) {8220672} [25]
[] in WindowGroup>>startupWith: >> value {8224768} [26]
Block >> ensure: [] in WindowGroup>>startupWith: {8228864} [12]
[] in WindowGroup>>startupWith: >> value  {8232960} [27]
ExceptionHandlerSet >> handleDo: [] in WindowGroup>>startupWith: {8237056} [14]
[] in Process>>start >> value {8241152} [34]
Block >> on:do:ensure: SignalSet(TerminateProcessRequest RestartProcessRequest AbortAllOperationRequest) [] in Process>>start [] in Process>>start {8245248} [15]
Process >> start  {8249344} [35]
UndefinedObject >> nil  {8253440} [0]

Perhaps one has to enable some sort of remote management? This is on Windows 10.

by patrik.svestka@…, 5 years ago

regression tests

comment:15 by patrik.svestka@…, 5 years ago

Ah, the text is missing for the regression_tests_issue250_Win32OperatingSystemTest_v2.7z​. I have adjusted a test for remoteKeyOnHost:. The test is now checking if the connection to localhost succeeds or not. If the connection fails it will skip the whole test. For the test to be run successfuly you need to have service RemoteRegistry turned on.

comment:16 by patrik.svestka@…, 5 years ago

Thank you for integrating the patches. Please integrate this patch - libbasic_fix_1_of_1_rev_ecd8104acb9a_Issue__250_User_input_checks___raise_an_error_when_the_method_parameter_does_not_fit_the_requirements.patch after importing #252's https://swing.fit.cvut.cz/projects/stx-jv/attachment/ticket/252/registry_patches_01.7z.

Note: See TracTickets for help on using tickets.