Re: svchost: Added base implementation of svchost (1/27)

[Juan Lang]

Hi Roy,

+#define WIN32_LEAN_AND_MEAN

That's not needed if you're not including windows.h (which you're not.)

+        if (unicode[i] > 129)

You have an off-by-two error here.  7-bit clean means if (unicode[i] >
127).  You can also use something like if (unicode[i] & 0x80).

Stylistic note, use or ignore at your option:
+        HeapFree(GetProcessHeap(), 0, service_param_key);
+        return FALSE;
(snip)
+        HeapFree(GetProcessHeap(), 0, service_param_key);
+        RegCloseKey(service_hkey);
+        return FALSE;
(snip)
+        HeapFree(GetProcessHeap(), 0, service_param_key);
+        HeapFree(GetProcessHeap(), 0, dll_name_short);
+        RegCloseKey(service_hkey);
+        return FALSE;
(snip)

Some judicious use of goto could reduce code for all your error paths.
 Remember that HeapFree of a NULL pointer is allowed.

+    dll_service_main = GetRegValue(service_hkey, service_main);
+    RegCloseKey(service_hkey);
+    if (!dll_service_main)
+    {
+        service_main_a = UnicodeToAscii(service_main);
+    }
+    else
+    {
+        service_main_a = UnicodeToAscii(dll_service_main);
+        HeapFree(GetProcessHeap(), 0, dll_service_main);

Why not just query dll_service_main and service_main from the registry
as an ASCII value to begin with?
--Juan

svchost: Added base implementation of svchost (1/27)

[Roy Shea]

Hello Juan,

Thank you for the code review!  A quick clarification about HeapFree:
> Some judicious use of goto could reduce code for all your error paths.
> Remember that HeapFree of a NULL pointer is allowed.

The docs for HeapFree state "If this pointer is NULL, the behavior is
undefined."  So in practice the undefined behavior is benign?

Peace,
-Roy

svchost: Added base implementation of svchost (1/27)

[Juan Lang]

Hi Roy,
> The docs for HeapFree state "If this pointer is NULL, the behavior is
> undefined."  So in practice the undefined behavior is benign?

Yes.  There are tests for that, and in fact there's a janitorial
project about removing redundant NULL pointer checks before HeapFree.
Michael Stefaniuc wrote about it here:
http://www.mail-archive.com/wine-devel(a)winehq.org/msg11972.html
You'll still see occasional commits along those lines, often from
Michael, tagged with "found by Smatch."  E.g.:
http://www.winehq.org/pipermail/wine-cvs/2007-November/038118.html

--Juan