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

[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).

[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.

[Patrik Svestka]

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

[Patrik Svestka]

Tests for the patch

[Jan Vrany]

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?

[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.

[Patrik Svestka]

tests

[Patrik Svestka]

The actual fixes

[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.

[Patrik Svestka]

registry reading patch

[Patrik Svestka]

Tests for read-only

[Patrik Svestka]

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

[Patrik Svestka]

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

[Patrik Svestka]

First patch

[Patrik Svestka]

Second patch

[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.

[Patrik Svestka]

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

[Patrik Svestka]

Here a the newest patches.

[Patrik Svestka]

UTF16 reading from registry re-fix

[Patrik Svestka]

Tests for unicode reading

[Patrik Svestka]

additiona tests for index methods

[Patrik Svestka]

adding a tiny patch to fix comments at method valueNamed:

[Patrik Svestka]

comment fix

[Patrik Svestka]

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

[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.

[Patrik Svestka]

regression tests

[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.

[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.