Re: msi: Fix use of uninitialized variable (Coverity) (Try 3)

[James Hawkins]

On 6/23/07, Andrew Talbot <[EMAIL PROTECTED]> wrote:

[Reverting to "Plan A":]
This patch should fix Coverity bug CID-562. Additionally, I have pinpointed
another suspected use-before-initialization bug, for future consideration.

-- Andy.
---
Changelog:
    msi: Fix use of uninitialized variable (Coverity).

diff -urN a/dlls/msi/action.c b/dlls/msi/action.c
--- a/dlls/msi/action.c 2007-06-18 17:52:27.000000000 +0100
+++ b/dlls/msi/action.c 2007-06-23 22:19:03.000000000 +0100
@@ -4668,7 +4668,7 @@

     res = env_set_flags(&name, &deformatted, &flags);
     if (res != ERROR_SUCCESS)
-       goto done;
+       goto done2;

     value = deformatted;

@@ -4676,8 +4676,14 @@
         root = HKEY_LOCAL_MACHINE;

     res = RegOpenKeyExW(root, environment, 0, KEY_ALL_ACCESS, &env);
+    /*
+    * FIXME: RegCloseKey() should not be called with an invalid handle.
+    * So should we simply "goto done2" here, if res != EXIT_SUCCESS,
+    * or does RegOpenKeyEx() have any failure modes in which "env" does
+    * get initialized?
+    */
     if (res != ERROR_SUCCESS)
-        goto done;
+        goto done1;

     if (flags & ENV_ACT_REMOVE)
         FIXME("Not removing environment variable on uninstall!\n");
@@ -4686,14 +4692,14 @@
     res = RegQueryValueExW(env, name, NULL, &type, NULL, &size);
     if ((res != ERROR_SUCCESS && res != ERROR_FILE_NOT_FOUND) ||
         (res == ERROR_SUCCESS && type != REG_SZ))
-        goto done;
+        goto done1;

     if (res != ERROR_FILE_NOT_FOUND)
     {
         if (flags & ENV_ACT_SETABSENT)
         {
             res = ERROR_SUCCESS;
-            goto done;
+            goto done1;
         }

         data = msi_alloc(size);
@@ -4705,12 +4711,12 @@

         res = RegQueryValueExW(env, name, NULL, &type, (LPVOID)data, &size);
         if (res != ERROR_SUCCESS)
-            goto done;
+            goto done1;

         if (flags & ENV_ACT_REMOVEMATCH && (!value || !lstrcmpW(data, value)))
         {
             res = RegDeleteKeyW(env, name);
-            goto done;
+            goto done1;
         }

         size =  (lstrlenW(value) + 1 + size) * sizeof(WCHAR);
@@ -4719,7 +4725,7 @@
         if (!newval)
         {
             res = ERROR_OUTOFMEMORY;
-            goto done;
+            goto done1;
         }

         if (!(flags & ENV_MOD_MASK))
@@ -4749,7 +4755,7 @@
         if (!newval)
         {
             res = ERROR_OUTOFMEMORY;
-            goto done;
+            goto done1;
         }

         lstrcpyW(newval, value);
@@ -4758,8 +4764,9 @@
     TRACE("setting %s to %s\n", debugstr_w(name), debugstr_w(newval));
     res = RegSetValueExW(env, name, 0, type, (LPVOID)newval, size);

-done:
+done1:
     RegCloseKey(env);
+done2:
     msi_free(deformatted);
     msi_free(data);
     msi_free(newval);

This is uglier than the other patch.  The only change needed in this
entire function is to set env to NULL initially.  There's nothing
wrong with calling RegCloseKey(NULL-var) and it's done everywhere in
the code base.

--
James Hawkins