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