Rejected patches needing review.

[Juan Lang]

Hi Peter,

diff --git a/programs/services/services.c b/programs/services/services.c
index 36ed117..cd71a1f 100644
--- a/programs/services/services.c
+++ b/programs/services/services.c
@@ -106,9 +106,9 @@ static DWORD load_service_config(HKEY hKey, struct
service_entry *entry)
         return err;
     if ((err = load_reg_string(hKey, SZ_DESCRIPTION,  0,
&entry->description)) != 0)
         return err;
-    if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_SERVICE,
&entry->dependOnServices)) != 0)
+    if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_SERVICE, TRUE,
&entry->dependOnServices)) != 0)
         return err;
-    if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_GROUP,
&entry->dependOnGroups)) != 0)
+    if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_GROUP, FALSE,
&entry->dependOnGroups)) != 0)
         return err;

     if ((err = load_reg_dword(hKey, SZ_TYPE,
&entry->config.dwServiceType)) != 0)
diff --git a/programs/services/services.h b/programs/services/services.h
index fd99bf9..cd7de02 100644
--- a/programs/services/services.h
+++ b/programs/services/services.h
@@ -87,7 +87,7 @@ LPWSTR strdupW(LPCWSTR str);
 BOOL check_multisz(LPCWSTR lpMultiSz, DWORD cbSize);

 DWORD load_reg_string(HKEY hKey, LPCWSTR szValue, BOOL bExpand,
LPWSTR *output);
-DWORD load_reg_multisz(HKEY hKey, LPCWSTR szValue, LPWSTR *output);
+DWORD load_reg_multisz(HKEY hKey, LPCWSTR szValue, BOOL bAllowSingle,
LPWSTR *output);
 DWORD load_reg_dword(HKEY hKey, LPCWSTR szValue, DWORD *output);

 static inline LPCWSTR get_display_name(struct service_entry *service)
diff --git a/programs/services/utils.c b/programs/services/utils.c
index 89eb500..191cc5f 100644
--- a/programs/services/utils.c
+++ b/programs/services/utils.c
@@ -102,7 +102,7 @@ failed:
     return err;
 }

-DWORD load_reg_multisz(HKEY hKey, LPCWSTR szValue, LPWSTR *output)
+DWORD load_reg_multisz(HKEY hKey, LPCWSTR szValue, BOOL bAllowSingle,
LPWSTR *output)
 {
     DWORD size, type;
     LPWSTR buf = NULL;
@@ -118,7 +118,7 @@ DWORD load_reg_multisz(HKEY hKey, LPCWSTR szValue,
LPWSTR *output)
         }
         goto failed;
     }
-    if (type != REG_MULTI_SZ)
+    if (!((type == REG_MULTI_SZ) || ((type == REG_SZ) && bAllowSingle)))
     {
         err = ERROR_INVALID_DATATYPE;
         goto failed;


It seems as though the bAllowSingle variable is useless, as the only
caller of it tries first with TRUE and again with FALSE.  If the
registry value is of type REG_SZ, the first call (when bAllowSingle is
TRUE) will succeed.  Similarly, if it's of type REG_MULTI_SZ, the
first call will succeed.  Thus, the second call, with bAllowSingle is
FALSE, is always unnecessary.

Furthermore, since there's only one place that calls load_reg_multisz,
and it accepts both REG_SZ and REG_MULTI_SZ, why not just change
load_reg_multisz always to accept either REG_SZ or REG_MULTI_SZ?
Complicating the interface unnecessarily isn't a good change.

I have a minor quibble with your second patch:
     LPWSTR path = NULL, str;
@@ -174,17 +175,31 @@ static BOOL load_driver(void)

     /* read the executable path from memory */
     size = 0;
-    if (RegQueryValueExW( driver_hkey, ImagePathW, NULL, &type, NULL,
&size )) return FALSE;
-
-    str = HeapAlloc( GetProcessHeap(), 0, size );
-    if (!RegQueryValueExW( driver_hkey, ImagePathW, NULL, &type,
(LPBYTE)str, &size ))
+    if (!RegQueryValueExW( driver_hkey, ImagePathW, NULL, &type, NULL, &size ))
+    {
+        str = HeapAlloc( GetProcessHeap(), 0, size );
You only use str in the then branch of this if statement, so its scope
should be changed:  it should be declared where, where it's allocated
and later freed, instead.
--Juan