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

[James Hawkins]

On 6/23/07, Andrew Talbot  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.