Faculty of Information Technology
Software Engineering Group

Opened 10 months ago

Last modified 8 months 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 10 months 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 10 months 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 10 months 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 10 months 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 9 months 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 9 months 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 9 months 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 9 months 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 9 months 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 9 months 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 9 months 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 9 months 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 9 months ago.
comment fix
libbasic_issue250_patches.7z (8.2 KB) - added by Patrik Svestka 8 months ago.
regression_tests_issue250_Win32OperatingSystemTest.7z (13.7 KB) - added by Patrik Svestka 8 months ago.
regression_tests_issue250_Win32OperatingSystemTest_v2.7z (14.1 KB) - added by Patrik Svestka 8 months 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 8 months ago.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 10 months ago by Patrik Svestka

Description: modified (diff)

comment:2 Changed 10 months ago by Patrik Svestka

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 10 months ago by Patrik Svestka (previous) (diff)

Changed 10 months ago by Patrik Svestka

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

comment:3 Changed 10 months ago by Patrik Svestka

Status: newtesting

comment:4 Changed 10 months ago by Jan Vrany

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 Changed 10 months ago by Patrik Svestka

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 10 months ago by Patrik Svestka (previous) (diff)

comment:6 Changed 10 months ago by Patrik Svestka

Status: needs_worktesting

comment:7 Changed 9 months ago by Patrik Svestka

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.

Changed 9 months ago by Patrik Svestka

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

comment:8 Changed 9 months ago by Patrik Svestka

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

comment:9 in reply to:  8 Changed 9 months ago by Jan Vrany

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 Changed 9 months ago by Patrik Svestka

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

Last edited 9 months ago by Patrik Svestka (previous) (diff)

comment:11 Changed 9 months ago by Patrik Svestka

Here a the newest patches.

comment:12 Changed 9 months ago by Patrik Svestka

adding a tiny patch to fix comments at method valueNamed:

comment:13 Changed 8 months ago by Patrik Svestka

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

Changed 8 months ago by Patrik Svestka

Changed 8 months ago by Patrik Svestka

comment:14 Changed 8 months ago by Jan Vrany

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.

Changed 8 months ago by Patrik Svestka

regression tests

comment:15 Changed 8 months ago by Patrik Svestka

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 Changed 8 months ago by Patrik Svestka

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.