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

[James Hawkins]

On 6/23/07, Andrew Talbot <Andrew.Talbot(a)talbotville.com> 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